Refactor `Scope` (#5797)

6 files changed, 61 insertions, 48 deletions

diff --git a/crates/typst-eval/src/access.rs b/crates/typst-eval/src/access.rs
index 9bcac4d6..22a6b7f3 100644
--- a/crates/typst-eval/src/access.rs
+++ b/crates/typst-eval/src/access.rs
@@ -30,12 +30,14 @@ impl Access for ast::Ident<'_> {
     fn access<'a>(self, vm: &'a mut Vm) -> SourceResult<&'a mut Value> {
         let span = self.span();
         if vm.inspected == Some(span) {
-            if let Ok(value) = vm.scopes.get(&self).cloned() {
-                vm.trace(value);
+            if let Ok(binding) = vm.scopes.get(&self) {
+                vm.trace(binding.read().clone());
             }
         }
-        let value = vm.scopes.get_mut(&self).at(span)?;
-        Ok(value)
+        vm.scopes
+            .get_mut(&self)
+            .and_then(|b| b.write().map_err(Into::into))
+            .at(span)
     }
 }
 
diff --git a/crates/typst-eval/src/call.rs b/crates/typst-eval/src/call.rs
index 2a2223e1..6f0ec1fc 100644
--- a/crates/typst-eval/src/call.rs
+++ b/crates/typst-eval/src/call.rs
@@ -6,8 +6,8 @@ use typst_library::diag::{
 };
 use typst_library::engine::{Engine, Sink, Traced};
 use typst_library::foundations::{
-    Arg, Args, Capturer, Closure, Content, Context, Func, NativeElement, Scope, Scopes,
-    SymbolElem, Value,
+    Arg, Args, Binding, Capturer, Closure, Content, Context, Func, NativeElement, Scope,
+    Scopes, SymbolElem, Value,
 };
 use typst_library::introspection::Introspector;
 use typst_library::math::LrElem;
@@ -196,7 +196,7 @@ pub fn eval_closure(
 
     // Provide the closure itself for recursive calls.
     if let Some(name) = name {
-        vm.define(name, Value::Func(func.clone()));
+        vm.define(name, func.clone());
     }
 
     let num_pos_args = args.to_pos().len();
@@ -317,11 +317,11 @@ fn eval_field_call(
 
     if let Some(callee) = target.ty().scope().get(&field) {
         args.insert(0, target_expr.span(), target);
-        Ok(FieldCall::Normal(callee.clone(), args))
+        Ok(FieldCall::Normal(callee.read().clone(), args))
     } else if let Value::Content(content) = &target {
         if let Some(callee) = content.elem().scope().get(&field) {
             args.insert(0, target_expr.span(), target);
-            Ok(FieldCall::Normal(callee.clone(), args))
+            Ok(FieldCall::Normal(callee.read().clone(), args))
         } else {
             bail!(missing_field_call_error(target, field))
         }
@@ -458,11 +458,9 @@ impl<'a> CapturesVisitor<'a> {
             // Identifiers that shouldn't count as captures because they
             // actually bind a new name are handled below (individually through
             // the expressions that contain them).
-            Some(ast::Expr::Ident(ident)) => {
-                self.capture(ident.get(), ident.span(), Scopes::get)
-            }
+            Some(ast::Expr::Ident(ident)) => self.capture(ident.get(), Scopes::get),
             Some(ast::Expr::MathIdent(ident)) => {
-                self.capture(ident.get(), ident.span(), Scopes::get_in_math)
+                self.capture(ident.get(), Scopes::get_in_math)
             }
 
             // Code and content blocks create a scope.
@@ -570,32 +568,34 @@ impl<'a> CapturesVisitor<'a> {
 
     /// Bind a new internal variable.
     fn bind(&mut self, ident: ast::Ident) {
-        self.internal.top.define_ident(ident, Value::None);
+        // The concrete value does not matter as we only use the scoping
+        // mechanism of `Scopes`, not the values themselves.
+        self.internal
+            .top
+            .bind(ident.get().clone(), Binding::detached(Value::None));
     }
 
     /// Capture a variable if it isn't internal.
     fn capture(
         &mut self,
         ident: &EcoString,
-        span: Span,
-        getter: impl FnOnce(&'a Scopes<'a>, &str) -> HintedStrResult<&'a Value>,
+        getter: impl FnOnce(&'a Scopes<'a>, &str) -> HintedStrResult<&'a Binding>,
     ) {
-        if self.internal.get(ident).is_err() {
-            let Some(value) = self
-                .external
-                .map(|external| getter(external, ident).ok())
-                .unwrap_or(Some(&Value::None))
-            else {
-                return;
-            };
-
-            self.captures.define_captured(
-                ident.clone(),
-                value.clone(),
-                self.capturer,
-                span,
-            );
+        if self.internal.get(ident).is_ok() {
+            return;
         }
+
+        let binding = match self.external {
+            Some(external) => match getter(external, ident) {
+                Ok(binding) => binding.capture(self.capturer),
+                Err(_) => return,
+            },
+            // The external scopes are only `None` when we are doing IDE capture
+            // analysis, in which case the concrete value doesn't matter.
+            None => Binding::detached(Value::None),
+        };
+
+        self.captures.bind(ident.clone(), binding);
     }
 }
 
diff --git a/crates/typst-eval/src/code.rs b/crates/typst-eval/src/code.rs
index 2baf4ea9..4ac48186 100644
--- a/crates/typst-eval/src/code.rs
+++ b/crates/typst-eval/src/code.rs
@@ -154,7 +154,7 @@ impl Eval for ast::Ident<'_> {
     type Output = Value;
 
     fn eval(self, vm: &mut Vm) -> SourceResult<Self::Output> {
-        vm.scopes.get(&self).cloned().at(self.span())
+        Ok(vm.scopes.get(&self).at(self.span())?.read().clone())
     }
 }
 
diff --git a/crates/typst-eval/src/import.rs b/crates/typst-eval/src/import.rs
index 2bbc7e41..27b06af4 100644
--- a/crates/typst-eval/src/import.rs
+++ b/crates/typst-eval/src/import.rs
@@ -4,7 +4,7 @@ use typst_library::diag::{
     bail, error, warning, At, FileError, SourceResult, Trace, Tracepoint,
 };
 use typst_library::engine::Engine;
-use typst_library::foundations::{Content, Module, Value};
+use typst_library::foundations::{Binding, Content, Module, Value};
 use typst_library::World;
 use typst_syntax::ast::{self, AstNode, BareImportError};
 use typst_syntax::package::{PackageManifest, PackageSpec};
@@ -43,7 +43,7 @@ impl Eval for ast::ModuleImport<'_> {
             }
         }
 
-        // Source itself is imported if there is no import list or a rename.
+        // If there is a rename, import the source itself under that name.
         let bare_name = self.bare_name();
         let new_name = self.new_name();
         if let Some(new_name) = new_name {
@@ -57,8 +57,7 @@ impl Eval for ast::ModuleImport<'_> {
                 }
             }
 
-            // Define renamed module on the scope.
-            vm.scopes.top.define_ident(new_name, source.clone());
+            vm.define(new_name, source.clone());
         }
 
         let scope = source.scope().unwrap();
@@ -76,7 +75,7 @@ impl Eval for ast::ModuleImport<'_> {
                                     "this import has no effect",
                                 ));
                             }
-                            vm.scopes.top.define_spanned(name, source, source_span);
+                            vm.scopes.top.bind(name, Binding::new(source, source_span));
                         }
                         Ok(_) | Err(BareImportError::Dynamic) => bail!(
                             source_span, "dynamic import requires an explicit name";
@@ -92,8 +91,8 @@ impl Eval for ast::ModuleImport<'_> {
                 }
             }
             Some(ast::Imports::Wildcard) => {
-                for (var, value, span) in scope.iter() {
-                    vm.scopes.top.define_spanned(var.clone(), value.clone(), span);
+                for (var, binding) in scope.iter() {
+                    vm.scopes.top.bind(var.clone(), binding.clone());
                 }
             }
             Some(ast::Imports::Items(items)) => {
@@ -103,7 +102,7 @@ impl Eval for ast::ModuleImport<'_> {
                     let mut scope = scope;
 
                     while let Some(component) = &path.next() {
-                        let Some(value) = scope.get(component) else {
+                        let Some(binding) = scope.get(component) else {
                             errors.push(error!(component.span(), "unresolved import"));
                             break;
                         };
@@ -111,6 +110,7 @@ impl Eval for ast::ModuleImport<'_> {
                         if path.peek().is_some() {
                             // Nested import, as this is not the last component.
                             // This must be a submodule.
+                            let value = binding.read();
                             let Some(submodule) = value.scope() else {
                                 let error = if matches!(value, Value::Func(function) if function.scope().is_none())
                                 {
@@ -153,7 +153,7 @@ impl Eval for ast::ModuleImport<'_> {
                                 }
                             }
 
-                            vm.define(item.bound_name(), value.clone());
+                            vm.bind(item.bound_name(), binding.clone());
                         }
                     }
                 }
diff --git a/crates/typst-eval/src/math.rs b/crates/typst-eval/src/math.rs
index bfb54aa8..23b293f2 100644
--- a/crates/typst-eval/src/math.rs
+++ b/crates/typst-eval/src/math.rs
@@ -35,7 +35,7 @@ impl Eval for ast::MathIdent<'_> {
     type Output = Value;
 
     fn eval(self, vm: &mut Vm) -> SourceResult<Self::Output> {
-        vm.scopes.get_in_math(&self).cloned().at(self.span())
+        Ok(vm.scopes.get_in_math(&self).at(self.span())?.read().clone())
     }
 }
 
diff --git a/crates/typst-eval/src/vm.rs b/crates/typst-eval/src/vm.rs
index a5cbb6fa..52cfb4b5 100644
--- a/crates/typst-eval/src/vm.rs
+++ b/crates/typst-eval/src/vm.rs
@@ -1,7 +1,7 @@
 use comemo::Tracked;
 use typst_library::diag::warning;
 use typst_library::engine::Engine;
-use typst_library::foundations::{Context, IntoValue, Scopes, Value};
+use typst_library::foundations::{Binding, Context, IntoValue, Scopes, Value};
 use typst_library::World;
 use typst_syntax::ast::{self, AstNode};
 use typst_syntax::Span;
@@ -42,13 +42,23 @@ impl<'a> Vm<'a> {
         self.engine.world
     }
 
-    /// Define a variable in the current scope.
+    /// Bind a value to an identifier.
+    ///
+    /// This will create a [`Binding`] with the value and the identifier's span.
     pub fn define(&mut self, var: ast::Ident, value: impl IntoValue) {
-        let value = value.into_value();
+        self.bind(var, Binding::new(value, var.span()));
+    }
+
+    /// Insert a binding into the current scope.
+    ///
+    /// This will insert the value into the top-most scope and make it available
+    /// for dynamic tracing, assisting IDE functionality.
+    pub fn bind(&mut self, var: ast::Ident, binding: Binding) {
         if self.inspected == Some(var.span()) {
-            self.trace(value.clone());
+            self.trace(binding.read().clone());
         }
-        // This will become an error in the parser if 'is' becomes a keyword.
+
+        // This will become an error in the parser if `is` becomes a keyword.
         if var.get() == "is" {
             self.engine.sink.warn(warning!(
                 var.span(),
@@ -58,7 +68,8 @@ impl<'a> Vm<'a> {
                 hint: "try `is_` instead"
             ));
         }
-        self.scopes.top.define_ident(var, value);
+
+        self.scopes.top.bind(var.get().clone(), binding);
     }
 
     /// Trace a value.