Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
> Martin Maechler > on Fri, 16 Jun 2023 11:41:12 +0200 writes: > Mikael Jagan > on Thu, 15 Jun 2023 22:00:45 -0400 writes: >> On 2023-06-15 5:25 pm, Hervé Pagès wrote: >>> Oh but I see now that you've already tried this in your >>> R/AllGenerics.R, sorry for missing that, > yes, this one: > 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 the 'signature = "qr"' is perfect (and as Hervé suggests). > > but the complication in useAsDefault = .. is really a bit ugly > ... notably if one compares with the nice original base :: qr.X () In spite of the "ugliness" (above) and of the "conflict rules"-resolution I've talked about below, I've now committed the above + implicit generic definition to R-devel's {methods} package. After all, it provides a clean situation also for other S4-using developers. I'm still musing if this is a bug fix to be back ported to the R-4-3-branch eventually. Best, Martin > >> 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. > > > Yes, maybe it would be fine in principle, and of course > > many popular packages emit startup messages. Still, in > > practice, I think that people are quite accustomed to > > library(Matrix) being "silent", and probably a nontrivial > > fraction of our reverse dependencies would encounter new > > NOTEs about differences between *.Rout and *.Rout.save, > > etc. > > I tend to agree with Hervé that the "masked" warning here is > a false positive. > > I also agree with Mikael that we would not want this for every > default use of library(Matrix) > > I believe we should fix it by using conflictRules(), and from > reading ?conflictRules I understand we could do that by setting > > options(conflict.policy = list(mask.ok = "qr.X")) > > possibly even in Matrix package's load or attach hook > [[and if really really necessary even as hack inside library()'s checks]] > > > The fraction of users who will ever call this method for > > qr.X is very very small compared to the fraction who will > > be confused or annoyed by the message. Hence my hope that > > an implicit generic qr.X could become part of package > > methods, notably as an implicit generic qr.R already lives > > there ... > > > Or maybe there is a way for Matrix to define qr.X as an > > implicit generic without creating other problems, but my > > experiments with setGenericImplicit were not promising ... > > In principle, I'd say that setGenericImplicit() would be a good > / "the correct" approach, but as you already tried > unsuccessfully, I'm not claiming the principle. > > Martin > > > > Mikael > > >> H. __ 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
> Mikael Jagan > on Thu, 15 Jun 2023 22:00:45 -0400 writes: > On 2023-06-15 5:25 pm, Hervé Pagès wrote: >> Oh but I see now that you've already tried this in your >> R/AllGenerics.R, sorry for missing that, yes, this one: 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 the 'signature = "qr"' is perfect (and as Hervé suggests). but the complication in useAsDefault = .. is really a bit ugly ... notably if one compares with the nice original base :: qr.X () >> 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. > Yes, maybe it would be fine in principle, and of course > many popular packages emit startup messages. Still, in > practice, I think that people are quite accustomed to > library(Matrix) being "silent", and probably a nontrivial > fraction of our reverse dependencies would encounter new > NOTEs about differences between *.Rout and *.Rout.save, > etc. I tend to agree with Hervé that the "masked" warning here is a false positive. I also agree with Mikael that we would not want this for every default use of library(Matrix) I believe we should fix it by using conflictRules(), and from reading ?conflictRules I understand we could do that by setting options(conflict.policy = list(mask.ok = "qr.X")) possibly even in Matrix package's load or attach hook [[and if really really necessary even as hack inside library()'s checks]] > The fraction of users who will ever call this method for > qr.X is very very small compared to the fraction who will > be confused or annoyed by the message. Hence my hope that > an implicit generic qr.X could become part of package > methods, notably as an implicit generic qr.R already lives > there ... > Or maybe there is a way for Matrix to define qr.X as an > implicit generic without creating other problems, but my > experiments with setGenericImplicit were not promising ... In principle, I'd say that setGenericImplicit() would be a good / "the correct" approach, but as you already tried unsuccessfully, I'm not claiming the principle. Martin > Mikael >> 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
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
On 2023-06-15 5:25 pm, Hervé Pagès wrote: 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. Yes, maybe it would be fine in principle, and of course many popular packages emit startup messages. Still, in practice, I think that people are quite accustomed to library(Matrix) being "silent", and probably a nontrivial fraction of our reverse dependencies would encounter new NOTEs about differences between *.Rout and *.Rout.save, etc. The fraction of users who will ever call this method for qr.X is very very small compared to the fraction who will be confused or annoyed by the message. Hence my hope that an implicit generic qr.X could become part of package methods, notably as an implicit generic qr.R already lives there ... Or maybe there is a way for Matrix to define qr.X as an implicit generic without creating other problems, but my experiments with setGenericImplicit were not promising ... Mikael 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, ...) { +
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
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
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
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 >>> 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 nam
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
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 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
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
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
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
Most of the errors, warnings and notes generated by R CMD check are generated by code in the tools package, usually in the tools/R/QC.R source file. Search that file for the error message, then backtrack to find the code that causes it to be triggered. If I recall correctly, it works on the evaluated source rather than the actual source, so it will only see the result of evaluating `setMethod` in your example. I don't know the methods package well enough to know exactly what that does, but presumably it produces a function and hides it somewhere so that the S4 dispatch can find it when it needs to. Duncan Murdoch On 12/06/2023 2:03 p.m., Mikael Jagan wrote: Thanks both. Yes, I was aware of globalVariables, etc. I guess I was hoping to be pointed to the right place in the source code, in case the issue could be addressed properly, notably as it seems to have already been addressed for functions that are not S4 methods, i.e., codetools is apparently not bothered by def <- function(x = y) { y <- 0; x } but still complains about setMethod("someGeneric", "someClass", def) ... Mikael On 2023-06-07 5:13 am, Gabriel Becker wrote: The API supported workaround is to call globalVariables, which, essentially, declares the variables without defining them (a distinction R does not usually make). The issue with this approach, of course, is that its a very blunt instrument. It will cause false negatives if you accidentally use the same symbol in a standard evaluation context elsewhere in your code. Nonetheless, that's the intended approach as far as i know. Best, ~G On Wed, Jun 7, 2023 at 1:07 AM Serguei Sokol via R-devel < r-devel@r-project.org> wrote: 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@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 __ 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
Re: [Rd] codetools wrongly complains about lazy evaluation in S4 methods
Thanks both. Yes, I was aware of globalVariables, etc. I guess I was hoping to be pointed to the right place in the source code, in case the issue could be addressed properly, notably as it seems to have already been addressed for functions that are not S4 methods, i.e., codetools is apparently not bothered by def <- function(x = y) { y <- 0; x } but still complains about setMethod("someGeneric", "someClass", def) ... Mikael On 2023-06-07 5:13 am, Gabriel Becker wrote: The API supported workaround is to call globalVariables, which, essentially, declares the variables without defining them (a distinction R does not usually make). The issue with this approach, of course, is that its a very blunt instrument. It will cause false negatives if you accidentally use the same symbol in a standard evaluation context elsewhere in your code. Nonetheless, that's the intended approach as far as i know. Best, ~G On Wed, Jun 7, 2023 at 1:07 AM Serguei Sokol via R-devel < r-devel@r-project.org> wrote: 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@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 __ 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
The API supported workaround is to call globalVariables, which, essentially, declares the variables without defining them (a distinction R does not usually make). The issue with this approach, of course, is that its a very blunt instrument. It will cause false negatives if you accidentally use the same symbol in a standard evaluation context elsewhere in your code. Nonetheless, that's the intended approach as far as i know. Best, ~G On Wed, Jun 7, 2023 at 1:07 AM Serguei Sokol via R-devel < r-devel@r-project.org> wrote: > 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@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 > [[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
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@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
[Rd] 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. Mikaelnm <- "TestPackage" dir.create(tmp <- file.path(tempdir(), nm)) dir.create(file.path(tmp, "R")) cat(file = file.path(tmp, "DESCRIPTION"), " Package: TestPackage Version: 0.0-0 License: GPL (>= 2) Description: A (one paragraph) description of what the package does and why it may be useful. Title: My First Collection of Functions Author: First Last [aut, cre] Maintainer: First Last Imports: methods ") cat(file = file.path(tmp, "NAMESPACE"), " importFrom(methods, setMethod, setOldClass) exportMethods(qr.X) export(xyz) ") cat(file = file.path(tmp, "R", paste0(nm, ".R")), " setOldClass(\"qr\") setMethod(\"qr.X\", signature(qr = \"qr\"), function(qr, complete, ncol) NULL) xyz <- function(x, y = z) { z <- 1; x + y } ") install.packages(tmp, repos = NULL) tools:::.check_code_usage_in_package(nm) ## 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 unloadNamespace(nm) remove.packages(nm) unlink(tmp) __ R-devel@r-project.org mailing list https://stat.ethz.ch/mailman/listinfo/r-devel