Add RNeXML_ prefix to tree (and other) classes

[krlmlr]

I'm seeing weird warnings when I run R CMD check on this package, both the CRAN version and the dev version, after r-lib/pillar@5e9abc0427388657ac790f44ee6bfdafff1d7e50. This commit (and subsequent ones) fails, the previous commit succeeds. Oddly enough, I brought back the crayon import, but the warning persists. I can't replicate with requireNamespace() and unloadNamespace() .

I have no idea what's happening here. Could you please advise?

W  checking whether the namespace can be unloaded cleanly (460ms)
   ---- unloading
   Error in .mergeMethodsTable(generic, mtable, tt, attach) :
     trying to get slot "defined" from an object of a basic class ("environment") with no slots
   Calls: unloadNamespace ... <Anonymous> -> .updateMethodsInTable -> .mergeMethodsTable
   Execution halted

[cboettig]

Thanks for the message @krlmlr ! I dunno -- you're debugging talents are far better than mine!

Sounds like some kind of S4-based namespace collision though? Maybe?

RNeXML defines a large number of S4 classes (matching each complexType in corresponding XML schema defined by NeXML), many of which have quite generic names, e.g.: https://github.com/ropensci/RNeXML/blob/master/R/character_classes.R

[krlmlr]

Thanks. It's the "tree" class that is defined (as an S4 class) both in the cli package and here. With 1e431bbd7a68f6aa3105465c3176800487d27407 (in my fork) the same failure is shown with R CMD check and CRAN pillar, and also shows a warning on R CMD INSTALL.

I think the main lesson is that S4 classes should be defined with a unique prefix, i.e. "cli_tree" and "RNeXML_tree". The other lesson I learned about S4 is that packages should never setOldClass() on other packages' classes.

CC @gaborcsardi.

[cboettig]

yup, apologies, we've hit namespace collision issues before; though it is a pity there's not a better mechanism to handle them -- Fortunately I guess the majority of CRAN packages never adopted the S4 mechanism in the first place or namespace collisions in classes would be inevitable, no?

I would definitely do many things differently today but as you see this package has been around a long time and I'm a bit worried it's difficult to change these somewhat-user-facing elements like classes. @hlapp any intuition on whether this would break much code?

[hlapp]

We encountered this issue first 3.5 years ago, see #169. The issue at the core is documented in detail in #205. To quote:

It turns out that S4 classes defined with the same name but in different packages are actually supported in R. Hence, pseudo-namespacing class names as proposed in the #201 change set should in principle not be required. The mechanism for namespacing a class is in essence through the package attribute of the class name string. The function packageSlot() can be used to query and set the package attribute, and the function className() generates a class name namespaced in this way from its two arguments.

The report there does go on to explain why, even though in theory this should solve the problem of identically named S4 classes being used in different packages, in practice the implementation in the methods package falls short in a few (though important) places. This was 3 years ago.

Is the argument here that either these bugs in the methods package have nonetheless remained, or that there is a different issue now (presumably in the methods package?), or that the mitigation functions that were added to RNeXML at the time don't work anymore (or perhaps are now superfluous and if anything do harm?).

The #205 change set was in fact preceded by considering a prefixing solution, see #201. Changing class names in RNeXML is consequential because the class names correspond to elements in the XML format that the package reads and writes (NeXML). That's why the prefixing solution includes code that adds and strips the prefixes dynamically. Not pretty at all, and one of the main reasons we discarded it in favor of #205.

[krlmlr]

Thanks for the clarification. The cli package implements an S3 class "tree" and calls setOldClass() on it.

These particular symptoms will go away when cli renames its class to "cli_tree", for now no furter action seems to be necessary here. I'll also check if passing the S4class argument to setOldClass() in cli helps.

[cboettig]

Thanks to you both! It's great working with such excellent colleagues

[cboettig]

@krlmlr this is now showing up on CRAN checks, CRAN's given us the two-week warning:

Please see the problems shown on https://cran.r-project.org/web/checks/check_results_RNeXML.html.

Please correct before 2022-02-11 to safely retain your package on CRAN.

Did I understand correctly that this will resolve itself or is further action needed on our part? (Or am I mistaken and the CRAN check looks unrelated to this?)

[hlapp]

This looks like a difference between the release and the devel versions of R. @cboettig do you have the devel version installed so you can try to reproduce it?

[krlmlr]

I haven't seen those failures before, and I reverted a change in pillar to avoid breaking RNeXML. However, I haven't checked in R-devel.

Working on the cli update that should resolve this problem.

[cboettig]

@krlmlr Any news on the update to cli ? CRAN has given us to the 11th of Feb before we're archived...

@hlapp I think it's just the version of cli, not the version of R that is the real issue here; I think CRAN's release test machine was just slower to pick up the updated cli -- note that CRAN checks now show the WARN on most platforms (everything except for mac, which is probably running an older version. Yeah it's really annoying that CRAN does not AFIK report the versions of dependencies installed on each of these test machines, so it's really easy to assume it must be the R version or the architecture even though that's not the only difference between the test machines, it's just the only documented difference).

[krlmlr]

@gaborcsardi: Any chance we can get cli to CRAN during the next few days?

[gaborcsardi]

Possibly, but releasing cli is not so simple any more, with 500+ revdeps. The tree class is also ~ 4 years old and renaming it does break things.

FWIW I think this is a bug in base R, even if there are name clashes, RNeXML should be using its own classes, and the classes should not be mixed up in the class cache.

Monkey patching can fix this, i.e. if you remove the class in cli before you define your own. Since this happens at installation time it should have to effect on cli at runtime. You need to add this before setClass("tree", ...):

tryCatch({
  rlang::env_unlock(asNamespace("cli"))
  removeClass("tree", asNamespace("cli"))
  rlang::env_lock(asNamespace("cli"))
}, error = function(err) NULL)

The tryCatch() is to make sure that it works with the updated version of cli as well.

I'll write an email to R-devel.

[cboettig]

I've patched RNeXML using @gaborcsardi 's rlang trick. Our patch was accepted so v2.4.6 is on CRAN now.

Also it sounds like Gabor was successful at getting R maintainers to patch this behavior on R, though the email thread is thin on specifics of how this was patched.

Big thanks to @gaborcsardi and @krlmlr for all the help!