Mikael Jagan
2023-Jun-03 15:50 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
In a package, I define a method for not-yet-generic function 'qr.X' like so: > setOldClass("qr") > setMethod("qr.X", signature(qr = "qr"), function(qr, complete, ncol) NULL) The formals of the newly generic 'qr.X' are inherited from the non-generic function in the base namespace. Notably, the inherited default value of formal argument 'ncol' relies on lazy evaluation: > formals(qr.X)[["ncol"]] if (complete) nrow(R) else min(dim(R)) where 'R' must be defined in the body of any method that might evaluate 'ncol'. To my surprise, tools:::.check_code_usage_in_package() complains about the undefined symbol: qr.X: no visible binding for global variable 'R' qr.X,qr: no visible binding for global variable 'R' Undefined global functions or variables: R I claim that it should _not_ complain, given that lazy evaluation is a really a feature of the language _and_ given that it already does not complain about the formals of functions that are not S4 methods. Having said that, it is not obvious to me what in codetools would need to change here. Any ideas? I've attached a script that creates and installs a test package and reproduces the check output by calling tools:::.check_code_usage_in_package(). Hope it gets through. Mikael -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: TestPackage.txt URL: <https://stat.ethz.ch/pipermail/r-devel/attachments/20230603/20664345/attachment.txt>
Serguei Sokol
2023-Jun-07 08:06 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
Le 03/06/2023 ? 17:50, Mikael Jagan a ?crit?:> In a package, I define a method for not-yet-generic function 'qr.X' > like so: > > ??? > setOldClass("qr") > ??? > setMethod("qr.X", signature(qr = "qr"), function(qr, complete, > ncol) NULL) > > The formals of the newly generic 'qr.X' are inherited from the > non-generic > function in the base namespace.? Notably, the inherited default value of > formal argument 'ncol' relies on lazy evaluation: > > ??? > formals(qr.X)[["ncol"]] > ??? if (complete) nrow(R) else min(dim(R)) > > where 'R' must be defined in the body of any method that might > evaluate 'ncol'. > To my surprise, tools:::.check_code_usage_in_package() complains about > the > undefined symbol: > > ??? qr.X: no visible binding for global variable 'R' > ??? qr.X,qr: no visible binding for global variable 'R' > ??? Undefined global functions or variables: > ????? RI think this issue is similar to the complaints about non defined variables in expressions involving non standard evaluation, e.g. column names in a data frame which are used as unquoted symbols. One of workarounds is simply to declare them somewhere in your code. In your case, it could be something as simple as: ? R=NULL Best, Serguei.> > I claim that it should _not_ complain, given that lazy evaluation is a > really > a feature of the language _and_ given that it already does not > complain about > the formals of functions that are not S4 methods. > > Having said that, it is not obvious to me what in codetools would need > to change > here.? Any ideas? > > I've attached a script that creates and installs a test package and > reproduces > the check output by calling tools:::.check_code_usage_in_package().? > Hope it > gets through. > > Mikael > > ______________________________________________ > R-devel at r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel
Ivan Krylov
2023-Jun-13 08:23 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
On Sat, 3 Jun 2023 11:50:59 -0400 Mikael Jagan <jaganmn2 at gmail.com> wrote:> > setOldClass("qr") > > setMethod("qr.X", signature(qr = "qr"), function(qr, complete, > > ncol) NULL) > > The formals of the newly generic 'qr.X' are inherited from the > non-generic function in the base namespace. Notably, the inherited > default value of formal argument 'ncol' relies on lazy evaluation: > > > formals(qr.X)[["ncol"]] > if (complete) nrow(R) else min(dim(R)) > > where 'R' must be defined in the body of any method that might > evaluate 'ncol'. To my surprise, > tools:::.check_code_usage_in_package() complains about the undefined > symbol: > > qr.X: no visible binding for global variable 'R' > qr.X,qr: no visible binding for global variable 'R' > Undefined global functions or variables: > RIn other words, codetools::checkUsage(base::qr.X) says nothing while codetools::checkUsage(TestPackage::qr.X) complains. I think the difference is that codetools::findFuncLocals sees an assignment to `R` in the body of base::qr.X: codetools::findFuncLocals(formals(base::qr.X), body(base::qr.X)) # [1] "cmplx" "cn" "ip" "p" "pivoted" "R" "res" # [8] "tmp" The problem, then, is that an S4 generic shouldn't be having such assignments in its body. One way to fix this would be to modify codetools::checkUsage to immediately return if inherits(fun, 'standardGeneric'), but I don't know enough about S4 to say whether this is safe. (A more comprehensive fix would be to check every encountered method against the formals of the generic, but that sounds complicated.) Arguably, static analysis will always be wrong about something, so we're trading a false positive for potential false negatives. -- Best regards, Ivan
Kasper Daniel Hansen
2023-Jun-13 18:03 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
On Sat, Jun 3, 2023 at 11:51?AM Mikael Jagan <jaganmn2 at gmail.com> wrote:> The formals of the newly generic 'qr.X' are inherited from the non-generic > function in the base namespace. Notably, the inherited default value of > formal argument 'ncol' relies on lazy evaluation: > > > formals(qr.X)[["ncol"]] > if (complete) nrow(R) else min(dim(R)) > > where 'R' must be defined in the body of any method that might evaluate > 'ncol'. >Perhaps I am misunderstanding something, but I think Mikael's expectations about the scoping rules of R are wrong. The enclosing environment of ncol is where it was _defined_ not where it is _called_ (apologies if I am messing up the computer science terminology here). This suggests to me that codetools is right. But a more extended example would be useful. Perhaps there is something special with setOldClass() which I am no aware of. Also, Bioconductor has 100s of packages with S4 where codetools works well. Kasper [[alternative HTML version deleted]]