[Rd] S4 Generics and NAMESPACE : justified warning ?

[Martin Morgan]

Yohan Chalabi wrote:
> Dear list,
> 
> It seems that a package (pkgB) using another package (pkgA) with S4
> generics formed by taking existing functions (for example 'plot') must
> not import the existing functions ('plot') in its namespace to avoid
> the warning "replacing previous import: plot".
> 
> Suppose we use the simple 'import' directive in the name space of
> pkgB.
> 
> pkgA and pkgB files would be
> 
> pkgA/NAMESPACE:
>     import(graphics)
>     import(methods)
>     exportClasses("classA")
>     exportMethods("plot")
> 
> pkgA/R/pkgA.R:
>     setClass("classA", contains = "matrix")
>     setMethod("plot", "classA", function(x, y, ...) NULL)
> 
> pkgB/NAMESPACE:
>     import(methods)
>     import(garphics)
>     import(pkgA)
> 
> Loading pkgB would then generate a warning because the generic 'plot'
> in pkgA overwrites the 'plot' function imported from graphics name
> space :
> 
> Loading required package: pkgA
> Warning message:
> In namespaceImportFrom(self, asNamespace(ns)) :
>   replacing previous import: plot
> 
> As far as I understood it, one must explicitly import the functions one
> needs in pkgB with 'importFrom' to avoid the previous warning. In
> our example, one would then take care to not import the 'plot'
> function from garphics name space.
> 
> This makes the maintenance of packages using S4 packages rather
> tedious because one needs to import exactly what one needs rather than
> using the simple 'import' directive.
> 
> Moreover, the warning "replacing previous import:" is confusing. In this
> case we have a generic replacing its existing function like
> 'setMethod' would do and not a function replacing another function
> with the same name.

Hi Yohan --

Commenting as a user, there's no guarantee that the 'plot' generic
defined in pkgA is derived from graphics::plot via setGeneric; pkgA
could define it's own generic, and one would want to be informed of the
collision.

Maintenance of packages that have used simple 'import' to pull in all
dependencies is tedious, but using 'import' in some ways undermines
benefits of name spaces (restricting the symbol lookup table to reduce
the number of symbols and the possibility of name collisions, and to
more carefully isolate code inside the package name space to changes in
imported packages or induced by the user). So I think a 'better
practice' is to explicitly import just those functions, classes, etc
that are required by the package. Maintenance of such selective imports
is much less tedious, even with complicated package dependencies. There
is an unreleased Bioconductor package to identify specific imports,
available for R-2.9.* at

  svn export
https://hedgehog.fhcrc.org/bioconductor/branches/RELEASE_2_4/madman/Rpacks/codetoolsBioC

or for the development version of R at

  svn export
https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/codetoolsBioC

with functions findExternalDeps and writeNamespaceImports. These are
fairly thoroughly tested, but perhaps not fool-proof.

Martin

> 
> IMO it is normal that a function in a previous import can be replaced
> by its generic. It should not create a warning. But a warning should
> be generated when a generic is replaced by another generic.
> 
> Does this make sense ?
> 
> This could be implemented in 'namespaceImportFrom' with the patch given
> bellow.
> 
> It would greatly help the maintanance of packages which use S4
> packages, because one could use the simple 'import' directive rather
> than 'importFrom'.
> 
> Looking forward for your comments.
> 
> Regards,
> Yohan
> 
> 
> #################################################################################
> 
> Index: src/library/base/R/namespace.R
> ===================================================================
> --- src/library/base/R/namespace.R	(revision 49278)
> +++ src/library/base/R/namespace.R	(working copy)
> @@ -797,8 +797,12 @@
>  	}
>      }
>      for (n in impnames)
> -        if (exists(n, envir = impenv, inherits = FALSE))
> -            warning(msg, " ", n)
> +        if (exists(n, envir = impenv, inherits = FALSE)) {
> +            if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
> +                if (methods:::isGeneric(n, impenv))
> +                    warning("replacing previous generic import: ", n)
> +            } else warning(msg, " ", n)
> +        }
>      importIntoEnv(impenv, impnames, ns, impvars)
>      if (register) {
>          addImports(self, ns,
> 
> #################################################################################
> 
>