about summary refs log tree commit diff
path: root/tvix/eval/src/compiler
AgeCommit message (Collapse)AuthorFilesLines
2022-12-21 r/5454 refactor(tvix/eval): add name-based index over compiler's localsVincent Ambo2-34/+108
Instead of finding locals by doing 2x O(n) walks over the compiler's locals list, use a secondary name-based index for resolving locals by name. Previously, almost 60% (!!) of eval time on some expressions over nixpkgs was spent in `Local::has_name`. This function doesn't even exist anymore now, and eval speed about doubles as a result. Note that this doesn't exactly make the locals code easier to read, but I'm also not sure what we can simplify in there in general. This fixes b/227. Change-Id: I29ce5eb9452b02d3b358c673e1f5cf8082e2fef9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7560 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-12-21 r/5452 feat(tvix/eval): wrap Closure in Rc<> to match cppnix semanticsAdam Joseph1-1/+1
Change-Id: I595087eff943d38a9fc78a83d37e207bb2ab79bc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7443 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-11-05 r/5251 fix(tvix/eval): Scope.inherit(): fix scope_depth, with_stack_depthAdam Joseph1-0/+2
Scope_depth and with_stack_depth were being reset to zero for nested function abstractions. Fortunately nothing depends on them being computed correctly in these cases, but it sure was confusing. Change-Id: I59980b6a5aff043f60079f97211220b0086eb97d Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7091 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-11-05 r/5250 refactor(tvix/eval): rename Opcode::DataLocalIdx to DataStackIdxAdam Joseph1-1/+1
It is very confusing that this opcode is called DataLocalIdx, but it carries a StackIdx rather than a LocalIdx. It seems like this really ought to be called DataStackIdx, but maybe I've misunderstood; if so please explain it to me. Change-Id: I91f6ffa759412beef0b91d3c19ec0d873fe51b99 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7088 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-11-04 r/5237 fix(tvix/eval): inline mis-named Local::above()Adam Joseph1-6/+1
If self.depth > other.depth then self is deeper than other, so self is *below* other, not above it. Let's just inline the function. Change-Id: I8dda3d90cbc86c8a6fa01bc4a5e506a2e403bd20 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7090 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-27 r/5214 refactor(tvix/eval): search-and-replace changesAdam Joseph1-5/+5
This commit contains two search-and-replace renames which are broken out from I04131501029772f30e28da8281d864427685097f in order to reduce the noise in that CL: - `is_thunk -> is_suspended_thunk`, since there are now OpThunkClosure and OpThunkSuspended - `compile_lambda_or_thunk` -> `compile_lambda_or_suspension` Change-Id: I7cc5bbb75ef6605e3428c7be27e812f41a10c127 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7037 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-27 r/5213 feat(tvix/eval): builtins.import without RefCellAdam Joseph1-45/+51
CL/6867 added support for builtins.import, which required a cyclic reference import->globals->builtins->import. This was implemented using a RefCell, which makes it possible to mutate the builtins during evaluation. The commit message for CL/6867 expressed a desire to eliminate this possibility: This opens up a potentially dangerous footgun in which we could mutate the builtins at runtime leading to different compiler invocations seeing different builtins, so it'd be nice to have some kind of "finalised" status for them or some such, but I'm not sure how to represent that atm. This CL replaces the RefCell with Rc::new_cyclic(), making the globals/builtins immutable once again. At VM runtime (once opcodes start executing) everything is the same as before this CL, except that the Rc<RefCell<>> introduced by CL/6867 is turned into an rc::Weak<>. The function passed to Rc::new_cyclic works very similarly to overlays in nixpkgs: a function takes its own result as an argument. However instead of laziness "breaking the cycle", Rust's Rc::new_cyclic() instead uses an rc::Weak. This is done to prevent memory leaks rather than divergence. This CL also resolves the following TODO from CL/6867: // TODO: encapsulate this import weirdness in builtins The main disadvantage of this CL is the fact that the VM now must ensure that it holds a strong reference to the globals while a program is executing; failure to do so will cause a panic when the weak reference in the builtins is upgrade()d. In theory it should be possible to create strong reference cycles the same way Rc::new_cyclic() creates weak cycles, but these cycles would cause a permanent memory leak -- without either an rc::Weak or RefCell there is no way to break the cycle. At some point we will have to implement some form of cycle collection; whatever library we choose for that purpose is likely to provide an "immutable strong reference cycle" primitive similar to Rc::new_cyclic(), and we should be able to simply drop it in. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: I34bb5821628eb97e426bdb880b02e2097402adb7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7097 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-26 r/5201 docs(tvix/eval): StackIdx, LocalIdx UpvalueIdxAdam Joseph1-3/+1
This adds a comment noting that StackIdx is an offset relative to the base of the current CallFrame, whereas UpvalueIdx is an absolute index into the upvalues array. It also removes the confusing mention of StackIdx in the descriptive comment for LocalIdx. They index into totally different structures; one exists at runtime and the other exists at compile time. Change-Id: Ib932b1b0679734c15001e8c5c95a08293fa016b4 Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7017 Reviewed-by: grfn <grfn@gws.fyi> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-23 r/5186 feat(tvix/eval): initial attempt at setting lambda namesVincent Ambo2-2/+18
When compiling a lambda, take the name of the outer slot (if available) and store it as the name on the lambda. These names are then shown in the observer, and nowhere else (so far). It is of course common for these things to thread through many different context levels (e.g. `f = a: b: c: ...`), in this setup only the outermost closure or thunk gains the name, but it's better than nothing. Change-Id: I681ba74e624f2b9e7a147144a27acf364fe6ccc7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7065 Reviewed-by: grfn <grfn@gws.fyi> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-23 r/5184 fix(tvix/eval): thunk let-expressionVincent Ambo1-1/+5
There are some rare scope cases with deferred access where this doesn't behave correctly otherwise. Change-Id: I6c774f5e62c1cb50b598026c54727017a52cd22d Reviewed-on: https://cl.tvl.fyi/c/depot/+/7064 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-10-23 r/5183 fix(tvix/eval): fix condition for useless inherit warningVincent Ambo1-2/+2
The warning needs to consider whether it is occuring inside of a thunk, i.e. the dynamic ancestry chain of lambda contexts must be inspected and not just the current scope. Change-Id: I5cf5482d67a8bbb9f03b0ecee7a62f58754f8e59 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7063 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: grfn <grfn@gws.fyi>
2022-10-23 r/5182 refactor(tvix/eval): simplify check for deferring upvalue resolutionVincent Ambo1-9/+4
This check is now actually simply equivalent to checking whether the target has been initialised or not. Change-Id: I30660d11073ba313358f3a64234a90ed81abf74c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7062 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-23 r/5180 refactor(tvix/eval): simplify self-reference checkVincent Ambo1-1/+1
Checking the computed depth and stack slot against the computed depth and stack slot is equivalent to just checking the indices into the locals vector against each other (i.e. "is the slot we're compiling into the slot we're accessing?") Change-Id: Ie85a68df073e3b2e3d9aba7fe8634c48eada81fc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7059 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-21 r/5172 fix(tvix): distinguish search- and relative path resolution errorssterni1-2/+2
Failures to resolve a nix search path lookup in angle brackets can be caught using tryEval (if it reaches the runtime). Resolving relative paths (either to the current directory or the current user's home) can never be caught, even if they happen inside a thunk at runtime (which is currently the case for home-relative paths). Change-Id: I7f73221df66d82a381dd4063358906257826995a Reviewed-on: https://cl.tvl.fyi/c/depot/+/7025 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-19 r/5159 feat(tvix/eval): deduplicate overlap between Closure and ThunkAdam Joseph2-4/+36
This commit deduplicates the Thunk-like functionality from Closure and unifies it with Thunk. Specifically, we now have one and only one way of breaking reference cycles in the Value-graph: Thunk. No other variant contains a RefCell. This should make it easier to reason about the behavior of the VM. InnerClosure and UpvaluesCarrier are no longer necessary. This refactoring allowed an improvement in code generation: `Rc<RefCell<>>`s are now created only for closures which do not have self-references or deferred upvalues, instead of for all closures. OpClosure has been split into two separate opcodes: - OpClosure creates non-recursive closures with no deferred upvalues. The VM will not create an `Rc<RefCell<>>` when executing this instruction. - OpThunkClosure is used for closures with self-references or deferred upvalues. The VM will create a Thunk when executing this opcode, but the Thunk will start out already in the `ThunkRepr::Evaluated` state, rather than in the `ThunkRepr::Suspeneded` state. To avoid confusion, OpThunk has been renamed OpThunkSuspended. Thanks to @sterni for suggesting that all this could be done without adding an additional variant to ThunkRepr. This does however mean that there will be mutating accesses to `ThunkRepr::Evaluated`, which was not previously the case. The field `is_finalised:bool` has been added to `Closure` to ensure that these mutating accesses are performed only on finalised Closures. Both the check and the field are present only if `#[cfg(debug_assertions)]`. Change-Id: I04131501029772f30e28da8281d864427685097f Signed-off-by: Adam Joseph <adam@westernsemico.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-18 r/5156 fix(tvix/eval): wrap dynamic resolution in an extra thunkVincent Ambo1-2/+9
Without this change it was possible to cause situations (see the new test) in which a `with`-namespace was forced prematurely. Change-Id: I879ea7763b43edc693feace2c73c890d426fafd3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7031 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2022-10-17 r/5154 feat(tvix/eval): Validate closed formalsGriffin Smith1-7/+6
Validate "closed formals" (formal parameters without an ellipsis) via a new ValidateClosedFormals op, which checks the arguments (in an attr set at the top of the stack) against the formal parameters on the Lambda in the current frame, and returns a new UnexpectedArgument error (including the span of the formals themselves!!) if any arguments aren't allowed Change-Id: Idcc47a59167a83be1832a6229f137d84e426c56c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7002 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-17 r/5153 feat(tvix/eval): Record formals on lambdaGriffin Smith1-11/+20
In preparation for both implementing the `functionArgs` builtin and adding support for validating closed formals, record information about the formal arguments to a function *on the Lambda itself*. This may seem a little odd for the purposes of just closed formal checking, but is something we have to have anyway for builtins.functionArgs so I figured I'd do it this way to kill both birds with one stone. Change-Id: Ie3770a607bf352a1eb395c79ca29bb25d5978cd8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7001 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-16 r/5150 refactor(tvix/eval): unify compile_lambda() with thunk()Adam Joseph1-61/+59
This resolves a TODO. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: If4d2124648ac88094e547e1ad7f1b446feb26182 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7010 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-16 r/5147 fix(tvix/eval): resolve home relative paths at runtimesterni1-12/+8
Home relative paths depend on the environment to be resolved. We have elected to do everything that depends on the environment, e.g. resolving SPATH expressions using NIX_PATH, at runtime, so tvix evaluation would continue to behave correctly even if we separated the compilation and execution phases more, e.g. via serializing bytecode. Then the value of HOME, NIX_PATH etc. could reasonably change in the time until execution, yielding wrong results if the resolution results were cached in the bytecode. We also take the opportunity to fix the broken path concatenation previously found in the compiler, fixing b/205. Another thing we could consider is emitting a warning for home relative path literals, as they are by nature relatively fragile. One sideeffect of this change is that home path resolution errors become catchable which is not the case in C++ Nix. This will need to be fixed up in a subsequent change. Change-Id: I30bd69b575667c49170a9fdea23a020565d0f9ec Reviewed-on: https://cl.tvl.fyi/c/depot/+/7024 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2022-10-16 r/5146 refactor(tvix/eval): make OpFindFile use internal UnresolvedPathsterni1-1/+1
To assert that OpFindFile is only emitted for specially compiled SPATH expressions, as well as make sure it doesn't accidentally operate on “ordinary values”, introduce an UnresolvedPath internal value. If OpFindFile sees a non-UnresolvedPath value, it'll crash. Note that this change is not done purely for OpFindFile: We may want to compile SPATH expressions as function calls to __findFile (like C++ Nix does) in the future, so the UnresolvedPath value would definitely need to be an ordinary string again then. Rather, this change is done in preparation for resolving home dir relative paths at runtime (since they depend on the environment) for which we'll need a similar mechanism to OpFindFile. Change-Id: I6acf287f35197cd9e13377079f972b9d36e5b22e Reviewed-on: https://cl.tvl.fyi/c/depot/+/7023 Autosubmit: sterni <sternenseemann@systemli.org> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-10-13 r/5121 fix(tvix/eval): fix Compiler::new on wasmVincent Ambo1-1/+3
This path normalisation business causes runtime panics on WebAssembly because those operations are unsupported. Maybe this shouldn't be happening in the compiler anyways, not sure, but for now this commit adds a workaround based on the target to disable the normalisation if we're compiling for wasm. Change-Id: I908a84fbdffc3401f8d443e2c73ec673e9f397ff Reviewed-on: https://cl.tvl.fyi/c/depot/+/7004 Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-10-13 r/5120 fix(tvix/eval): src/compiler: ensure root_dir is absoluteAdam Joseph1-8/+16
Cppnix immediately absolutizes pathnames at parse time; if you write `./foo`, it is immediately converted to `$(pwd)/foo` and manipulated as an absolute path at all times. To avoid having to introduce filesystem access operations in the implementation of otherwise-pure builtins, let's guarantee that the `root_dir` of the VM is always an absolute path. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: I7cbbae2cba4b2716ff3f5ff7c9ce0ad529358c8a Reviewed-on: https://cl.tvl.fyi/c/depot/+/6995 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-10-13 r/5119 refactor(tvix/eval): factor out all calls to canon_pathAdam Joseph1-2/+1
Right now we're pretending that the Rust library path_clean does the same thing that cppnix's canonPath() does. This is not true. It's close enough for the test suite, but may come back to bite us. Let's create our own canon_path() function and call that in all the places where we intend to match the behavior of cppnix's canonPath(). That way when we fix this we can fix it once, in one place. Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: Ia6f9577f62f49ef352ff9cfa5efdf37c32d31b11 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6993 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-10 r/5097 fix(tvix/eval): Actually trace spans for thunksGriffin Smith1-1/+1
Currently, the span on *all* thunk force errors is the span at which the thunk is forced, which for recursive thunk forcing ends up just being the same span over and over again. This changes the span on thunk force errors to be the span at which point the thunk is *created*, which is a bit more helpful (though the printing atm is a little... crowded). To make this work, we have to thread through the span at which a thunk is created into a field on the thunk itself. Change-Id: I81474810a763046e2eb3a8f07acf7d8ec708824a Reviewed-on: https://cl.tvl.fyi/c/depot/+/6932 Autosubmit: grfn <grfn@gws.fyi> Reviewed-by: Adam Joseph <adam@westernsemico.com> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-10 r/5090 fix(tvix/eval): Thunk `if` exprGriffin Smith1-1/+3
Since the body of an `if` expr can refer to deferred upvalues, it needs to be thunked so when we actually compile those deferred upvalues we have something for the finalize op to point at. Without this all sorts of weird things can happen due to the finalize op being run in the wrong lambda context, up to and including a panic. Change-Id: I040d5e1a7232fd841cfa4953539898fa49cbbb83 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6929 Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-10-10 r/5087 feat(tvix/eval): Initial resolution of `<...>` pathsGriffin Smith1-6/+17
This commit implements (lazy) resolution of `<...>` paths via either the NIX_PATH environment variable, or the -I command-line flag - both handled via EvalOptions. As a result, EvalOptions can no longer derive Copy, meaning we have to clone it at each line of the repl - this is probably not a huge deal as repl performance is not exactly an inner loop and we're not cloning very much. Internally, this works by creating a thunk which pushes a constant containing the string inside the brackets to the stack, then a new opcode to resolve that path via the `NixPath`. To get that opcode to work, we now have to pass in the NixPath when constructing the VM. This (intentionally) leaves out proper implementation of path resolution via `findFile` (cppnix just calls whatever identifier called findFile is in scope!!!) as that's widely considered a bit of a misfeature, but if we do decide to implement that down the road it likely wouldn't be more than a few extra ops within the thunk introduced here. Change-Id: Ibc979b7e425b65cbe88599940520239a4a10cee2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6918 Autosubmit: grfn <grfn@gws.fyi> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-10 r/5082 refactor(tvix/eval): Compile OpAssert using conditional jumpsGriffin Smith1-4/+22
In order to behave nicely with tryEval, asserts need to leave the instruction pointer in a reasonable place even if they fail - whereas with the previous implementation catching a failed assert would still end up running the op for the *body* of the assert. With this change, we compile asserts much more like an `if` expression with conditional jumps rather than having an OpAssert op. Change-Id: I1b266c3be90185c84000da6b1995ac3e6fd5471b Reviewed-on: https://cl.tvl.fyi/c/depot/+/6925 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-08 r/5069 fix(tvix/eval): end scope after compiling legacy let bindingsVincent Ambo1-0/+5
When contrasting the compilation of the desugared version to the "sugared" version, this was the noticeable difference. This fixes b/203. Change-Id: Iae02ffc56e06de1de091b84cdc59d8fe83a17d69 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6898 Reviewed-by: grfn <grfn@gws.fyi> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-10-08 r/5057 refactor(tvix/eval): move `spans` module to crate rootVincent Ambo2-86/+7
This is also useful for error-handling related logic, outside of just the compiler module. Change-Id: I5c386e2b4c31cda0a0209b31136ca07f00e39e45 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6869 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-07 r/5048 feat(tvix/eval): insert `import` into the builtins itselfVincent Ambo1-5/+9
Adding `import` to builtins causes causes a bootstrap cycle because the `import` builtin needs to be initialised with the set of globals before being inserted into the globals, which also must contain itself. To break out of the cycle this hack wraps the builtins passed to the compiler in an `Rc` (probably sensible anyways, as they will end up getting cloned a bunch), containing a RefCell which gives us mutable access to the builtins. This opens up a potentially dangerous footgun in which we could mutate the builtins at runtime leading to different compiler invocations seeing different builtins, so it'd be nice to have some kind of "finalised" status for them or some such, but I'm not sure how to represent that atm. Change-Id: I25f8d4d2a7e8472d401c8ba2f4bbf9d86ab2abcb Reviewed-on: https://cl.tvl.fyi/c/depot/+/6867 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-04 r/5034 refactor(tvix/eval): remove unnecessary clones in compilerVincent Ambo2-148/+144
There's basically nothing that needs *ownership* of an AST node (which is just a little box full of references to other things anyways), so we can thread this through as references all the way. Change-Id: I35a1348a50c0e8e07d51dfc18847829379166fbf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6853 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-10-04 r/5033 refactor(tvix/eval): split observer traits in twoVincent Ambo1-4/+4
There are actually two different types of observers, the ones that observe the compiler (and emitted chunks from different kinds of expressions), and the ones that trace runtime execution. Use of the NoOpObserver is unchanged, it simply implements both traits. Change-Id: I4277b82674c259ec55238a0de3bb1cdf5e21a258 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6852 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-09-30 r/5004 chore(tvix/eval): remove unused field in TrackedBindingsVincent Ambo1-7/+3
Change-Id: I65e31e9173e4f5bba19cc4e3d45eb4f8bf91b424 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6808 Reviewed-by: grfn <grfn@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-30 r/5002 feat(tvix/eval): implement nested keysVincent Ambo1-29/+60
This finishes up the implementation of nested keys after the key insight that the nesting level does not need to be tracked, and instead the attribute iterator can simply be retained inside the structures as is (in an advanced state). With this implementation, when encountering a nested key, the Tvix compiler will first analyse whether there is already a matching binding that can be merged (i.e. a binding that is a literal attribute set), and perform the merge, or otherwise create a new recursive set of bindings in which the entry is inserted with the path iterator advanced beyond the first name component. With this, all the logic simply applies recursively until there are no more nested bindings (i.e. until all iterators are "empty"). Note that this has one (potentially insignificant) deviation from Nix currently: If a non-mergable value is supplied (e.g. `a.b = 1; a = 2;`), Tvix will emit a *runtime* error (whereas it is *parse* time in Nix) as the branch which could statically analyse this is currently unreachable. There's a TODO for this, so we can fix it up later. Change-Id: I53df70e09614ff4281a70b80eac7da3beca12da9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6806 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-30 r/5001 refactor(tvix/eval): split out AttributeSet::from_ast helperVincent Ambo1-26/+31
Change-Id: Id43dbd06aef14cf01b4901d9b3668d790cd2b5ae Reviewed-on: https://cl.tvl.fyi/c/depot/+/6805 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-09-30 r/5000 chore(tvix/eval): remove `nesting_level` trackingVincent Ambo1-25/+2
This is actually quite useless, as we can just pass `AstChildren<ast::Attr>` around after partially consuming it. Change-Id: If0aefa2b53fc801fced1ae0709bff93966bf19f8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6804 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-09-29 r/4999 refactor(tvix/eval): clean up representation flip in bindingsVincent Ambo1-37/+32
When encountering a nested binding for the first time, cleanly flip the representation to `Binding::Set` in `Binding::merge` before proceeding with the actual merge. This reduces the number of points where we have to deal with the (soon to be slightly more complex) construction of the nested binding representation. Change-Id: Ifd43aac7b59ebd15a72c3ec512386a5bcf26ec13 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6802 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4998 feat(tvix/eval): (partially) track nesting level of attrsetsVincent Ambo1-29/+82
This adds the scaffolding required for tracking the nesting level (and appropriately skipping the correct amount of attrpath entries when inserting nested sets). In order for all of this to work correctly, we can no longer track `AttrpathValue` directly in the entries vector as rnix does not allow us to construct values of that type - so instead we have to track its inner components. Change-Id: Icb18e105586bf6c247c2e66c302cde5609ad9789 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6801 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4997 feat(tvix/eval): merge attribute sets in bindingsVincent Ambo1-13/+110
This is a significant step towards correctly implemented nested bindings. All attribute sets defined within the same binding scope will now be merged as in Nix, if they use the same key. Change-Id: I13e056693d5e73192280043c6dd93b47d1306ed6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6800 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4996 feat(tvix/eval): add error kind for unmergeable nested attributesVincent Ambo1-1/+4
Change-Id: Ic5e6d1bf2625c33938360affb0d1a7c922af11bf Reviewed-on: https://cl.tvl.fyi/c/depot/+/6799 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4995 feat(tvix/eval): add scaffolding for merging nested attribute setsVincent Ambo1-25/+101
This sets up the required logic for finding and merging attribute sets into nested bindings if they exist. This is absolutely not complete yet and can, at this commit, probably cause undefined runtime behaviour if nested attributes are specified. The basic idea is that a new helper function on the `TrackedBindings` struct is called with each encountered attribute and determines whether the new entry can be merged into an existing attribute or not. Right now the only effect this has in practice is that a new error becomes available if somebody attempts to cause a merge into an inherited key. Change-Id: Id010df3605055eb1ad7fa65241055889dd21bab0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6798 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4994 refactor(tvix/eval): emit OpAttrs inside of compile_bindingsVincent Ambo2-6/+6
This needs to move here so that we can reuse compile_bindings for the nested attribute sets we're about to start constructing. Change-Id: Ie83f52f7e1d128886e96a1da47792211fa826f21 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6796 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4993 refactor(tvix/eval): introduce `TrackedBindings` structVincent Ambo1-11/+34
This struct will be the key to correctly compiling nested bindings, by having insertions flow through some logic that will attempt to bind attribute-set-like things when encountering them. Change-Id: I8b5b20798de60688f3b6dc4526a460ebb2079f6e Reviewed-on: https://cl.tvl.fyi/c/depot/+/6795 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4992 refactor(tvix/eval): compile_recursive_scope -> compile_bindingsVincent Ambo1-4/+4
Change-Id: Iff18d0f84ba2b7a4194797e6c52c55b1c37e419c Reviewed-on: https://cl.tvl.fyi/c/depot/+/6794 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4991 chore(tvix/eval): fix all current clippy lintsVincent Ambo1-2/+2
Change-Id: I28d6af8cb408f8427a75d30b9120aaa809a1ea40 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6784 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4989 refactor(tvix/eval): merge all bindings creation logicVincent Ambo2-136/+6
As of this commit, all three types of bindings scopes are compiled the same way (i.e. compilation of non-recursive attribute sets has been switched over to the new code paths). This sets us up for doing the final implementation of nested attribute sets. HOWEVER, this breaks the existing implementation of nested attributes in non-recursive attribute sets. That implementation is flawed and unworkable in practice, so we need to do this dance to be able to implement it correctly. Change-Id: Iba2545c0d1d6b51f5e1a31a5d005b8d01da546d3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6782 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4988 feat(tvix/eval): implement dynamic keys in recursive attrsVincent Ambo1-29/+33
This wires up the new bindings setup logic to be able to thread through & compile dynamic attributes in recursive attrs. It seems like we don't actually need to retain the phasing of Nix exactly, as we can use the phantom mechanism to declare all locals without making the dynamic ones accessible. Change-Id: Ic2d43dd8fd97d7ccd56d8c6adf2ff97274cd837a Reviewed-on: https://cl.tvl.fyi/c/depot/+/6781 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-09-29 r/4986 feat(tvix/eval): add KeySlot::Dynamic variant for dynamic keysVincent Ambo1-14/+29
Another slice of the salami, but no functionality changes yet (other than opening a code path that can reach a `todo!()`, but this will be removed soon). Change-Id: I56b4ed323f70754ed1ab27964ee3c99cf3bf3292 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6780 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-09-29 r/4985 refactor(tvix/eval): clean up unused attrpath normalisation logicVincent Ambo1-52/+36
The previous way of sanitising dynamic keys is going away as we're slowly introducing the new nested key logic. While touching this stuff, I've also changed all the related string types to SmolStr as that is more sensible for identifiers. Change-Id: If30c74151508719d646d0e68e7d6f62c36f4d23f Reviewed-on: https://cl.tvl.fyi/c/depot/+/6779 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
generated by cgit-pink 1.4.1 (git 2.44.2) at 2024-11-09T02·25+0000