GH-48631: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB'

[thisisnic]

Rationale for this change

CRAN complains about non-API calls

What changes are included in this PR?

Update those items to use alternative approaches

Are these changes tested?

Well, if the tests pass I think we're good?

Are there any user-facing changes?

Nope

Summary of AI changes

🤖 Generated with Claude Code

Have added comments inline regarding assumptions behind changes, and where I questioned those to try to verify it.

Sources referenced in approach:

• Replace ATTRIB, SET_ATTRIB Rdatatable/data.table#7487 - similar fixes
• https://cran.r-project.org/doc/manuals/r-release/R-ints.html on DUPLICATE_ATTRIB

• GitHub Issue: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB' #48631

[thisisnic]

CI failure R / AMD64 Ubuntu 24.04 R 4.4 Force-Tests true (pull_request) due to unrelated issue (downloading Parquet file from our repo)

[thisisnic]

Alternative approach to try based on conversation with CRAN folks:

The other one is tricky (L152 , 1163, 1194) - you are trying to get the class name of the altrep object for which there is no API AFAICT. ...A workaround is to use something like:

const char *altrep_class_name(SEXP x) {
   SEXP l = PROTECT(Rf_lang2(Rf_install(".Internal"),
			  PROTECT(Rf_lang2(Rf_install("altrep_class"), x))));
   SEXP r = Rf_eval(l, R_BaseEnv);
   UNPROTECT(2);
   return CHAR(STRING_ELT(r, 0));
}

@jonkeane - I'm leaning towards not implementing this approach due to advice from 🤖

🤖 The above suggestion calls into R at runtime to query the class name, while the current PR simply stores the name as a static string when the class is registered. Since Arrow controls its own altrep classes and already has the name at registration time, storing it is simpler, faster, and avoids the overhead of R evaluation.

[yutannihilation]

It seems they added R_altrep_class_name() and R_altrep_class_package() for replacing CAR(ATTRIB(ALTREP_CLASS(x))) and CADR(ATTRIB(ALTREP_CLASS(x))). As this pull request already succeeded to remove this ATTRIB() usage, you might not need this, but I'm sharing here just in case this is useful.

r-devel/r-svn@648c5b1

[thisisnic]

It seems they added R_altrep_class_name() and R_altrep_class_package() for replacing CAR(ATTRIB(ALTREP_CLASS(x))) and CADR(ATTRIB(ALTREP_CLASS(x))). As this pull request already succeeded to remove this ATTRIB() usage, you might not need this, but I'm sharing here just in case this is useful.

r-devel/r-svn@648c5b1

Ah, that's great, thanks! I guess we wanna stick with this workaround for now so that we're still compatible with older versions of R which don't have these new functions but can open a ticket to replace it with them later.

[yutannihilation]

compatible with older versions of R which don't have these new functions

You can probably backport these functions like below. In my understanding, since Writing R Extension explains how to backport R_mapAttrib using ATTRIB, we can still use ATTRIB on R < 4.6.

#if R_VERSION < R_Version(4, 6, 0)
SEXP R_altrep_class_name(SEXP x)
{
    return ALTREP(x) ? CAR(ATTRIB(ALTREP_CLASS(x))) : R_NilValue;
}
#endif

[thisisnic]

I guess they still would most likely get flagged up in our CRAN checks, and I don't see that one listed explicitly; tempted to leave as-is to save the hassle. @jonkeane - given you're submitting it, what do you think?

[yutannihilation]

If I understand correctly, ATTRIB and SET_ ATTRIB are caught only on R-devel. Actually, we see the NOTEs only on R-devel.

https://cran.r-project.org/web/checks/check_results_arrow.html

Anyway, I don't have strong opinion. I think you can choose the one you feel easier and/or safer!

[jonkeane]

I guess they still would most likely get flagged up in our CRAN checks, and I don't see that one listed explicitly; tempted to leave as-is to save the hassle. @jonkeane - given you're submitting it, what do you think?

If I understand correctly, ATTRIB and SET_ ATTRIB are caught only on R-devel. Actually, we see the NOTEs only on R-devel.

The backport very much should work. Here is an example that we did for Rf_findVarInFrame3():

arrow/r/src/arrow_cpp11.h

Lines 390 to 400 in 0b9f06c

	// R_existsVarInFrame doesn't exist before R 4.2, so we need to fall back to
	// Rf_findVarInFrame3 if it is not defined.
	#if R_VERSION >= R_Version(4, 2, 0)
	if (!R_existsVarInFrame(arrow::r::ns::arrow, r6_class)) {
	cpp11::stop("No arrow R6 class named '%s'", r6_class_name);
	}
	#else
	if (Rf_findVarInFrame3(arrow::r::ns::arrow, r6_class, FALSE) == R_UnboundValue) {
	cpp11::stop("No arrow R6 class named '%s'", r6_class_name);
	}
	#endif

So if we want to do that now, that would be cleaner and we won't need to come back to it. But I also would be ok to merge this as is with a follow on to do that cleanup if that's more expedient (with the upcoming release + time we all have available to dedicate to this). If we do defer it and come back to it, mind making a follow up ticket if you haven't already?

[thisisnic]

Right, I see, my mental model of how this works was wrong; - I didn't realise that they run their checks for non-API calls on the built package rather than some kind of static analysis, and hence why the backport stuff is fine. Thanks for the suggestion @yutannihilation and helping me find that other one there @jonkeane. Will merge once CI passes.