R devel - Jun 2023 - 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>

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:
> ????? R
I 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

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:
>        R
In 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

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]]