-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUGZILLA #16662] exportClasses() directive silently ignores missing S4 classes #6049
Comments
Created attachment 2611 [details] I confirm that it is possible for a package NAMESPACE to contain an exportClasses or exportMethods directive for a non-existent S4 class or method. The CRAN package smoothSurv (still) provides an example for this; a tiny toy package is attached (source tarball). Checking this package gives
However, the situation is much less severe than suggested by the report. As soon as the package actually defines any S4 classes or methods, i.e., methods:::.hasS4MetaData(ns) in line 635 of src/library/base/R/namespace.R 1 is TRUE, the package installation will indeed fail from non-existent classes or methods. For example, when uncommenting the setClass expression in the toy package's R file, we get
where we find:
AFAIK this bug does no harm, but I tend to think that such misuse of exportClasses should give a warning. A patch proposal follows. METADATA
INCLUDED PATCH
|
Created attachment 2612 [details] Checking the toy package with this patch in place gives
i.e., installation succeeds but warns about these undesired export directives. I'm not entirely sure, though, if this patch could result in false positives. Maybe the if-condition needs further tweaks. METADATA
INCLUDED PATCH
|
Thank you Sebastian, for your work on this. METADATA
|
Created attachment 2613 [details] I was curious if I can already find false positives in checks on CRAN. Thanks to tools::CRAN_check_details() it is very easy to scan the check results for the new warning. There are already true positives (e.g., psda has exportClasses(summary.plr)), but I also found that R CMD check will warn if an undefined S4 export is detected in any of the packages loaded by the currently checked package. Current examples of such "foreign" positives include packages BMTME (with warning from BGLR) and Brundle (with warning from the Bioconductor packgage DiffBind). I suggest to exclude the warning when the error is not in the checked package; a proposal for a patch is attached. METADATA
INCLUDED PATCH
|
Hmm, the warning as produced when loading the namespace (+- your last patch), Other experts also say that grepping for error or warning strings was more generally not a good idea (because of the translation issue; because also because of creating dependecies between "far away" parts of code, ...). The new better alternative would be to create a specific (sub)class of the error/warning and check for that... however this does not work here (I think), because the tools/R/check.R (R CMD check ..) is setup to work with text output exclusively, IIRC. A workaround here would be to not allow the warning message to be translatable (in the namespace code), METADATA
|
Interesting, I did not think about translation. I simply followed the approach already used in check.R. So this may be a more general issue unrelated to this particular bug. I always configure my R-devel with --disable-nls, so I did not test the new warning in this regard, but I can see that a comparable namespace warning is not translated in my R 3.6.3 installation: When I import both dplyr and stats in the toy NAMESPACE, installation gives ** byte-compile and prepare package for lazy loading So I get "Warnung" but the message remains English (although a German translation for that kind of message exists in src/library/base/po/R-de.po). I don't understand why. Furthermore, this warning is successfully excluded during R CMD check's check_install(), although it uses the English filtergrep("Warning: replacing previous import", ...), note the Warning. Maybe R CMD check somewhere enforces an English environment? This topic is obviously beyond my current knowledge of R's internals. METADATA
|
(In reply to Sebastian Meyer from comment #6)
Sorry, my bad, I misread that filtergrep code. In a German locale, any "Warnung" lines get already filtered out much earlier in check_install()
which only keeps the English warnings collected in "warn_re". Thus, in a German locale, I don't get the warnings, whereas in an English locale I do. The bottom line for me is that the check code currently needs an English locale to work properly, but this limitation is unrelated to the bug addressed here. METADATA
|
We have a different fix ready, which will emit the messages only in the METADATA
|
When running install_package(), undefined S4 classes declared in the NAMESPACE are silently ignored.
For example,
exportClasses(THIS_CLASS_DOES_NOT_EXIST)
is not treated as an error.
There is at least one package in CRAN affected:
https://github.com/cran/smoothSurv/blob/7e4a7c30ff856bf4028924f82345139cf2924b55/NAMESPACE#L23
This package does not use S4 at all, so it clearly an error.
To be consistent with export(), exportClasses() should throw an error upon encountering an invalid class.
METADATA
The text was updated successfully, but these errors were encountered: