[Rd] codetools wrongly complains about lazy evaluation in S4 methods

[Ivan Krylov]

On Sat, 3 Jun 2023 11:50:59 -0400
Mikael Jagan <jaganmn2 using 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