[Rd] methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true
Martin Maechler
m@ech|er @end|ng |rom @t@t@m@th@ethz@ch
Mon Jul 1 14:11:16 CEST 2019
>>>>> Georgi Boshnakov
>>>>> on Mon, 1 Jul 2019 09:11:39 +0000 writes:
> I run checks with R-devel rev. 76756 on several of my
> packages which use S4, with no ill effects.
Thank you, Georgi!
> With the risk of not adding much, it seems that Martin's example, which
> I repeat below, suggests a crossover between several
> "signatures" involved here - the signature of the generic,
> the signature of the method (the argument for setMethod()),
> and the signature of the function object defining the method.
that's correct.
and the conformMethod() plus rematchDefinition() utility functions
have been created to deal with this problem "optimally" and consistently.
>>> r2 <- oligoFn(object=diag(2), target=array(42))
>> Error in .local(object, target) :
>> argument "target" is missing, with no default
>>> getMethod("oligoFn", signature(object="matrix", subset="missing", target="array"))
>>
>> Method Definition:
>
>> function (object, subset, target, value)
>> {
>> .local <- function (object, subset, target, value)
>> list(object = object, target = target)
>> .local(object, target)
>> }
> a) .local() seems superfluous here, since the signature of
> the function object is identical to that of the generic.
that's only true in the *result*ing method,
but not in how the method was defined (the following code was in
my post) :
setMethod("oligoFn", signature(object = "matrix", target="array"), ## Method _ 2 _
function(object, target) list(object=object, target=target))
Here, function object has arguments (object, target) only
> b) "subset" is "missing" in the signature of the method
> but it is (legally, I believe) present in the function
> object. The call .local(object, target) would be
> appropriate if "subset" was missing also from the
> signature of the function object (the latter would be
> illegal here). Alternatively, it could be named, as in
> .local(object, target = target) or maybe even
> .local(object, , target).
> --
> Georgi Boshnakov
The 2nd patch I committed -- the one mostly to
rematchDefiniton() -- does exactly that: It uses .local()
calls where those argument *are* named which should be named.
==> hence the above example works fine there.
Martin
> ------------------------------
> Date: Sat, 29 Jun 2019 22:44:49 +0200
> From: Martin Maechler <maechler using stat.math.ethz.ch>
> To: R-devel <r-devel using r-project.org>, Henrik Bengtsson
> <henrik.bengtsson using gmail.com>
> Cc: Martin Maechler <maechler using stat.math.ethz.ch>
> Subject: Re: [Rd] methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true
> error
> Message-ID: <23831.52673.201433.320952 using stat.math.ethz.ch>
> Content-Type: text/plain; charset="utf-8"
>
> >>>>> Martin Maechler
> >>>>> on Sat, 29 Jun 2019 12:05:49 +0200 writes:
>
> >>>>> Martin Maechler
> >>>>> on Sat, 29 Jun 2019 10:33:10 +0200 writes:
>
> >>>>> peter dalgaard
> >>>>> on Fri, 28 Jun 2019 16:20:03 +0200 writes:
>
>>>> > On 28 Jun 2019, at 16:03 , Martin Maechler <maechler using stat.math.ethz.ch> wrote:
>>>> >
>>>> >>>>>> Henrik Bengtsson
>>>> >>>>>> on Thu, 27 Jun 2019 16:00:39 -0700 writes:
>>>> >
>>>> >> Using:
>>>> >>
>>>> >> untrace(methods::conformMethod)
>>>> >> at <- c(12,4,3,2)
>>>> >> str(body(methods::conformMethod)[[at]])
>>>> >> ## language omittedSig <- omittedSig && (signature[omittedSig] != "missing")
>>>> >> cc <- 0L
>>>> >> trace(methods::conformMethod, tracer = quote({
>>>> >> cc <<- cc + 1L
>>>> >> print(cc)
>>>> >> if (cc == 31) { ## manually identified
>>>> >> untrace(methods::conformMethod)
>>>> >> trace(methods::conformMethod, at = list(at), tracer = quote({
>>>> >> str(list(signature = signature, mnames = mnames, fnames = fnames))
>>>> >> print(ls())
>>>> >> try(str(list(omittedSig = omittedSig, signature = signature)))
>>>> >> }))
>>>> >> }
>>>> >> }))
>>>> >> loadNamespace("oligo")
>>>> >>
>>>> >> gives:
>>>> >>
>>>> >> Untracing function "conformMethod" in package "methods"
>>>> >> Tracing function "conformMethod" in package "methods"
>>>> >> Tracing conformMethod(signature, mnames, fnames, f, fdef, definition)
>>>> >> step 12,4,3,2
>>>> >> List of 3
>>>> >> $ signature: Named chr [1:4] "TilingFeatureSet" "ANY" "ANY" "array"
>>>> >> ..- attr(*, "names")= chr [1:4] "object" "subset" "target" "value"
>>>> >> ..- attr(*, "package")= chr [1:4] "oligoClasses" "methods" "methods" "methods"
>>>> >> $ mnames : chr [1:2] "object" "value"
>>>> >> $ fnames : chr [1:4] "object" "subset" "target" "value"
>>>> >> [1] "f" "fdef" "fnames" "fsig" "imf"
>>>> >> [6] "method" "mnames" "omitted" "omittedSig" "sig0"
>>>> >> [11] "sigNames" "signature"
>>>> >> List of 2
>>>> >> $ omittedSig: logi [1:4] FALSE TRUE TRUE FALSE
>>>> >> $ signature : Named chr [1:4] "TilingFeatureSet" "ANY" "ANY" "array"
>>>> >> ..- attr(*, "names")= chr [1:4] "object" "subset" "target" "value"
>>>> >> ..- attr(*, "package")= chr [1:4] "oligoClasses" "methods" "methods" "methods"
>>>> >> Error in omittedSig && (signature[omittedSig] != "missing") :
>>>> >> 'length(x) = 4 > 1' in coercion to 'logical(1)'
>>>> >> Error: unable to load R code in package 'oligo'
>>>> >>
>>>> >
>>>> > Thank you, Henrik, nice piece of using trace() .. and the above
>>>> > is useful for solving the issue -- I can work with that.
>>>> >
>>>> > I'm already pretty sure the wrong code starts with
>>>> >
>>>> > omittedSig <- sigNames %in% fnames[omitted] # ....
>
> >> my "pretty sure" statement above has proven to be wrong ..
>
>>>> > -------------
>>>> >
>>>>
>>>> I think the intention must have been that the two "ANY" signatures should change to "missing".
>
> >> Definitely.
>
>>>> However, with the current logic that will not happen, because
>>>>
>>>> > c(F,T,T,F) && c(T,T)
>>>> [1] FALSE
>>>>
>>>> Henrik's non-fix would have resulted in
>>>>
>>>> > c(F,T,T,F) & c(T,T)
>>>> [1] FALSE TRUE TRUE FALSE
>>>>
>>>> which is actually right, but only coincidentally due to recycling of c(T,T). Had it been c(F,T) then it would have been expanded to c(F,T,F,T) which would be the opposite of what was wanted.
>>>>
>>>> Barring NA issues, I still think
>>>>
>>>> omittedSig[omittedSig] <- (signature[omittedSig] != "missing")
>>>>
>>>> should do the trick.
>
> >> yes, (most probably). I've found a version of that which should
> >> be even easier to "read and understand", in svn commit 76753 :
>
> >> svn diff -c 76753 src/library/methods/R/RMethodUtils.R
>
> >> --- src/library/methods/R/RMethodUtils.R (Revision 76752)
> >> +++ src/library/methods/R/RMethodUtils.R (Revision 76753)
> >> @@ -342,8 +342,7 @@
> >> gettextf("formal arguments (%s) omitted in the method definition cannot be in the signature", bad2),
> >> call. = TRUE, domain = NA)
> >> }
> >> - else if(!all(signature[omittedSig] == "missing")) {
> >> - omittedSig <- omittedSig && (signature[omittedSig] != "missing")
> >> + else if(any(omittedSig <- omittedSig & signature != "missing")) {
>
>
> >> BTW: I've marked this --- and the runmed() seg.fault + na.action
> >> change --- as something to be added to R 3.6.1 patched, as I
> >> deemed I should obey the "code freeze" rule in both cases.
>
> >> Martin
>
> > Hmm... I think we got a 'Neverending Story' here -- because it
> > seems, both Peter and I were wrong in thinking that it's a good
> > idea to change "missing" to "ANY" here ...
> > ((or if that *was* correct, that needs to entail more changes
> > happening during setMethod(.) {conformMethod() is only called in
> > one place in our code base, namely from setMethod()} :
>
> as a matter of fact, I've been brave for now, left the change to
> conformMethod() and started to fix the constructed .local()
> calls which are created in conformMethod()'s sibling,
> which is rematchDefinition().
>
> It seems that this builds e.g. Matrix {with its gazillion
> setMethod()s} and that continues to run its own tests. OTOH,
> Matrix may not trigger the situations that are dealt with here
> at all, as the signature() are rarely longer than three, and at
> some point in time I had made long passes through the package in
> order to "minimize" the .local() calls.
>
> --> svn commit 76756 (in addition to 76753, mentioned earlier)
> now has rematchDefinition() changes
>
> I would be positively surprised if (but can imagine that) this
> had no affect on CRAN / Bioconductor packages.
>
> Still, these two changes seem to achieve what both the comments
> and the documentation of conformMethod() and rematchDefinition()
> suggest should happen.
>
> Of course, I'd really be happy if people with S4 packages would
> check them with an R-devel version with svn rev >= 76756
> and report problems they see.
> I do imagine effects, and would expect that bugs in current code
> become visible where they had not done so previously.
>
> Martin
>
> > I've started to explore the effects of the change using and
> > extending the tests/reg-tests-1d.R example that I had just committed.
>
> > And the result is *not good* :
>
> > As we said above, the new code does replace all "ANY" by "missing" in the
> > signature, unless the "ANY" are at the end of the signature.
>
> > However, later methods package code producing the .local(.)
> > calls in the method definition (in cases where the signature of
> > the generic and the method do not match exactly) --- possibly
> > may have been tweaked later than the conformMethod() code ---
> > and the .local() calls they now produce
>
> > - work as intended for R <= 3.6.0 (and R 3.6.1 RC)
> > - fail to work for R-devel with the change :
>
> > See this (not a minimal rep.rex. but one building on Henrik's
> > oligo case and what's newly in tests/reg-tests-1d.R ) :
>
> > -----------------------------------------------------------------------------
>
> > ## conformMethod() "&& logic" bug, by Henrik Bengtsson on R-devel list, 2019-06-22
> > setClass("tilingFSet", slots = c(x = "numeric"))
> > if(!is.null(getGeneric("oligoFn"))) removeGeneric("oligoFn")
> > setGeneric("oligoFn",
> > function(object, subset, target, value) { standardGeneric("oligoFn") })
> > Sys.unsetenv("_R_CHECK_LENGTH_1_LOGIC2_") ## << added here, to compare with R 3.6.0, 3.5.3, ..
> > setMethod("oligoFn", signature(object = "tilingFSet", value="array"), ## Method _ 1 _
> > function(object, value) { list(object=object, value=value) })
> > setMethod("oligoFn", signature(object = "matrix", target="array"), ## Method _ 2 _
> > function(object, target) list(object=object, target=target))
> > setMethod("oligoFn", signature(object = "matrix", subset="integer"), ## Method _ 3 _
> > function(object, subset) list(object=object, subset=subset)) # (*no* Note: ANY at end)
> > setMethod("oligoFn", signature(object = "matrix"), ## Method _ 4 _
> > function(object) list(object=object)) # (*no* Note: ANY at end)
> > showMethods("oligoFn") # F.Y.I.: in R 3.6.0 and earlier: contains "ANY" everywhere
>
> > ##-- Now, the following *DOES* work fine in R <= 3.6.0 but *no longer* in R-devel :
> > str(r1 <- oligoFn(object=new("tilingFSet"), value=array(2)))
> > str(r2 <- oligoFn(object=diag(2), target=array(42)))
> > ## These 2 work fine in all versions of R: Here the "ANY" remain at the end:
> > str(r3 <- oligoFn(object=diag(2), subset=1:3))
> > str(r4 <- oligoFn(object=diag(2)))
>
> > -----------------------------------------------------------------------------
> > The two errors in R-devel are actually quite user-confusing:
>
> >> r2 <- oligoFn(object=diag(2), target=array(42))
> > Error in .local(object, target) :
> > argument "target" is missing, with no default
> >> getMethod("oligoFn", signature(object="matrix", subset="missing", target="array"))
> > Method Definition:
>
> > function (object, subset, target, value)
> > {
> > .local <- function (object, subset, target, value)
> > list(object = object, target = target)
> > .local(object, target)
> > }
>
> > Signatures:
> > object subset target value
> > target "matrix" "missing" "array" "ANY"
> > defined "matrix" "missing" "array" "ANY"
> >>
>
>
> > My conclusion: There's something really wrong with what
> > conformMethod() has been *intended* to achieve and what it
> > "accidentally" did achieve in these cases: it never replaced
> > "ANY" by "missing" in all these cases *AND* that is what it
> > seems it should continue doing.
>
> > BTW: ?conformMethod goes to a page with quite a few things,
> > relevantly containing
>
> > ‘conformMethod’: If the formal arguments, ‘mnames’, are not
> > identical to the formal arguments to the function, ‘fnames’,
> > ‘conformMethod’ determines whether the signature and the two
> > sets of arguments conform, and returns the signature,
> > possibly extended. The function name, ‘f’ is supplied for
> > error messages. The generic function, ‘fdef’, supplies the
> > generic signature for matching purposes.
>
> > The method assignment conforms if method and generic function
> > have identical formal argument lists. It can also conform if
> > the method omits some of the formal arguments of the function
> > but: (1) the non-omitted arguments are a subset of the
> > function arguments, appearing in the same order; (2) there
> > are no arguments to the method that are not arguments to the
> > function; and (3) the omitted formal arguments do not appear
> > as explicit classes in the signature. A future extension
> > hopes to test also that the omitted arguments are not assumed
> > by being used as locally assigned names or function names in
> > the body of the method.
>
> > ---
> > It seems my commit to R-devel needs another change.
> > This has to wait for hours currently, though.
>
> > Martin
>
> > ______________________________________________
> > R-devel using r-project.org mailing list
> > https://stat.ethz.ch/mailman/listinfo/r-devel
More information about the R-devel
mailing list