mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(tvix/eval): don't mark locals as used in resolve_local

Browse source 
resolve_local isn't exclusively used to compile variable access, so we
shouldn't automatically mark a local as used when it's called.
Specifically, the optimiser would call `is_user_defined`, causing locals
to be marked as used even when they weren't, erroneously silencing the
unused local variable warning.

For resolve_upvalue I've opted not to do the same, instead renaming the
function to make its behavior clearer. The reason for this is that
resolve_upvalue is only used in the code for the purpose of compiling
variable access and mark_used for upvalues would be needlessly
expensive, as it requires recursing through enclosing contexts.

Supersedes / alternative to cl/12708.

Change-Id: Ib4a4a9dfcecf710ab2766b03d0a0cc09245c2d0a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/12869
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This commit is contained in:
sterni 2024-12-05 14:01:26 +01:00
parent 7ea442b21a
commit bd4da5f444
2 changed files with 23 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

27
tvix/eval/src/compiler/bindings.rs
View file

@ -716,7 +716,7 @@ impl Compiler<'_, '_> {
match self.scope_mut().resolve_local(ident) {
LocalPosition::Unknown => {
// Are we possibly dealing with an upvalue?
if let Some(idx) = self.resolve_upvalue(self.contexts.len() - 1, ident) {
if let Some(idx) = self.resolve_upvalue_for_use(self.contexts.len() - 1, ident) {
self.push_op(Op::GetUpvalue, node);
self.push_uvarint(idx.0 as u64);
return;
@ -751,6 +751,8 @@ impl Compiler<'_, '_> {
}
LocalPosition::Known(idx) => {
self.scope_mut().mark_used(idx);
let stack_idx = self.scope().stack_index(idx);
self.push_op(Op::GetLocal, node);
self.push_uvarint(stack_idx.0 as u64);
@ -758,12 +760,15 @@ impl Compiler<'_, '_> {
// This identifier is referring to a value from the same scope which
// is not yet defined. This identifier access must be thunked.
LocalPosition::Recursive(idx) => self.thunk(slot, node, move |compiler, _| {
let upvalue_idx =
compiler.add_upvalue(compiler.contexts.len() - 1, UpvalueKind::Local(idx));
compiler.push_op(Op::GetUpvalue, node);
compiler.push_uvarint(upvalue_idx.0 as u64);
}),
LocalPosition::Recursive(idx) => {
self.scope_mut().mark_used(idx);
self.thunk(slot, node, move |compiler, _| {
let upvalue_idx =
compiler.add_upvalue(compiler.contexts.len() - 1, UpvalueKind::Local(idx));
compiler.push_op(Op::GetUpvalue, node);
compiler.push_uvarint(upvalue_idx.0 as u64);
})
}
};
}
@ -775,7 +780,8 @@ impl Compiler<'_, '_> {
/// Private compiler helpers related to bindings.
impl Compiler<'_, '_> {
fn resolve_upvalue(&mut self, ctx_idx: usize, name: &str) -> Option<UpvalueIdx> {
// ATTN: Also marks local backing the upvalue as used if any
fn resolve_upvalue_for_use(&mut self, ctx_idx: usize, name: &str) -> Option<UpvalueIdx> {
if ctx_idx == 0 {
// There can not be any upvalue at the outermost context.
return None;
@ -788,7 +794,8 @@ impl Compiler<'_, '_> {
// stack (i.e. in the right position) *during* their runtime
// construction
LocalPosition::Known(idx) | LocalPosition::Recursive(idx) => {
return Some(self.add_upvalue(ctx_idx, UpvalueKind::Local(idx)))
self.contexts[ctx_idx - 1].scope.mark_used(idx);
return Some(self.add_upvalue(ctx_idx, UpvalueKind::Local(idx)));
}
LocalPosition::Unknown => { /* continue below */ }
@ -796,7 +803,7 @@ impl Compiler<'_, '_> {
// If the upvalue comes from even further up, we need to recurse to make
// sure that the upvalues are created at each level.
if let Some(idx) = self.resolve_upvalue(ctx_idx - 1, name) {
if let Some(idx) = self.resolve_upvalue_for_use(ctx_idx - 1, name) {
return Some(self.add_upvalue(ctx_idx, UpvalueKind::Upvalue(idx)));
}

13
tvix/eval/src/compiler/scope.rs
View file

@ -211,15 +211,10 @@ impl Scope {
}
/// Resolve the stack index of a statically known local.
pub fn resolve_local(&mut self, name: &str) -> LocalPosition {
pub fn resolve_local(&self, name: &str) -> LocalPosition {
if let Some(by_name) = self.by_name.get(name) {
let idx = by_name.index();
let local = self
.locals
.get_mut(idx.0)
.expect("invalid compiler state: indexed local missing");
local.used = true;
let local = &self.locals[idx.0];
// This local is still being initialised, meaning that
// we know its final runtime stack position, but it is
@ -309,6 +304,10 @@ impl Scope {
self.locals[idx.0].initialised = true;
}
pub fn mark_used(&mut self, idx: LocalIdx) {
self.locals[idx.0].used = true;
}
/// Mark local as needing a finaliser.
pub fn mark_needs_finaliser(&mut self, idx: LocalIdx) {
self.locals[idx.0].needs_finaliser = true;