Scaffold NSE support - #1339
Conversation
d9505e7 to
5bea085
Compare
DavisVaughan
left a comment
There was a problem hiding this comment.
I've looked at everything except for builder_nse.rs, which I'll do on Monday, but here are some starting comments
| fn resolve_qualified_effects(&mut self, package: &str, name: &str) -> Option<Effects> { | ||
| effects_registry::lookup(package, name) | ||
| .copied() | ||
| .map(Effects::nse) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do you think this one really needs to be a trait method?
It feels like this is the definitive implementation for this function, and it doesn't use self at all.
Like, could it just be a "free" function that queries this external effect registry?
There was a problem hiding this comment.
The registry is a crutch, but ideally the effects should come from annotations, and then we do need the imports resolver.
The method will probably evolve over time though. Some thoughts:
- Effects parsing belongs in oak_semantic, so the oak_db impl would have to call oak_semantic to parse decls
- Maybe instead we resolve to a definition and parse the decls from there. In this case the trait method is no longer about effects at all.
| pub enum NseScope { | ||
| /// Definitions go to the current (parent) environment. | ||
| /// e.g. `rlang::on_load()` | ||
| Current, | ||
| /// Definitions go to a nested environment. | ||
| /// e.g. `local()`, `test_that()`, `with()` | ||
| Nested, | ||
| } |
There was a problem hiding this comment.
Isn't there a 3rd possibility? Like Parent maybe? I'm thinking of some environment that is passed through multiple layers before finally being used. This is a bad example but kind of demonstrates the idea:
x <- "foo"
fn <- function(env = caller_env()) {
fn2(env)
}
fn2 <- function(env = caller_env()) {
x <- "bar"
# `env` here isn't really Current, it's multiple layers up
eval(quote(x), envir = env)
}
fn()
#> [1] "foo"There was a problem hiding this comment.
I'm not sure yet how this will play out exactly, but this struct will change completely when we introduce environment capture. This will require some thinking.
| /// Annotation describing how an NSE function's arguments create scopes. | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub struct ArgumentsAnnotation { | ||
| pub arguments: &'static [Argument], |
There was a problem hiding this comment.
Neat entry! macro to get a static lifetime
| pub nse: Option<ArgumentsAnnotation>, | ||
| } | ||
|
|
||
| impl Effects { | ||
| pub fn nse(nse: ArgumentsAnnotation) -> Self { | ||
| Self { nse: Some(nse) } | ||
| } |
There was a problem hiding this comment.
Nit:
I have some gut feeling that pub nse here should really be pub arguments or something?
Like when use use Effect::nse it seems like its because you're wrapping argument info.
Or maybe it's that a function can (eventually) contribute 3 ish kinds of effects:
- NSE
- Attach
- Assign
And it just so happens to be that the only way it can contribute an NSE effect is via these arguments? That's fine then, I guess.
There was a problem hiding this comment.
Yep this becomes arguments in an ulterior PR. In addition to Nse (which will become EvalQ), an argument might be Quote or Eval.
| /// local({ | ||
| /// x # {1, 2}: Should be {1} but shares one snapshot with the use below | ||
| /// x <<- 2 # deferred def, folded into the snapshot | ||
| /// x # {1, 2} | ||
| /// }) |
There was a problem hiding this comment.
And what happens when you start incorporating the envir argument?
There was a problem hiding this comment.
It'll keep working as is when the environment is nested or current. What happens in the other cases depends on what the environment resolves to (unknown, data-masked, etc).
| /// ```r | ||
| /// x <- 1 | ||
| /// local({ | ||
| /// x # {1, 2}: Should be {1} but shares one snapshot with the use below |
There was a problem hiding this comment.
"Should be" makes me think we have a bug, is it?
There was a problem hiding this comment.
More like a known limitation, but I've just refactored things to lift it.
3668c8f to
31490b4
Compare
0af41a1 to
05edf5d
Compare
Progress towards #1338
This PR adds preliminary support for NSE functions that create a local scope, like
local()ortest_that(). Data-masking functions likewith()are also supported, although only for the effect of opening a scope, not for the effect of injecting a data frame in the scope.The semantic index is created in two passes per scan unit to resolve a chicken and egg problem: to recognise an NSE function while we're building the index, we need to resolve the function callee: is it locally bound, perhaps to a function with NSE annotations? If it's locally unbound, does it come from an imported file?
The two passes are structured by scan units, which are either the file scope, or a lazy scope. Eager scopes are just traversed linearly without creating a new unit.
We already had a similar pre-scan to collect bound symbols for lazy scopes. This first pass now has more responsibilities. The two passes design is forward-looking and takes into account that file imports from
source()(andlibrary()in a future PR) calls may themselves be masked by assignments, which may or may not have local effects if they happen within a nested NSE scope.NSE functions are parameterised along two axes:
NseScope: EitherCurrent(evalq(),rlang::on_load()) orNested(local(),test_that()).NseTiming: EitherEager(local(),test_that()) orLazy(rlang::on_load(),shiny::observe())Each combination gets distinct handling:
Current + Eager: Same as regular evaluation. The reason this combination exists is that in the future we'll support the
envirargument ofevalq(), at which point it will stop falling into the Current bucket depending on its arguments.Nested + Eager: Pushes a scope. Scanned inline during the eager linear scan. Uses of enclosing names get a point-in-time snapshot, unlike the accumulated union a lazy scope gets. That's the main difference with lazy scopes like functions.
Current + Lazy: It's a special scan unit whose definitions are deferred, just like
<<-Nested + Lazy: Pushes a scope, special scan unit. Uses refer to an enclosing snapshot, exactly like
functionscopes.The linear scanner decides whether a function is NSE by resolving symbols eagerly. This is always correct in an eager context, but is only a guess in a lazy context, like inside a function or Shiny reactive. If a binding comes in later and contradicts that guess, we can't order these two states of the world.
The position that we take from now on is that this is an ambiguous program that should be linted. We record the ambiguity in
LazyShadowAmbiguity { name, range }and log it later on. In a future PR, we'll surface in diagnostics.That's a general principle that we'll apply for all effects, for instance this will produce a lint:
Imported effects are now handled by new ImportsResolver methods. The PR also introduces an effect registry where we'll start documenting effects of known functions from base and other important packages (rlang, shiny, etc).
Positron Release Notes
New Features
test_that().Bug Fixes