R devel - Dec 2017 - possible bug in utils::removeSource - NULL argument is silently dropped

Dear R-Core Team,

I found an unexpected behaviour in utils::removeSource (also present in 
r-devel as of today).

---

# create a function which accepts NULL argument
foo <- function(x, y) {
   if (is.null(y)) y <- "default foo"
   attr(x, "foo") <- y
   x
}

# create a function which utilizes 'foo'
testSrc <- function() {
   x <- 1:3
   x <- foo(x, NULL)
   x
}

# this works fine
testSrc()

# this fails
testNoSrc <- utils::removeSource(testSrc)
testNoSrc()

# removeSource removes NULL from the 'foo' call
print(testNoSrc)

---

I traced back the bug to this row in removeSource:

(line 33 in sourceutils.R)
part[[i]] <- recurse(part[[i]])

it should be (IMHO):
part[i] <- list(recurse(part[[i]]))

---

# create a function with the above patch
rmSource <- function (fn) {
   stopifnot(is.function(fn))
   if (is.primitive(fn))
     return(fn)
   attr(fn, "srcref") <- NULL
   attr(body(fn), "wholeSrcref") <- NULL
   attr(body(fn), "srcfile") <- NULL
   recurse <- function(part) {
     if (is.name(part))
       return(part)
     attr(part, "srcref") <- NULL
     attr(part, "wholeSrcref") <- NULL
     attr(part, "srcfile") <- NULL
     if (is.language(part) && is.recursive(part)) {
       for (i in seq_along(part))
         part[i] <- list(recurse(part[[i]]))
     }
     part
   }
   body(fn) <- recurse(body(fn))
   fn
}

# test
( testNoSrc2 <- rmSource(testSrc) )
testNoSrc2()


Regards,
Denes

>>>>> D?nes T?th <toth.denes at kogentum.hu>
>>>>>     on Mon, 11 Dec 2017 11:39:38 +0100 writes:
    > Dear R-Core Team,

    > I found an unexpected behaviour in utils::removeSource (also present in
    > r-devel as of today).
> ---
> 
> # create a function which accepts NULL argument
> foo <- function(x, y) {
>    if (is.null(y)) y <- "default foo"
>    attr(x, "foo") <- y
>    x
> }
> 
> # create a function which utilizes 'foo'
> testSrc <- function() {
>    x <- 1:3
>    x <- foo(x, NULL)
>    x
> }
> 
> # this works fine
> testSrc()
> 
> # this fails
> testNoSrc <- utils::removeSource(testSrc)
> testNoSrc()
> 
> # removeSource removes NULL from the 'foo' call
> print(testNoSrc)
> 
> ---
> 
> I traced back the bug to this row in removeSource:
> 
> (line 33 in sourceutils.R)
> part[[i]] <- recurse(part[[i]])
> 
> it should be (IMHO):
> part[i] <- list(recurse(part[[i]]))
> 
> ---
> 
> # create a function with the above patch
> rmSource <- function (fn) {
>    stopifnot(is.function(fn))
>    if (is.primitive(fn))
>      return(fn)
>    attr(fn, "srcref") <- NULL
>    attr(body(fn), "wholeSrcref") <- NULL
>    attr(body(fn), "srcfile") <- NULL
>    recurse <- function(part) {
>      if (is.name(part))
>        return(part)
>      attr(part, "srcref") <- NULL
>      attr(part, "wholeSrcref") <- NULL
>      attr(part, "srcfile") <- NULL
>      if (is.language(part) && is.recursive(part)) {
>        for (i in seq_along(part))
>          part[i] <- list(recurse(part[[i]]))
>      }
>      part
>    }
>    body(fn) <- recurse(body(fn))
>    fn
> }
> 
> # test
> ( testNoSrc2 <- rmSource(testSrc) )
> testNoSrc2()
> 
> 
> Regards,
> Denes
Thank you very much, Denes. This is indeed a bug 

As it happens I had wanted to improve the removeSource()
function many months ago [to make it work so it removes source attributes also
from non-function expressions] and have had a changed version of
it in my version of R-devel.

Your proposed change looks quite reasonable, and I'll try adding
it (and look at the consequences if it is ok in all use cases).

Thank you once more!

Martin Maechler
ETH Zurich