>>>>> 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 <maech...@stat.math.ethz.ch>
> To: R-devel <r-devel@r-project.org>, Henrik Bengtsson
> <henrik.bengts...@gmail.com>
> Cc: Martin Maechler <maech...@stat.math.ethz.ch>
> Subject: Re: [Rd] methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true
> error
> Message-ID: <23831.52673.201433.320...@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 <maech...@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@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
