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

Hervé Pagès hp@ge@@on@g|thub @end|ng |rom gm@||@com
Thu Jun 15 23:25:17 CEST 2023 

Oh but I see now that you've already tried this in your R/AllGenerics.R, 
sorry for missing that, but that you worry about the following message 
being disruptive on CRAN:

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

         qr.X

Why would that be? As long as you only define methods for objects that 
**you** control everything is fine. In other words you're not allowed to 
define a method for "qr" objects because that method would override 
base::qr.X(). But the generic itself and the method that you define for 
your objects don't override anything so should not disrupt anything.

H.

On 6/15/23 13:51, Hervé Pagès wrote:
>
> I'd argue that at the root of the problem is that your qr.X() generic 
> dispatches on all its arguments, including the 'ncol' argument which I 
> think the dispatch mechanism needs to evaluate **before** dispatch can 
> actually happen.
>
> So yes lazy evaluation is a real feature but it does not play well for 
> arguments of a generic that are involved in the dispatch.
>
> If you explicitly defined your generic with:
>
>    setGeneric("qr.X", signature="qr")
>
> you should be fine.
>
> More generally speaking, it's a good idea to restrict the signature of 
> a generic to the arguments "that make sense". For unary operations 
> this is usually the 1st argument, for binary operations the first two 
> arguments etc... Additional arguments that control the operation like 
> modiflers, toggles, flags, rng seed, and other parameters, usually 
> have not place in the signature of the generic.
>
> H.
>
> On 6/14/23 20:57, Mikael Jagan wrote:
>> 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 using gmail.com> wrote:
>>>>
>>>> On Sat, Jun 3, 2023 at 11:51 AM Mikael Jagan <jaganmn2 using 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 using r-project.org mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>
>>>
>>
>> ______________________________________________
>> R-devel using r-project.org mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
> -- 
> Hervé Pagès
>
> Bioconductor Core Team
> hpages.on.github using gmail.com

-- 
Hervé Pagès

Bioconductor Core Team
hpages.on.github using gmail.com

	[[alternative HTML version deleted]]

More information about the R-devel mailing list