Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[BUGZILLA #16662] exportClasses() directive silently ignores missing S4 classes #6049

Open
MichaelChirico opened this issue May 18, 2020 · 8 comments
Open

[BUGZILLA #16662] exportClasses() directive silently ignores missing S4 classes #6049

MichaelChirico opened this issue May 18, 2020 · 8 comments
Labels
Component - Installation Hardware - All All Importance - P5 minor Status - ASSIGNED Version - R 3.2.2

Comments

@MichaelChirico
Copy link
Owner

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

  • Bug author - Alex Bertram
  • Creation time - 2016-01-11 09:28:52 UTC
  • Bugzilla link
  • Status - UNCONFIRMED
  • Alias - None
  • Component - Installation
  • Version - R 3.2.2
  • Hardware - All All
  • Importance - P5 minor
  • Assignee - R-core
  • URL -
@MichaelChirico
Copy link
Owner Author

Created attachment 2611 [details]
Tiny package (source tarball) exemplifying this bug

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

  • checking whether package ‘exNSS4nil’ can be installed ... OK

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

  • checking whether package ‘exNSS4nil’ can be installed ... ERROR
    Installation failed.
    See ‘/opt/R/R-devel/tests/Pkgs/exNSS4nil.Rcheck/00install.out’ for details.

where we find:

** testing if installed package can be loaded from temporary location
Error: package or namespace load failed for ‘exNSS4nil’ in
loadNamespace(package, lib.loc):
in package ‘exNSS4nil’ classes THIS_CLASS_DOES_NOT_EXIST were specified for
export but not defined
Error: loading failed
Execution halted

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

  • Comment author - Sebastian Meyer
  • Timestamp - 2020-05-25 18:21:26 UTC

INCLUDED PATCH

@MichaelChirico
Copy link
Owner Author

Created attachment 2612 [details]
Warn if S4 export directives are used although the package does not define S4 things

Checking the toy package with this patch in place gives

  • checking whether package ‘exNSS4nil’ can be installed ... WARNING
    Found the following significant warnings:
    Warning: S4 exports specified in 'NAMESPACE' but not defined in package ‘exNSS4nil’
    See ‘/tmp/exNSS4nil.Rcheck/00install.out’ for details.

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

  • Comment author - Sebastian Meyer
  • Timestamp - 2020-05-25 18:39:01 UTC

INCLUDED PATCH

@MichaelChirico
Copy link
Owner Author

Thank you Sebastian, for your work on this.
Both your attachments (minimal repr.ex. package and the patch) look good to me,
and I plan to commit (a version of) your patch.

METADATA

  • Comment author - Martin Maechler
  • Timestamp - 2020-05-26 13:32:11 UTC

@MichaelChirico
Copy link
Owner Author

Created attachment 2613 [details]
exclude warnings from other packages

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

  • Comment author - Sebastian Meyer
  • Timestamp - 2020-05-27 09:01:44 UTC

INCLUDED PATCH

@MichaelChirico
Copy link
Owner Author

Hmm, the warning as produced when loading the namespace (+- your last patch),
is translated (well, not yet currently, because it is a new string not yet in any translation database) .. and now you want to grep for that. This works when people run in an English language locale, but not otherwise.

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),
and still grep outputs in the checking code ...

METADATA

  • Comment author - Martin Maechler
  • Timestamp - 2020-05-27 10:13:23 UTC

@MichaelChirico
Copy link
Owner Author

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
Warnung: replacing previous import ‘stats::lag’ by ‘dplyr::lag’ when loading ‘exNSS4nil’
Warnung: replacing previous import ‘stats::filter’ by ‘dplyr::filter’ when loading ‘exNSS4nil’

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

  • Comment author - Sebastian Meyer
  • Timestamp - 2020-05-27 12:41:55 UTC

@MichaelChirico
Copy link
Owner Author

(In reply to Sebastian Meyer from comment #6)

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 Warn**i**ng.
Maybe R CMD check somewhere enforces an English environment? This topic is
obviously beyond my current knowledge of R's internals.

Sorry, my bad, I misread that filtergrep code.

In a German locale, any "Warnung" lines get already filtered out much earlier in check_install()

lines <- grep(warn_re, lines, value = TRUE, useBytes = TRUE)

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.
My patch proposal for the check code makes R CMD check only warn about the package's own undefined S4 exports (in an English locale, otherwise, if "Warning" is translated, warnings are filtered out anyway).

METADATA

  • Comment author - Sebastian Meyer
  • Timestamp - 2020-05-28 09:11:30 UTC

@MichaelChirico
Copy link
Owner Author

We have a different fix ready, which will emit the messages only in the
loadNamespace() check, and only for the package being checked.

METADATA

  • Comment author - Kurt Hornik
  • Timestamp - 2020-05-28 09:15:48 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Component - Installation Hardware - All All Importance - P5 minor Status - ASSIGNED Version - R 3.2.2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@MichaelChirico