R devel - Jul 2019 - methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error

In 'conformMethod', there is another instance of
omittedSig & <something about signature[omittedSig]> .
It just affects error message.

Original:
? ? if(any(is.na(match(signature[omittedSig], c("ANY",
"missing"))))) {
? ? ? ? bad <- omittedSig & is.na(match(signature[omittedSig],
c("ANY", "missing")))

After r76756:
? ? if(any(iiN <- is.na(match(signature[omittedSig], c("ANY",
"missing"))))) {
? ? ? ? bad <- omittedSig & iiN


----------------------------------------------
>>>>> 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

>>>>> Suharto Anggono Suharto Anggono via R-devel 
>>>>>     on Thu, 4 Jul 2019 15:13:55 +0000 writes:
    > In 'conformMethod', there is another instance of
    > omittedSig & <something about signature[omittedSig]> .
    > It just affects error message.

    > Original:
    > ? ? if(any(is.na(match(signature[omittedSig], c("ANY",
"missing"))))) {
    > ? ? ? ? bad <- omittedSig & is.na(match(signature[omittedSig],
c("ANY", "missing")))

    > After r76756:
    > ? ? if(any(iiN <- is.na(match(signature[omittedSig],
c("ANY", "missing"))))) {
    > ? ? ? ? bad <- omittedSig & iiN

Yes, thank you, that's same "internal logic" error as later.

One difference is that I think the above case cannot be
triggered by setMethod() .. one needs an explicit "artificial"
conformMethod() call.
Still, of course, it's wrong and should be corrected.
I've started to test changes .. maye not get to commit (with
enough confidence) before taking the train to
useR! 2019 @  Toulouse (France).

Martin