R devel - Aug 2009 - S4 Generics and NAMESPACE : justified warning ?

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.

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,

#################################################################################


-- 
PhD student
Swiss Federal Institute of Technology
Zurich

www.ethz.ch

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

>>>> "MM" == Martin Morgan <mtmorgan at fhcrc.org>
>>>> on Tue, 18 Aug 2009 06:15:50 -0700
Hi Martin,

Thanks for your response.

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

This shouldn't be a problem because generics keep track of the function
and package used.

setGeneric(plot)
str(getGeneric(plot))

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


I agree with you that 'importFrom' should be the preferred approach. I
have been using it for some time and I have even wrote my own
functions to automatically generate the NAMESPACE. 


But the drawback is that it makes the dependency with other packages
version specific and can become tricky when one has several such
dependencies.

To make the maintenance of packages easier, I would like to use
the suboptimal 'import' so that I do not need to care about this
NAMESPACE/package version issue. But I cannot do that because of an,
IMO, unjustified warning.

regards,
Yohan

-- 
PhD student
Swiss Federal Institute of Technology
Zurich

www.ethz.ch