Bug 15970 - tcltk package breaks when certain objects are in .GlobalEnv
Summary: tcltk package breaks when certain objects are in .GlobalEnv
Status: CLOSED FIXED
Alias: None
Product: R
Classification: Unclassified
Component: Misc (show other bugs)
Version: R-devel (trunk)
Hardware: All All
: P5 normal
Assignee: R-core
URL:
Depends on:
Blocks:
 
Reported: 2014-09-10 16:19 UTC by Konrad
Modified: 2014-09-11 16:10 UTC (History)
2 users (show)

See Also:


Attachments
Pull request for the described fix (774 bytes, application/mbox)
2014-09-10 16:19 UTC, Konrad
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Konrad 2014-09-10 16:19:00 UTC
Created attachment 1656 [details]
Pull request for the described fix

The .GlobalEnv scope gets leaked into tcltk, which means that having certain objects (re-)defined in .GlobalEnv (or in the search() path) breaks the package and generates an error.

An MWE:

```
library(tcltk)
`<-` = function (...) stop('foo')
tktoplevel()
```

Expected result:

```
$ID
[1] ".1"

$env
<environment: 0x7fe0e5500c78>

attr(,"class")
[1] "tkwin"
```

Actual result:

```
Error in num.subwin <- num.subwin + 1 : foo
```

The source of the error is src/library/tcltk/R/Tk.R:169:

```
win <- list(ID = ID, env = evalq(new.env(), .GlobalEnv))
```

It’s unclear why this line is using `evalq` in the first place, but maybe there’s a good reason for not creating a new environment whose parent is the local scope. At any rate, the parent should *not* be .GlobalEnv: this breaks the package’s isolation. Instead, `base()` or the package’s on namespace should be used. I’ve attached a patch/PR based on that.

A final remark: this example may seem arcane since it overloads `<-`. However, the bug would also occur for other cases (e.g. `+` and presumably others). Furthermore, the above is entirely *valid* R code and by virtue of that alone it should work. In particular, overriding `<-` in the global environment does not break other packages. The actual code where this occurred is exploiting the redundancy of `<-` and `=` to override `<-` for something worthwhile that is being used in real-world code [1].

[1]: https://gist.github.com/klmr/25dc765211c59bb749b0
Comment 1 Duncan Murdoch 2014-09-10 19:17:56 UTC
It looks to me as though the intention of the line

    win <- list(ID = ID, env = evalq(new.env(), .GlobalEnv))
    evalq(num.subwin <- 0, win$env)

is to create a new environment with .GlobalEnv as the parent, and then to assign 0 to num.subwin in that environment, i.e. equivalent to

 win <- list(ID = ID, env = new.env(parent = .GlobalEnv))
 win$env$num.subwin <- 0

The code in tcltk is 14 years old, so I'm pretty sure the rewriting I've done was not available then; that might explain the use of evalq().  (I think there are other ways to write it that would have been available, but I'm not sure.)

But your claim is that the parent of env should be the namespace or base, not .GlobalEnv.  The code predates namespaces, so you might be right, but I still think you need a stronger argument for that.  Are there any cases where env is used and the variable being sought is not in it?  That's the only time the parent matters.
Comment 2 Konrad 2014-09-11 09:16:25 UTC
> But your claim is that the parent of env should be the namespace or base, not .GlobalEnv.  The code predates namespaces, so you might be right, but I still think you need a stronger argument for that.

The only other argument I have is that I can’t think of a valid reason to make the global scope intentionally leak into a package (except for inspection). But really, the strongest argument is that the package breaks when used in conjunction with valid, non-intrusive (i.e. not trying to tamper with the package) R code.

Of course you’re right that the bug could be fixed by performing the relevant assignment in the current scope instead of via `evalq`, and maybe that’s the safer approach. I don’t know the code well enough to judge that. It would require changes in more than one place though.

> Are there any cases where env is used and the variable being sought is not in it?  That's the only time the parent matters.

I *think* that there is no such case, from my reading of the code. The only variables that I see being used in this environment are all assigned before they are read (`subwin`, `parent`, `TclVarCount`, the window ID, and the callbacks, to guard against premature GC (?)). In fact, the code explicitly guards against inheriting when testing whether a variable exists in the environment in some cases.

I’m not very confident though, since tracing all uses of the environment is hairy.
Comment 3 Peter Dalgaard 2014-09-11 12:16:17 UTC
As Duncan points out, the code predates namespaces, and probably also other things that might have been useful. Possibly, also new.env() was different at the time -- I believe it was once defined as function()environment().

If I remember my chain of thought (from some time close to the turn of the millennium!) correctly, there was indeed a point to creating the window environment to be rooted somewhere outside of the local scope; the window objects are designed to hang around indefinitely (they need to be there as long as the window exists) and there was the possibility of a memory-consuming chain of calling environments opening a window, causing garbage collection to be prevented.

With namespaces, this could quite likely be designed better. On the other hand, if redefining `<-`is the only way to trigger a problem, I'm not sure it needs to be given very high priority.
Comment 4 Konrad 2014-09-11 13:20:21 UTC
The problem also occurs for (weird) redefinitions of `+`. I agree that this isn’t a common problem. It does break my (real) code though: I’m being naughty and redefine `<-` to have a concise lambda notation [1].

That said, I agree with the priority, and I didn’t have high hopes that this would be fixed soon. But hopefully the fix is pretty straightforward. I didn’t test my patch extensively but it does work.

[1]: https://gist.github.com/klmr/25dc765211c59bb749b0
Comment 5 Duncan Murdoch 2014-09-11 16:10:16 UTC
I'll be committing a fix soon.  It doesn't use the submitted patch; it avoids calling evalq() completely, and sets the parent to the empty environment.