Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setDT Fails to Detect Locked Objects In Corner Case #1827

Closed
brodieG opened this issue Aug 26, 2016 · 5 comments
Closed

setDT Fails to Detect Locked Objects In Corner Case #1827

brodieG opened this issue Aug 26, 2016 · 5 comments

Comments

@brodieG
Copy link

brodieG commented Aug 26, 2016

Warning, running the following code will modify objects in locked namespaces:

mtcars2 <- mtcars
setDT(mtcars2)
mtcars                         # this is now a data.table

Obviously this happens because mtcars is never copied since mtcars2 is not touched. This bypasses the setDT safety check because the name mtcars2 is defined in .RGlobalEnv and is not locked, even though the associated SEXP is still the one in package:datasets.

This actual issue came up in the following example:

library(magrittr)
mtcars %>% setDT

Where magrittr "copies" mtcars to ., but since . isn't modified, it still points to the original mtcars SEXP.

@arunsrinivasan
Copy link
Member

Unfortunately locked binding allows an object to be modified by reference. setDT checks if an environment is locked, and if so errors. But mtcars2 isn't. I don't think there's anything we can do about this.

@brodieG
Copy link
Author

brodieG commented Aug 26, 2016

You could check whether NAMED() == 2 and refuse to apply setDT, with a warning on how to circumvent, and possibly an argument in setDT(force=TRUE) to override. Not a huge deal in my books though.

Alternatively, duplicate the object if NAMED() == 2?

@arunsrinivasan
Copy link
Member

Brodie, those won't work since all data.frames have NAMED() = 2 upon creation.

@brodieG
Copy link
Author

brodieG commented Aug 26, 2016

Didn't realize that. Do you want me to close this or just leave it here as wontfix for reference?

@arunsrinivasan
Copy link
Member

It's better to have it open, since it's still an issue. I labelled it just for easier searching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants