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

> On 28 Jun 2019, at 16:03 , Martin Maechler <maechler at
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] # ....
> 
> -------------
> 
I think the intention must have been that the two "ANY" signatures
should change to "missing". 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.

-pd
> 
> *Still*  I cannot understand why in my case (and probably Peter,
> as he also said he can't reproduce),
> the  conformMethod() function is not even called  when I run
>     loadNamespace("oligo").
> 
> As conformMethod() is *only* called from setMethod(), I've
> started trace()ing  setMethod() and indeed, it is *only* called one,
> and not with oligo methods/generics,... 
> 
> Henrik, do you per chance not install packages in the usual way,
> i.e., do you install them without saving all the pre-computed
> classes and methods tables etc,
> and that would be *why* these setMethod() etc are only called at
> this point in time ?
> 
> Martin
-- 
Peter Dalgaard, Professor,
Center for Statistics, Copenhagen Business School
Solbjerg Plads 3, 2000 Frederiksberg, Denmark
Phone: (+45)38153501
Office: A 4.23
Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com

>>>>> peter dalgaard 
>>>>>     on Fri, 28 Jun 2019 16:20:03 +0200 writes:
> > On 28 Jun 2019, at 16:03 , Martin Maechler <maechler at
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

> > *Still*  I cannot understand why in my case (and probably Peter,
> > as he also said he can't reproduce),
> > the  conformMethod() function is not even called  when I run
> >     loadNamespace("oligo").
> > 
> > As conformMethod() is *only* called from setMethod(), I've
> > started trace()ing  setMethod() and indeed, it is *only* called one,
> > and not with oligo methods/generics,... 
> > 
> > Henrik, do you per chance not install packages in the usual way,
> > i.e., do you install them without saving all the pre-computed
> > classes and methods tables etc,
> > and that would be *why* these setMethod() etc are only called at
> > this point in time ?
> > 
> > Martin
> 
> -- 
> Peter Dalgaard, Professor,
> Center for Statistics, Copenhagen Business School
> Solbjerg Plads 3, 2000 Frederiksberg, Denmark
> Phone: (+45)38153501
> Office: A 4.23
> Email: pd.mes at cbs.dk  Priv: PDalgd at gmail.com
> 
> 
> 
> 
>

>>>>> 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 at
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()} :

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