[Rd] RFC: Declaring "foo.bar" as nonS3method() ?!
John Fox
jfox at mcmaster.ca
Sat Jun 13 14:52:14 CEST 2015
Dear Martin et al.,
Thank you for considering so thoroughly the issue that I raised.
Best,
John
On Sat, 13 Jun 2015 13:35:41 +0200
Martin Maechler <maechler at stat.math.ethz.ch> wrote:
> >>>>> Luke Tierney <luke-tierney at uiowa.edu>
> >>>>> on Fri, 12 Jun 2015 10:30:29 -0500 writes:
>
> > The notes available off the devloper page
> > https://developer.r-project.org/ describe some of the rationale for
> > the S3 method search design. One thing that has changed since then is
> > that all packages now have name spaces. We could change the search
> > algorithm to skip attached package exports (and package imports and
> > base), which would require methods defined in packages that are to be
> > accessible outside the package to be declared. Methods defined inside
> > a package for internal use or methods defined in scripts not in
> > packages would still be found. Packages not currently registering
> > their methods would have to do so -- not sure how many that would
> > affect. Testing on CRAN/Bioc should show how much of an effect this
> > would have and whether there are any other issues.
>
> > Best,
> > luke
>
> Thanks a lot Luke, for the extra perspective.
> I think the four R core commenters here (Duncan, Kurt, Luke and
> me) agree that this is not trivial to implement, but hopefully
> not too hard either, and I think we also +- agree that it seems
> desirable to try adding a bit more flexibility in how functions
> are "made into" S3 methods.
>
> I had not envisaged to change the S3 method search
> algorithm but rather to tweak part of it "database" but am aware
> that I don't know enough of the details.
> Also, I did not find which notes (from developer.r-project.org)
> you were refering to.
>
> Given the broad agreement that we want to start working /
> investigating this, we can well close the thread here for now
> (and deal with ideas, issues, details within R-core).
>
> Martin
>
> > On Fri, 12 Jun 2015, Duncan Murdoch wrote:
>
> >> On 12/06/2015 10:53 AM, Hadley Wickham wrote:
> >>> To me, it seems like there's actually two problems here:
> >>>
> >>> 1) Preventing all() from dispatching to all.effects() for objects of
> >>> class effects
> >>> 2) Eliminating the NOTE in R CMD check
> >>>
> >>> My impression is that 1) actually causes few problems, particularly
> >>> since people are mostly now aware of the problem and avoid using `.`
> >>> in function names unless they're S3 methods. Fixing this issue seems
> >>> like it would be a lot of work for relatively little gain.
> >>>
> >>> However, I think we want to prevent people from writing new functions
> >>> with this confusing naming scheme, but equally we want to grandfather
> >>> in existing functions, because renaming them all would be a lot of
> >>> work (I'm looking at you t.test()!).
> >>>
> >>> Could we have a system similar to globalVariables() where you could
> >>> flag a function as definitely not being an S3 method? I'm not sure
> >>> what R CMD check should do - ideally you wouldn't be allow to use
> >>> method.class for new functions, but still be able suppress the note
> >>> for old functions that can't easily be changed.
> >>
> >> We have a mechanism for suppressing the warning for existing functions,
> >> it's just not available to users to modify. So it would be possible to
> >> add effects::all.effects to the stop list, and this might be the easiest
> >> action here.
> >>
> >> This isn't perfect because all.effects() would still act as a method.
> >> However, it does give the deprecated message if you ever call it, so
> >> nobody would do this unknowingly. The only real risk is that if anyone
> >> ever wrote an all.effects function that *was* supposed to be an S3
> >> method, it might be masked by the one in effects.
> >>
> >> Duncan Murdoch
> >>
> >>>
> >>> Hadley
> >>>
> >>> On Fri, Jun 12, 2015 at 6:52 AM, Kurt Hornik <Kurt.Hornik at wu.ac.at> wrote:
> >>>>>>>>> Duncan Murdoch writes:
> >>>>
> >>>>> On 12/06/2015 7:16 AM, Kurt Hornik wrote:
> >>>>>>>>>>> Duncan Murdoch writes:
> >>>>>>
> >>>>>>> On 12/06/2015 4:12 AM, Martin Maechler wrote:
> >>>>>>>> This is a topic ' "apparent S3 methods" note in R CMD check '
> >>>>>>>> from R-package-devel
> >>>>>>>> https://stat.ethz.ch/pipermail/r-package-devel/2015q2/000126.html
> >>>>>>>>
> >>>>>>>> which is relevant to here because some of us have been thinking
> >>>>>>>> about extending R because of the issue.
> >>>>>>>>
> >>>>>>>> John Fox, maintainer of the 'effects' package has enquired about
> >>>>>>>> the following output from 'R CMD check effects'
> >>>>>>>>
> >>>>>>>>> * checking S3 generic/method consistency ... NOTE
> >>>>>>>>> Found the following apparent S3 methods exported but not registered:
> >>>>>>>>> all.effects
> >>>>>>>>
> >>>>>>>> and added
> >>>>>>>>
> >>>>>>>>> The offending function, all.effects(), is deprecated in favour of
> >>>>>>>>> allEffects(), but I'd rather not get rid of it for backwards compatibility.
> >>>>>>>>> Is there any way to suppress the note without removing all.effects()?
> >>>>>>>>
> >>>>>>>> and I had agreed that this was a "False Positive" in this case.
> >>>>>>>>
> >>>>>>>> [.......]
> >>>>>>>>
> >>>>>>>> and then
> >>>>>>>>
> >>>>>>>>> Now I agree .. and have e-talked about this with another R core
> >>>>>>>>> member .. that it would be desirable for the package author to
> >>>>>>>>> effectively declare the fact that such a function is not an S3
> >>>>>>>>> method even though it "looks like it" at least if looked from far.
> >>>>>>>>
> >>>>>>>>> So, ideally, you could have something like
> >>>>>>>>
> >>>>>>>>> nonS3method("all.effects")
> >>>>>>>>
> >>>>>>>>> somewhere in your package source ( in NAMESPACE or R/*.R )
> >>>>>>>>> which would tell the package-checking code -- but *ALSO* all the other S3
> >>>>>>>>> method code that all.effects should be treated as a regular R
> >>>>>>>>> function.
> >>>>>>>>
> >>>>>>>>> I would very much like such a feature in R, and for that reason,
> >>>>>>>>> I'm cross posting this (as one of the famous exceptions that
> >>>>>>>>> accompany real-life rules!!) to R-devel.
> >>>>>>>>
> >>>>>>>> and actually I did *not* cross post, but have now moved the
> >>>>>>>> relevant part of the thread to R-devel.
> >>>>>>>>
> >>>>>>
> >>>>>>> It sounds like a good idea. It's a nontrivial amount of work, because
> >>>>>>> of the "all the other S3 method code" part. There's the question of
> >>>>>>> functions defined outside of packages: presumably they are still S3
> >>>>>>> methods, with no way to suppress that.
> >>>>>>
> >>>>> I am not sure this is the right solution: S3 dispatch will still occur
> >>>>> because we first look at foo.bar exports and then in the S3 registry,
> >>>>> afaicr (the "all the other S3 method code" part).
> >>>>>>
> >>>>> If we could move to only looking at the registry for dispatch, there
> >>>>> would be no need to declare situations where we should not dispatch on
> >>>>> foo.bar exports.
> >>>>>>
> >>>>
> >>>>> I think that would break a lot of existing scripts. I think the logic
> >>>>> should be something like this.
> >>>>
> >>>>> For each class in the class list:
> >>>>
> >>>>> Search backwards through the environment chain. If the current location
> >>>>> is a package environment or namespace, look only in the registry. If
> >>>>> not, look at all functions.
> >>>>
> >>>>> If that search failed, try the next class.
> >>>>
> >>>> Yep---that's what I meant. I forgot to write the "if namespace
> >>>> semantics applies" part :-)
> >>>>
> >>>> Best
> >>>> -k
> >>>>
> >>>>> A variation on the test is: If there's a registry in the current
> >>>>> location, look there. But I think the registry is not on the search
> >>>>> list, so I'm not sure that would work.
> >>>>
> >>>>> This assumes that we keep separate registries in each package; if we
> >>>>> merge them into one big registry, it gets harder. I'm not familiar
> >>>>> enough with the code to know which way we do it.
> >>>>
> >>>>> Duncan Murdoch
> >>>>
> >>>> ______________________________________________
> >>>> R-devel at r-project.org mailing list
> >>>> https://stat.ethz.ch/mailman/listinfo/r-devel
> >>>
> >>>
> >>>
> >>
> >> ______________________________________________
> >> R-devel at r-project.org mailing list
> >> https://stat.ethz.ch/mailman/listinfo/r-devel
> >>
>
> > --
> > Luke Tierney
> > Ralph E. Wareham Professor of Mathematical Sciences
> > University of Iowa Phone: 319-335-3386
> > Department of Statistics and Fax: 319-335-3017
> > Actuarial Science
> > 241 Schaeffer Hall email: luke-tierney at uiowa.edu
> > Iowa City, IA 52242 WWW: http://www.stat.uiowa.edu
More information about the R-devel
mailing list