[Rd] "False" warning on "replacing previous import" when re-exporting identical object
Henrik Bengtsson
hb at biostat.ucsf.edu
Sat Sep 21 20:06:09 CEST 2013
On Sat, Sep 21, 2013 at 9:44 AM, Ben Bolker <bbolker at gmail.com> wrote:
> Henrik Bengtsson <hb <at> biostat.ucsf.edu> writes:
>
>>
>> On Fri, Aug 30, 2013 at 11:51 AM, Henrik Bengtsson
>> <hb <at> biostat.ucsf.edu> wrote:
>> > Hi.
>
> [snip]
>
> Bump ... Henrik, did you ever post this as a request/wishlist at
> https://bugs.r-project.org/bugzilla3/ ? (I don't think so, I couldn't
> find it there, but maybe I wasn't looking in the right place.) I
I submitted PR#15451:
'Bug 15451 - PATCH: namespaceImportFrom() not to warn when the
identical object is imported twice' on 2013-09-10
https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=15451
There was a brief response from Prof. Ripley and then it was closed.
However, I followed up asking [cc:ing BDR and R-core] for
clarification and I gave some further arguments. There has been no
response to date.
> would be curious to know what the response was from R-Core, because
> these kinds of warnings are starting to pop up in the nlme/lme4/
> downstream package complex. In particular,
>
> VarCorr
> fixef
> lmList
> ranef
>
> are defined by nlme, imported and re-exported by lme4. This doesn't
> seem to make any trouble for lme4, but it does cause problems for
> some of the downstream packages (such as GRRGI:
> [broken URL for Gmane]
> http://www.r-project.org/nosvn/R.check/
> r-devel-linux-x86_64-fedora-gcc/GRRGI-00check.html )
>
> (Right now GRRGI uses a pretty crude import-all strategy:
> import(nlme,lme4,...); exportPattern("."); which might be
> tweaked, but I think the issue is still worth resolving.)
My interpretation of all this is that either re-exports or import():s
has to/should go. Trying to predict the future of 'R CMD check' and
re-exports are likely to cause issues as it now stands, I'd say, move
towards dropping import() and start using importFrom(). It's a
tedious move but it can be done. My issue with packages using
importFrom() is that they will be less agile for changes in APIs, e.g.
one function is moved from on package to another (of the same
"family").
>
> In a perfect world, I guess some of these generics would be moved
> upstream to a package that both nlme and lme4 could import from,
> but that seems tricky to do without making fairly major architectural
> changes.
Yes, that's a very ad hoc workaround (and not part of my perfect world
because then we get back to the time of name/argument clashes) and I
don't think you want to go there.
/Henrik
>
> Ben Bolker
On Thu, Sep 12, 2013 at 9:46 AM, Henrik Bengtsson <hb at biostat.ucsf.edu> wrote:
> Just for the record, I did submit PR#15451:
>
> 'Bug 15451 - PATCH: namespaceImportFrom() not to warn when the
> identical object is imported twice' on 2013-09-10
> https://bugs.r-project.org/bugzilla3/show_bug.cgi?id=15451
>
> /Henrik
>
> On Mon, Sep 9, 2013 at 2:37 PM, Henrik Bengtsson <hb at biostat.ucsf.edu> wrote:
>> Any intelligent comments on this before I submit a proposal/patch via
>> bugs.r-project.org?
>>
>> /Henrik
>>
>> On Fri, Aug 30, 2013 at 12:47 PM, Henrik Bengtsson <hb at biostat.ucsf.edu> wrote:
>>> On Fri, Aug 30, 2013 at 11:51 AM, Henrik Bengtsson <hb at biostat.ucsf.edu> wrote:
>>>> Hi.
>>>>
>>>> On Fri, Aug 30, 2013 at 6:58 AM, Paul Gilbert <pgilbert902 at gmail.com> wrote:
>>>>> This is related to the recent thread on correct NAMESPACE approach when writing S3 methods. If your methods are S4 I think pkgB does not need to export the generic. Just export the method and everything works magically and your problem disappears. For S3 methods there seems to be the difficultly you describe. Of course, the difference between S3 and S4 on this appears somewhat bug like. (I have not tested all this very carefully so I may have something wrong.)
>>>>
>>>> For the record, you're referring to R-devel thread 'Correct NAMESPACE
>>>> approach when writing an S3 method for a generic in another package'
>>>> started on Aug 24, 2013
>>>> [https://stat.ethz.ch/pipermail/r-devel/2013-August/067221.html].
>>>> Yes, it's closely related or possibly the same issue. However, I do
>>>> not find it odd that the S3 generic function needs to be exported
>>>> from/available via the namespace. Hence it needs to be export():ed by
>>>> at least one package/namespace.
>>>>
>>>> The real issue is when two package needs to export a generic function
>>>> with the same name, e.g. foo(). If the two generic functions are
>>>> defined differently (e.g. different arguments/signatures), they will
>>>> be in conflict with each other. If they are identical, this should
>>>> not be a problem, but here I might be wrong. However, there is also
>>>> the special case where one package reexports the generic function from
>>>> another package, e.g. PkgB::foo() exports PkgA:foo(). In this case,
>>>> the object 'foo' does actually not existing in the name space of
>>>> package PkgB - instead it provides a "redirect" to it, e.g.
>>>>
>>>>> PkgA::foo
>>>> function (...)
>>>> UseMethod("foo")
>>>> <environment: namespace:PkgA>
>>>>
>>>>> PkgB::foo
>>>> function (...)
>>>> UseMethod("foo")
>>>> <environment: namespace:PkgA>
>>>>
>>>>> exists("foo", envir=getNamespace("PkgB"), inherits=FALSE)
>>>> [1] FALSE
>>>>
>>>>> exists("foo", envir=getNamespace("PkgB"), inherits=TRUE)
>>>> [1] TRUE
>>>>
>>>>> identical(PkgB::foo, PkgA::foo)
>>>> [1] TRUE
>>>>
>>>>
>>>> The warning on "replacing previous import by 'PkgA::foo' when loading
>>>> 'PkgC'" that occurs due to import(PkgA, PkgB) is generated in
>>>> base::namespaceImportFrom()
>>>> [http://svn.r-project.org/R/trunk/src/library/base/R/namespace.R],
>>>> simply because it detects that "foo" (=n) has already been imported to
>>>> PkgC' namespace (=impenv):
>>>>
>>>> if (exists(n, envir = impenv, inherits = FALSE)) {
>>>> if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
>>>> ## warn only if generic overwrites a function which
>>>> ## it was not derived from
>>>> ...
>>>> }
>>>> warning(sprintf(msg, sQuote(paste(nsname, n, sep = "::")),
>>>> sQuote(from)), call. = FALSE, domain = NA)
>>>> }
>>>>
>>>> Note how there is already code for avoiding "false" warnings on S4
>>>> generic function. This is what we'd like to have also for S3 generic
>>>> functions, but it's much harder to test for such since they're not
>>>> formally defined. However, I'd argue that it is safe to skip the
>>>> warning *when the variable to be imported does not actually exist in
>>>> the package being imported* (e.g. when just rexported), i.e.
>>>>
>>>>>svn diff namespace.R
>>>> Index: namespace.R
>>>> ===================================================================
>>>> --- namespace.R (revision 63776)
>>>> +++ namespace.R (working copy)
>>>> @@ -871,6 +871,10 @@
>>>> }
>>>> for (n in impnames)
>>>> if (exists(n, envir = impenv, inherits = FALSE)) {
>>>> + ## warn only if imported variable actually exists in the
>>>> + ## namespace imported from, which is not the case if
>>>> + ## the variable is rexported from another namespace
>>>> + if (!exists(n, envir = ns, inherits = FALSE)) next
>>>> if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
>>>> ## warn only if generic overwrites a function which
>>>> ## it was not derived from
>>>
>>> Ok, if import(PkgA, PkgB) and PkgB reexports a *different* foo() than
>>> PkgA::foo(), say PkgZ::foo so identical(PkgB::foo, PkgA::foo) is
>>> FALSE, then there is indeed a conflict. An alternative patch:
>>>
>>>>svn diff namespace.R
>>> Index: namespace.R
>>> ===================================================================
>>> --- namespace.R (revision 63776)
>>> +++ namespace.R (working copy)
>>> @@ -871,6 +871,11 @@
>>> }
>>> for (n in impnames)
>>> if (exists(n, envir = impenv, inherits = FALSE)) {
>>> + ## warn only if imported variable is non-identical to
>>> + ## the one already imported
>>> + getImp <- get(n, envir = impenv)
>>> + obj <- get(n, envir = ns)
>>> + if (identical(obj, getImp)) next
>>> if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
>>> ## warn only if generic overwrites a function which
>>> ## it was not derived from
>>>
>>> /Henrik
>>>
>>>>
>>>> I'm planning to propose ("wishlist / enhancement"; it may even be a
>>>> bug) this over at https://bugs.r-project.org/.
>>>>
>>>> Comments, anyone?
>>>>
>>>> /Henrik
>>>>
>>>>
>>>>> Paul
>>>>>
>>>>> Henrik Bengtsson <hb at biostat.ucsf.edu> wrote:
>>>>>
>>>>>>Hi,
>>>>>>
>>>>>>SETUP:
>>>>>>Consider three packages PkgA, PkgB and PkgC.
>>>>>>
>>>>>>PkgA defines a generic function foo() and exports it;
>>>>>>
>>>>>>export(foo)
>>>>>>
>>>>>>PkgB imports PkgA::foo() and re-exports it;
>>>>>>
>>>>>>importFrom(PkgA, foo)
>>>>>>export(foo)
>>>>>>
>>>>>>PkgC imports everything from PkgA and PkgB:
>>>>>>
>>>>>>imports(PkgA, PkgB)
>>>>>>
>>>>>>
>>>>>>PROBLEM:
>>>>>>Loading or attaching the namespace of PkgC will generate a warning:
>>>>>>
>>>>>> replacing previous import by 'PkgA::foo' when loading 'PkgC'
>>>>>>
>>>>>>This in turn causes 'R CMD check' on PkgC to generate a WARNING (no-go at CRAN):
>>>>>>
>>>>>>* checking whether package 'PkgC' can be installed ... WARNING
>>>>>>Found the following significant warnings:
>>>>>> Warning: replacing previous import by 'PkgA::foo' when loading
>>>>>>'CellularAutomaton'
>>>>>>
>>>>>>
>>>>>>FALSE?
>>>>>>Isn't it valid to argue that this is a "false" warning, because
>>>>>>identical(PkgB::foo, PkgA::foo) is TRUE and therefore has no effect?
>>>>>>
>>>>>>
>>>>>>/Henrik
>>>>>>
>>>>>>PS. The above can be avoided by using explicit importFrom() on PkgA
>>>>>>and PkgB, but that's really tedious. In my case this is out of my
>>>>>>reach, because I'm the author of PkgA and PkgB but not many of the
>>>>>>PkgC packages.
>>>>>>
>>>>>>______________________________________________
>>>>>>R-devel at r-project.org mailing list
>>>>>>https://stat.ethz.ch/mailman/listinfo/r-devel
More information about the R-devel
mailing list