R devel - Jun 2023 - 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 <kasperdanielhansen at
gmail.com> wrote:
> 
> 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]]
> 
> ______________________________________________
> R-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>

Thanks all - yes, I think that Simon's diagnosis ("user error") is
correct:
in this situation one should define a reasonable generic function explicitly,
with a call to setGeneric, and not rely on the call inside of setMethod ...

But it is still not clear what the way forward should be (for package Matrix,
where we would like to export a method for 'qr.X').  If we do nothing,
then
there is the note, already mentioned:

     * checking R code for possible problems ... NOTE
     qr.X: no visible binding for global variable ?R?
     Undefined global functions or variables:
       R

If we add the following to our R/AllGenerics.R :

     setGeneric("qr.X",
                function(qr, complete = FALSE, ncol, ...)
                    standardGeneric("qr.X"),
                useAsDefault = function(qr, complete = FALSE, ncol, ...) {
                    if(missing(ncol))
                        base::qr.X(qr, complete = complete)
                    else base::qr.X(qr, complete = complete, ncol = ncol)
                },
                signature = "qr")

then we get a startup message, which would be quite disruptive on CRAN :

     The following object is masked from 'package:base':

         qr.X

and if we further add setGenericImplicit("qr.X", restore =
(TRUE|FALSE))
to our R/zzz.R, then for either value of 'restore' we encounter :

     ** testing if installed package can be loaded from temporary location
     Error: package or namespace load failed for 'Matrix':
      Function found when exporting methods from the namespace 'Matrix'
which is
not S4 generic: 'qr.X'

Are there possibilities that I have missed?

It seems to me that the best option might be to define an implicit generic
'qr.X' in methods via '.initImplicitGenerics' in
methods/R/makeBasicFunsList.R,
where I see that an implicit generic 'qr.R' is already defined ... ?

The patch pasted below "solves everything", though we'd still have
to think
about how to work for versions of R without the patch ...

Mikael

Index: src/library/methods/R/makeBasicFunsList.R
==================================================================---
src/library/methods/R/makeBasicFunsList.R	(revision 84541)
+++ src/library/methods/R/makeBasicFunsList.R	(working copy)
@@ -263,6 +263,17 @@
  	       signature = "qr", where = where)
      setGenericImplicit("qr.R", where, FALSE)

+    setGeneric("qr.X",
+               function(qr, complete = FALSE, ncol, ...)
+                   standardGeneric("qr.X"),
+               useAsDefault = function(qr, complete = FALSE, ncol, ...) {
+                   if(missing(ncol))
+                       base::qr.X(qr, complete = complete)
+                   else base::qr.X(qr, complete = complete, ncol = ncol)
+               },
+               signature = "qr", where = where)
+    setGenericImplicit("qr.X", where, FALSE)
+
      ## our toeplitz() only has 'x'; want the generic "here"
rather than "out
there"
      setGeneric("toeplitz", function(x, ...)
standardGeneric("toeplitz"),
  	       useAsDefault= function(x, ...) stats::toeplitz(x),

On 2023-06-13 8:01 pm, Simon Urbanek wrote:
> 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 <kasperdanielhansen
at gmail.com> wrote:
>>
>> 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]]
>>
>> ______________________________________________
>> R-devel at r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>