The flattening is unconditional. There is no option, and no way to ask this package for the conservative output that blockr.core produces — grep -rE "blockr_option|getOption|options\(" R/ returns nothing. That is survivable while core still ships generate_code() and a front-end can simply mount the other plugin, and it stops being survivable once BristolMyersSquibb/blockr.core#336 removes it and #8 makes this the only implementation.
What "flattened" means here, exactly
The two packages differ in one branch of wrap_expr(). Core wraps every non-with() expression:
if (length(args) && identical(types, "quoted")) {
call("with", args, exprs)
} else {
call("local", exprs)
}
Here the local() is dropped whenever the expression is judged not to contain an escaping assignment:
if (length(args) && identical(types, "quoted")) {
call("with", args, exprs)
} else if (has_assignment(exprs)) {
call("local", exprs)
} else {
exprs
}
So the whole of the flattening rests on has_assignment(), and through it on check_assignment_recursive() — a hand-written recursive walk over the language object with a special case each for <-, <<-, assign, function, \, local and {. When that walk is wrong, the emitted script is wrong in a way that runs.
Why this needs a switch rather than more confidence
The walk has already been wrong once, in a way that reached users. Commit f3be825 fixed check_assignment_recursive() calling as.character(expr[[1]]) without checking that the call head was a name: for pkg::fun(...) the head is itself a call, as.character() returned c("::", "pkg", "fun"), and the following fn == "<-" test aborted with 'length = 3' in coercion to 'logical(1)'. Its commit message is the relevant part — block expressions self-qualify their calls, so this "fired on essentially every board that mounts generate_flat_code() rather than on an edge case: clicking Show code errored out."
Today that class of defect costs a front-end nothing much: mount core's generate_code() and carry on. After core#336 there is no other implementation to fall back to, and a defect in the walk means the stack has no working code export at all. An option restores the fallback inside this package, where it becomes the only place it can live.
The direction of the failure matters too. The 'Show code' crash was loud. The walk returning FALSE for an expression that does assign is silent: the script emits at top level, the assignment leaks into the calling environment, and the code still runs.
Proposal
Gate the flattening on blockr_option("flatten_code", TRUE), which is exported from blockr.core and already reads both the blockr.flatten_code option and the BLOCKR_FLATTEN_CODE environment variable. Setting it to FALSE takes the else branch back to call("local", exprs), which is core's behaviour exactly, and costs four lines in wrap_expr(). Core's export_wrapped_code() is not exported, so this reimplements rather than delegates — but the branch being reimplemented is one line.
Default TRUE: idiomatic output is the reason this package exists, and the option is an escape hatch, not a migration flag.
Worth naming for later rather than settling now: the pipe folding in #1 and #3 is the same class of transformation over the same language objects, and it belongs behind the same switch when it lands.
The flattening is unconditional. There is no option, and no way to ask this package for the conservative output that
blockr.coreproduces —grep -rE "blockr_option|getOption|options\(" R/returns nothing. That is survivable while core still shipsgenerate_code()and a front-end can simply mount the other plugin, and it stops being survivable once BristolMyersSquibb/blockr.core#336 removes it and #8 makes this the only implementation.What "flattened" means here, exactly
The two packages differ in one branch of
wrap_expr(). Core wraps every non-with()expression:Here the
local()is dropped whenever the expression is judged not to contain an escaping assignment:So the whole of the flattening rests on
has_assignment(), and through it oncheck_assignment_recursive()— a hand-written recursive walk over the language object with a special case each for<-,<<-,assign,function,\,localand{. When that walk is wrong, the emitted script is wrong in a way that runs.Why this needs a switch rather than more confidence
The walk has already been wrong once, in a way that reached users. Commit
f3be825fixedcheck_assignment_recursive()callingas.character(expr[[1]])without checking that the call head was a name: forpkg::fun(...)the head is itself a call,as.character()returnedc("::", "pkg", "fun"), and the followingfn == "<-"test aborted with'length = 3' in coercion to 'logical(1)'. Its commit message is the relevant part — block expressions self-qualify their calls, so this "fired on essentially every board that mountsgenerate_flat_code()rather than on an edge case: clicking Show code errored out."Today that class of defect costs a front-end nothing much: mount core's
generate_code()and carry on. After core#336 there is no other implementation to fall back to, and a defect in the walk means the stack has no working code export at all. An option restores the fallback inside this package, where it becomes the only place it can live.The direction of the failure matters too. The 'Show code' crash was loud. The walk returning
FALSEfor an expression that does assign is silent: the script emits at top level, the assignment leaks into the calling environment, and the code still runs.Proposal
Gate the flattening on
blockr_option("flatten_code", TRUE), which is exported fromblockr.coreand already reads both theblockr.flatten_codeoption and theBLOCKR_FLATTEN_CODEenvironment variable. Setting it toFALSEtakes theelsebranch back tocall("local", exprs), which is core's behaviour exactly, and costs four lines inwrap_expr(). Core'sexport_wrapped_code()is not exported, so this reimplements rather than delegates — but the branch being reimplemented is one line.Default
TRUE: idiomatic output is the reason this package exists, and the option is an escape hatch, not a migration flag.Worth naming for later rather than settling now: the pipe folding in #1 and #3 is the same class of transformation over the same language objects, and it belongs behind the same switch when it lands.