[Rd] RFC: Declaring "foo.bar" as nonS3method() ?!

[Duncan Murdoch]

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