Skip to content

Commit ae5b4a5

Browse files
MichaelChiricoMichaelChirico
authored
Avoid eval(parse(...)) internally (#7156)
* simplify setting options in .onLoad * another site * encode as a linter * fix, and use fixed=TRUE * cache regex in namespace instead of recomputing it at runtime * refactor to reduce nesting, use proper S3 method on condition object * move regex back to run-time generation for robustness * expression->quote * missed trailing ','
1 parent 50c8eb4 commit ae5b4a5

File tree

3 files changed

+50
-46
lines changed

3 files changed

+50
-46
lines changed

‎.ci/linters/r/eval_parse_linter.R‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
eval_parse_linter = make_linter_from_xpath(
2+
"//SYMBOL_FUNCTION_CALL[text() = 'parse']
3+
/ancestor::expr
4+
/preceding-sibling::expr[SYMBOL_FUNCTION_CALL[text() = 'eval']]
5+
/parent::expr
6+
",
7+
"Avoid eval(parse()); build the language directly, possibly using substitute2()."
8+
)

‎R/data.table.R‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -97,34 +97,32 @@ replace_dot_alias = function(e) {
9797
}
9898

9999
.checkTypos = function(err, ref) {
100+
err_str <- conditionMessage(err)
100101
# a slightly wonky workaround so that this still works in non-English sessions, #4989
101102
# generate this at run time (as opposed to e.g. onAttach) since session language is
102103
# technically OK to update (though this should be rare), and since it's low-cost
103104
# to do so here because we're about to error anyway.
104-
missing_obj_fmt = gsub(
105-
"'missing_datatable_variable____'",
105+
missing_obj_regex = gsub(
106+
"'____missing_datatable_variable____'",
106107
"'(?<obj_name>[^']+)'",
107-
tryCatch(eval(parse(text="missing_datatable_variable____")), error=identity)$message
108-
# eval(parse()) to avoid "no visible binding for global variable" note from R CMD check
109-
# names starting with _ don't parse, so no leading _ in the name
108+
# expression() to avoid "no visible binding for global variable" note from R CMD check
109+
conditionMessage(tryCatch(eval(quote(`____missing_datatable_variable____`)), error=identity)),
110+
fixed=TRUE
110111
)
111-
idx = regexpr(missing_obj_fmt, err$message, perl=TRUE)
112-
if (idx > 0L) {
113-
start = attr(idx, "capture.start", exact=TRUE)[ , "obj_name"]
114-
used = substr(
115-
err$message,
116-
start,
117-
start + attr(idx, "capture.length", exact=TRUE)[ , "obj_name"] - 1L
118-
)
119-
found = agrep(used, ref, value=TRUE, ignore.case=TRUE, fixed=TRUE)
120-
if (length(found)) {
121-
stopf("Object '%s' not found. Perhaps you intended %s", used, brackify(found))
122-
} else {
123-
stopf("Object '%s' not found amongst %s", used, brackify(ref))
124-
}
112+
idx = regexpr(missing_obj_regex, err_str, perl=TRUE)
113+
if (idx == -1L)
114+
stopf("%s", err_str, domain=NA) # Don't use stopf() directly, since err_str might have '%', #6588
115+
start = attr(idx, "capture.start", exact=TRUE)[ , "obj_name"]
116+
used = substr(
117+
err_str,
118+
start,
119+
start + attr(idx, "capture.length", exact=TRUE)[ , "obj_name"] - 1L
120+
)
121+
found = agrep(used, ref, value=TRUE, ignore.case=TRUE, fixed=TRUE)
122+
if (length(found)) {
123+
stopf("Object '%s' not found. Perhaps you intended %s", used, brackify(found))
125124
} else {
126-
# Don't use stopf() directly, since err$message might have '%', #6588
127-
stopf("%s", err$message, domain=NA)
125+
stopf("Object '%s' not found amongst %s", used, brackify(ref))
128126
}
129127
}
130128

‎R/onLoad.R‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -73,31 +73,29 @@
7373
# In fread and fwrite we have moved back to using getOption's default argument since it is unlikely fread and fread will be called in a loop many times, plus they
7474
# are relatively heavy functions where the overhead in getOption() would not be noticed. It's only really [.data.table where getOption default bit.
7575
# Improvement to base::getOption() now submitted (100x; 5s down to 0.05s): https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17394
76-
opts = c(
77-
"datatable.verbose"="FALSE", # datatable.<argument name>
78-
"datatable.optimize"="Inf", # datatable.<argument name>
79-
"datatable.print.nrows"="100L", # datatable.<argument name>
80-
"datatable.print.topn"="5L", # datatable.<argument name>
81-
"datatable.print.class"="TRUE", # for print.data.table
82-
"datatable.print.rownames"="TRUE", # for print.data.table
83-
"datatable.print.colnames"="'auto'", # for print.data.table
84-
"datatable.print.keys"="TRUE", # for print.data.table
85-
"datatable.print.trunc.cols"="FALSE", # for print.data.table
86-
"datatable.show.indices"="FALSE", # for print.data.table
87-
"datatable.allow.cartesian"="FALSE", # datatable.<argument name>
88-
"datatable.join.many"="TRUE", # mergelist, [.data.table #4383 #914
89-
"datatable.dfdispatchwarn"="TRUE", # not a function argument
90-
"datatable.warnredundantby"="TRUE", # not a function argument
91-
"datatable.alloccol"="1024L", # argument 'n' of alloc.col. Over-allocate 1024 spare column slots
92-
"datatable.auto.index"="TRUE", # DT[col=="val"] to auto add index so 2nd time faster
93-
"datatable.use.index"="TRUE", # global switch to address #1422
94-
"datatable.prettyprint.char" = NULL, # FR #1091
95-
"datatable.old.matrix.autoname"="TRUE", # #7145: how data.table(x=1, matrix(1)) is auto-named set to change
96-
NULL
97-
)
98-
for (i in setdiff(names(opts),names(options()))) {
99-
eval(parse(text=paste0("options(",i,"=",opts[i],")")))
100-
}
76+
opts = list(
77+
datatable.verbose=FALSE, # datatable.<argument name>
78+
datatable.optimize=Inf, # datatable.<argument name>
79+
datatable.print.nrows=100L, # datatable.<argument name>
80+
datatable.print.topn=5L, # datatable.<argument name>
81+
datatable.print.class=TRUE, # for print.data.table
82+
datatable.print.rownames=TRUE, # for print.data.table
83+
datatable.print.colnames='auto', # for print.data.table
84+
datatable.print.keys=TRUE, # for print.data.table
85+
datatable.print.trunc.cols=FALSE, # for print.data.table
86+
datatable.show.indices=FALSE, # for print.data.table
87+
datatable.allow.cartesian=FALSE, # datatable.<argument name>
88+
datatable.join.many=TRUE, # mergelist, [.data.table #4383 #914
89+
datatable.dfdispatchwarn=TRUE, # not a function argument
90+
datatable.warnredundantby=TRUE, # not a function argument
91+
datatable.alloccol=1024L, # argument 'n' of alloc.col. Over-allocate 1024 spare column slots
92+
datatable.auto.index=TRUE, # DT[col=="val"] to auto add index so 2nd time faster
93+
datatable.use.index=TRUE, # global switch to address #1422
94+
datatable.prettyprint.char=NULL, # FR #1091
95+
datatable.old.matrix.autoname=TRUE # #7145: how data.table(x=1, matrix(1)) is auto-named set to change
96+
)
97+
opts = opts[!names(opts) %chin% names(options())]
98+
options(opts)
10199

102100
# Test R behaviour that changed in v3.1 and is now depended on
103101
x = 1L:3L

0 commit comments

Comments
 (0)