Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
On Sat, 3 Jun 2023 11:50:59 -0400
Mikael Jagan 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
__
R-devel@r-project.org mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
On Sat, Jun 3, 2023 at 11:51 AM Mikael Jagan 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]] __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
I agree that this is not an R issue, but rather user error of not defining a proper generic so the check is right. Obviously, defining a generic with implementation-specific ncol default makes no sense at all, it should only be part of the method implementation. If one was to implement the same default behavior in the generic itself (not necessarily a good idea) the default would be ncol = if (complete) nrow(qr.R(qr, TRUE)) else min(dim(qr.R(qr, TRUE))) to not rely on the internals of the implementation. Cheers, Simon > On 14/06/2023, at 6:03 AM, Kasper Daniel Hansen > wrote: > > On Sat, Jun 3, 2023 at 11:51 AM Mikael Jagan 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]] > > __ > R-devel@r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel > __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel
