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.

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

Future-proof against breaking change on R-devel #103

Closed
wleoncio opened this issue Feb 24, 2024 · 6 comments
Closed

Future-proof against breaking change on R-devel #103

wleoncio opened this issue Feb 24, 2024 · 6 comments
Assignees
Labels
bug Something isn't working critical This issue should be prioritized

Comments

@wleoncio
Copy link
Member

This was received on 2024-02-23 as a message forwarded by Luke Tierney from the R-devel mailing list:

This note was sent to R-devel last October. Your TruncExpFam nodbi is affected by this change and will start failing CRAN checks under R-devel shortly.

Best,

luke
---------- Forwarded message ----------
Date: Fri, 20 Oct 2023 19:56:20 +0000
From: luke-tierney@uiowa.edu
To: R-devel@r-project.org
Subject: UseMethod forwarding of local variables

UseMethod has since the beginning had the 'feature' that local
variables in the generic are added to the environment in which the
method body is evaluated. This is documented in ?UseMethod and
R-lang.texi, but use of this 'feature' has been explicitly discouraged
in R-lang.texi for many years.

This is an unfortunate design decision for a number of reasons (see
below), so the plan is to remove this 'feature' in the next major
release.

Fortunately only a small number of packages on CRAN (see below) seem
to make use of this feature directly; a few more as reverse
dependencies. The maintainers of the directly affected packages will
be notified separately.

Current R-devel allows you to set the environment variable
R_USEMETHOD_FORWARD_LOCALS=none to run R without this feature or
R_USEMETHOD_FORWARD_LOCALS=error to signal an error when a forwarded
variable's value is used.

Some more details:

An example:

 > foo <- function(x) { yyy <- 77; UseMethod("foo") }
 > foo.bar <- function(x) yyy
 > foo(structure(1, class = "bar"))
 [1] 77

Some reasons the design is a bad idea:

  • You can't determine what a method does without knowing what the
    generic it will be called from looks like.

  • Code analysis (codetools, the compiler) can't analyze method
    code reliably.

  • You can't debug a method on its own. For the foo() example,

    foo.bar(structure(1, class = "bar"))
    Error in foo.bar(structure(1, class = "bar")) : object 'yyy' not found

  • A method relying on these variables won't work when reached via NextMethod:

    foo.baz <- function(x) NextMethod("foo")
    foo(structure(2, class = c("baz", "bar")))
    Error in foo.bar(structure(2, class = c("baz", "bar"))) :
    object 'yyy' not found

The directly affected CRAN packages I have identified are:

  • actuar
  • quanteda
  • optmatch
  • rlang
  • saeRobust
  • Sim.DiffProc
  • sugrrants
  • texmex

Some of these fail with the environment set to 'error' but not to
'none', so they are getting a value from somewhere else that may or
may not be right.

Affected as revdeps of optmatch:

  • cobalt
  • htetree
  • jointVIP
  • MatchIt
  • PCAmatchR
  • rcbalance
  • rcbsubset
  • RItools
  • stratamatch

Affected as revdeps of texmex:

  • lax
  • mobirep

I'll study and try to reproduce the issue―so far our R-devel checks are fine, but as Luke said they are bound to fail soon―and release a fix ASAP.

@wleoncio wleoncio added bug Something isn't working critical This issue should be prioritized labels Feb 24, 2024
@wleoncio wleoncio self-assigned this Feb 24, 2024
@wleoncio
Copy link
Member Author

Hard deadline on 2024-03-10, as the package is failing on CRAN:

Dear maintainer,

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

The ERRORs for r-devel are from

r85979 | luke | 2024-02-24 01:18:27 +0100 (Sat, 24 Feb 2024) | 4 lines
Move towards UseMethod no longer forwarding local variables from the generic.
For now they are forwarded with promises that signal an error i forced.

Please correct before 2024-03-10 to safely retain your package on CRAN.

Best,
-k

@wleoncio
Copy link
Member Author

Example of useful error message:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-direct-sampling.R:7:3'): Original attributes are retrieved ─────
Error in `mget(ls())`: getting UseMethod variable 'parms' from generic 'rtrunc_direct'; this is no longer supported
Backtrace:
    ▆
 1. └─TruncExpFam::rtrunc(1e+06, mean = 1, sd = 2, faster = TRUE) at test-direct-sampling.R:7:3
 2.   ├─TruncExpFam::rtrunc_direct(n, family, ...)
 3.   └─TruncExpFam:::rtrunc_direct.normal(n, family, ...)
 4.     ├─TruncExpFam:::truncated_q(...)
 5.     │ └─base::paste0("trunc_", class(parms[["n"]]))
 6.     └─base::mget(ls())

See more here. Problem seems to be contained to gcc, as R-devel on clang doesn't fail. Regardless, must address within the fortnight.

@wleoncio
Copy link
Member Author

To reproduce:

rhub::check_with_rdevel(env_vars = c("R_USEMETHOD_FORWARD_LOCALS" = "error"))

wleoncio added a commit that referenced this issue Feb 26, 2024
wleoncio added a commit that referenced this issue Feb 26, 2024
@wleoncio
Copy link
Member Author

CI tests failing on develop but passing on release, so something probably went wrong in merging the hotfix branch.

@wleoncio
Copy link
Member Author

Keep an eye on the CRAN check results for 1.1.1 before really closing this. Fix on develop is independent of all this.

@wleoncio
Copy link
Member Author

All good, closing:

bilde

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working critical This issue should be prioritized
Projects
None yet
Development

No branches or pull requests

1 participant