[Bioc-devel] Latest R-devel revision breaks flowCore and other flow packages

In the S4Vectors/IRanges infrastructure, we have never defined an
initialize method, so it is certainly possible to use the constructor
pattern in complex frameworks, and IRanges serves as a good example of
doing so. The basic pattern is to delegate to the superclass
constructor and pass that object as an unnamed argument to new().

We stayed away from changing the formals of initialize, because (a)
initialize() has a special contract of no-arg invocation that is a
pain to support and may not be consistent with the user-facing
interface and (b) more importantly, we wanted to preserve the ability
(at the low level) to re-initialize based purely on slots. Once you
have a custom initialize, it is no longer possible to call new()
consistently (just with slots) across all classes. And (c), since in
general we want constructors anyway (the user calling new() directly
would sacrifice abstraction), this policy greatly reduced complexity
by keeping the logic at a single layer. Others might have more to add;
I stopped thinking after I got to three ;)

Hope that helps,
Michael





On Mon, Jan 26, 2015 at 12:38 AM, Hahne, Florian
<florian.hahne at novartis.com> wrote:

Hi Michael,
I'll take a look. In order to improve my code: what exactly do you think
should be part of the initialiser, and what should be in the
constructor?
There don't seem to bee any clear guidelines out there anywhere. And if
all logic goes in the constructor, how does one deal with more complex
nested inheritance? And what's the use for the initialiser in the first
place?
Florian


On 24/01/15 03:03, "Michael Lawrence" <lawrence.michael at gene.com> wrote:

I have found and fixed the affected initialize cases and will begin
checking in fixes.

Affected packages: RDAVIDWebService, flowCore (as we know), Gviz,
ChIPseqR, Pviz, VanillaICE, flowFit.

As an aside, in all of these cases, initialize is implementing logic
that really belongs in a constructor function. Treating new() as a
high-level constructor should be discouraged in my opinion. Has
nothing to do with this bug though.

I have also begun looking through cases outside of initialize. There
are only about 300 cases of callNextMethod() in the codebase. Great
majority seem OK so far.


On Fri, Jan 23, 2015 at 12:46 PM, Michael Lawrence <michafla at gene.com>
wrote:

That's right.

On Fri, Jan 23, 2015 at 12:22 PM, Mike <wjiang2 at fhcrc.org> wrote:

Sorry, I just want to clarify some more on this.(maybe useful for
others as
well)
What you actually mean is callNextMethod() used to drop both ... and
the
other arguments explicitly supplied from the parent call (in my case,
parameters argument).
And now after your fix, both gets passed on and that?s why I should
explicitly select the argument for callNextMethod?

Thanks.

Mike

On 01/23/2015 11:30 AM, Michael Lawrence wrote:

The bug has existed forever. The commit log may be confusing. The
actual bug (or at least a very undesirable behavior) was in
match.call(), not callNextMethod().

I think it's still true that callNextMethod() is the natural
invocation. When adding arguments to initialize that you do not want
to pass on (and thus set as slots), it's necessary to use explicit
args. There are other cases where callNextMethod() is exactly what
you
want. Like the case in rtracklayer that motivated this fix.


On Fri, Jan 23, 2015 at 11:23 AM, Mike <wjiang2 at fhcrc.org> wrote:

Michael,

Thanks for the confirmation of the issue. I see you were trying to
tackle it
in the latest commits r67467:67472 which apparently haven?t fixed the
bug
yet (instead it triggers the error of the existing legacy code in
other R
packages like flowCore). I can certainly change the code to
explicitly
pass
on all the arguments to callNextMethod as you and Martin suggested. I
just
wonder if the documentation should drop the sentence of Calling with
no
arguments is often the natural way to use callNextMethod and change
the
example code (at least before the bug is eventually fixed.) so that
users
won?t be misusing it.



Mike

On 01/23/2015 10:55 AM, Martin Morgan wrote:

On 01/23/2015 10:52 AM, Michael Lawrence wrote:

First let me apologize for my failure to read emails. There was a
long-standing bug in the methods package where arguments passed as
"..." to a method would be dropped by callNextMethod(). It turns out
that in all known cases this affects calls to initialize(), probably
because there are many initialize() methods, and new() calls
initialize with "...".

This case is a very typical one, and Martin's fix is the right one,
according to the (unchanged) documentation of callNextMethod().

It's in general impossible to detect these cases from static
analysis,
since we do not know how the user is calling a method. But catching
them in initialize() should be easy, with some false positives. Just
look for callNextMethod().

Is it OK for me to checkout all of Bioconductor so that I can perform
that analysis, or will that stress the servers too much? I have a
package that embeds GNU Global to index and search
CRAN/Bioconductor-size repositories for these cases.


Hi Michael -- there is no problem checking out all
(https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks
presumably) of
Bioc.

Thanks! Martin


Michael

On Thu, Jan 22, 2015 at 6:15 AM, Martin Morgan
<mtmorgan at fredhutch.org>
wrote:

On 01/22/2015 12:26 AM, Martin Maechler wrote:


Mike  <wjiang2 at fhcrc.org>
      on Tue, 20 Jan 2015 17:16:37 -0800 writes:

      > I don't think it has been addressed yet in the later commits

of
R-devel.

      > And that piece of code in flowCore package was written long

time
ago and

      > there is nothing wrong with it as far as I can see.

You are right.

I thought Michael Lawrence (member of R Core since last summer!)
was also reading Bioc-devel, so wonder why he has not yet
replied --> CC'ing him

The changes to R-devel also did break the Matrix package in
exactly the same way.  You said

Here is the |initialize|method for |parameterFilter| which causes the
various errors from flow package vignettes.

|setMethod("initialize",
             signature=signature(.Object="parameterFilter"),
             definition=function(.Object, parameters,...)
             {
               if  (!missing(parameters))
                 parameters(.Object) <- parameters
               callNextMethod()
             })
|



and I also had  a  _no argument_ call
        callNextMethod()
inside an  initialize method.

I'm pretty sure that if you change (the same as I)

             callNextMethod()
to
             callNextMethod(.Object, ...)

you'll have a version of the code that works both in current R 3.1.2
(and older versions)  *and* in the R-devel version.


I also pinged Michael??

What's a precise statement of the problem -- no-argument
callNextMethod()
inside initialize? Any suggestions on ways to discover these without
relying
on package break during build / check / install?

Martin Morgan

Michael L and I were not sure how many other packages or R code this
would influence and he was expecting very very few.

Best regards,

Martin Maechler, ETH Zurich

      > On 01/20/2015 05:06 PM, Dan Tenenbaum wrote:

      >> I'm not sure if you were implying that this has already been

fixed
in R-devel. Note that the devel build machines currently have r67501
installed, which is later than all the commits you cite above, yet
the
flow
packages are still broken.