From 9973ddfcba57e1a2d1a72351814210ab652bd860 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sat, 3 Sep 2022 00:26:32 +0300 Subject: refactor(tvix/eval): refactor locals to use an enum for phantoms Instead of using sentinel values and an additional bool, this tracks the identifier of a local as an enum that is either a statically known name, or a phantom. To make this work correctly some more locals related logic has been encapsulated in the `scope` module, which is a good thing (that's the goal). Phantom values are now not initialised by default, but the only current call site of phantoms (`with` expression compilation) performs the initialisation right away. This commit changes no actual functionality right now, but paves the way for fixing an issue related to `let` bodies. Change-Id: I679f93a59a4daeacfe40f4012263cfb7bc05034e Reviewed-on: https://cl.tvl.fyi/c/depot/+/6421 Reviewed-by: sterni Tested-by: BuildkiteCI --- tvix/eval/src/compiler/mod.rs | 20 ++++++++++------- tvix/eval/src/compiler/scope.rs | 50 +++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 22 deletions(-) (limited to 'tvix/eval/src/compiler') diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index 242001c81f..d02c0e69c5 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -29,7 +29,7 @@ use crate::opcode::{CodeIdx, Count, JumpOffset, OpCode, UpvalueIdx}; use crate::value::{Closure, Lambda, Thunk, Value}; use crate::warnings::{EvalWarning, WarningKind}; -use self::scope::{Local, LocalIdx, LocalPosition, Scope, Upvalue, UpvalueKind}; +use self::scope::{LocalIdx, LocalPosition, Scope, Upvalue, UpvalueKind}; /// Represents the result of compiling a piece of Nix code. If /// compilation was successful, the resulting bytecode can be passed @@ -842,7 +842,14 @@ impl Compiler<'_> { self.emit_force(&node.namespace().unwrap()); let span = self.span_for(&node.namespace().unwrap()); + + // The attribute set from which `with` inherits values + // occupies a slot on the stack, but this stack slot is not + // directly accessible. As it must be accounted for to + // calculate correct offsets, what we call a "phantom" local + // is declared here. let local_idx = self.scope_mut().declare_phantom(span); + self.scope_mut().mark_initialised(local_idx); let with_idx = self.scope().stack_index(local_idx); self.scope_mut().push_with(); @@ -1058,12 +1065,9 @@ impl Compiler<'_> { // While removing the local, analyse whether it has been // accessed while it existed and emit a warning to the // user otherwise. - if let Some(Local { - span, used, name, .. - }) = self.scope_mut().locals.pop() - { - if !used && !name.starts_with('_') { - self.emit_warning(span, WarningKind::UnusedBinding); + if let Some(local) = self.scope_mut().locals.pop() { + if !local.used && !local.is_ignored() { + self.emit_warning(local.span, WarningKind::UnusedBinding); } } } @@ -1106,7 +1110,7 @@ impl Compiler<'_> { let mut shadowed = false; for other in self.scope().locals.iter().rev() { - if other.name == name && other.depth == depth { + if other.has_name(&name) && other.depth == depth { shadowed = true; break; } diff --git a/tvix/eval/src/compiler/scope.rs b/tvix/eval/src/compiler/scope.rs index 7d557a7d2f..78012add40 100644 --- a/tvix/eval/src/compiler/scope.rs +++ b/tvix/eval/src/compiler/scope.rs @@ -19,13 +19,23 @@ use smol_str::SmolStr; use crate::opcode::{StackIdx, UpvalueIdx}; +#[derive(Debug)] +enum LocalName { + /// Normally declared local with a statically known name. + Ident(String), + + /// Phantom stack value (e.g. attribute set used for `with`) that + /// must be accounted for to calculate correct stack offsets. + Phantom, +} + /// Represents a single local already known to the compiler. #[derive(Debug)] pub struct Local { - // Definition name, which can be different kinds of tokens (plain - // string or identifier). Nix does not allow dynamic names inside - // of `let`-expressions. - pub name: String, + // Identifier of this local. This is always a statically known + // value (Nix does not allow dynamic identifier names in locals), + // or a "phantom" value not accessible by users. + name: LocalName, // Source span at which this local was declared. pub span: codemap::Span, @@ -36,10 +46,6 @@ pub struct Local { // Is this local initialised? pub initialised: bool, - // Phantom locals are not actually accessible by users (e.g. - // intermediate values used for `with`). - pub phantom: bool, - // Is this local known to have been used at all? pub used: bool, @@ -53,6 +59,24 @@ impl Local { pub fn above(&self, theirs: usize) -> bool { self.depth > theirs } + + /// Does the name of this local match the given string? + pub fn has_name(&self, other: &str) -> bool { + match &self.name { + LocalName::Ident(name) => name == other, + + // Phantoms are *never* accessible by a name. + LocalName::Phantom => false, + } + } + + /// Is this local intentionally ignored? (i.e. name starts with `_`) + pub fn is_ignored(&self) -> bool { + match &self.name { + LocalName::Ident(name) => name.starts_with('_'), + LocalName::Phantom => false, + } + } } /// Represents the current position of a local as resolved in a scope. @@ -196,7 +220,7 @@ impl Scope { /// Resolve the stack index of a statically known local. pub fn resolve_local(&mut self, name: &str) -> LocalPosition { for (idx, local) in self.locals.iter_mut().enumerate().rev() { - if !local.phantom && local.name == name { + if local.has_name(name) { local.used = true; // This local is still being initialised, meaning that @@ -219,12 +243,11 @@ impl Scope { pub fn declare_phantom(&mut self, span: codemap::Span) -> LocalIdx { let idx = self.locals.len(); self.locals.push(Local { + name: LocalName::Phantom, span, depth: self.scope_depth, - initialised: true, + initialised: false, needs_finaliser: false, - name: "".into(), - phantom: true, used: true, }); @@ -235,12 +258,11 @@ impl Scope { pub fn declare_local(&mut self, name: String, span: codemap::Span) -> LocalIdx { let idx = self.locals.len(); self.locals.push(Local { - name, + name: LocalName::Ident(name), span, depth: self.scope_depth, initialised: false, needs_finaliser: false, - phantom: false, used: false, }); -- cgit 1.4.1