fix: make Scope's unique_id truly unique and add a getter for it (#4527)

Right now, `Scope::unique_id` is always the same as the parent Scope's
`index + 1`. This means that if two different scopes are created as
children of the same scope, they will have the same `unique_id`, belying
its name.

In this PR, add a new `ScopeContext` struct that all Scopes in an AST
share; this new struct contains a container which controls the
identifiers that are given out to scopes in the tree.

Also make the field accessible via a getter function, as well as on
`BindingLocator`, so that `BindingLocator` and `Scope` can be
correlated.

Relates to #4519.

Author: rix0rrr

core/ast/src/scope.rs

@@ -105,6 +105,8 @@ pub(crate) struct Inner {
     function: bool,
     // Has the `this` been accessed/escaped outside the function environment boundary.
     this_escaped: Cell<bool>,
+
+    context: Rc<ScopeContext>,
 }
 
 impl Scope {
@@ -119,22 +121,23 @@ impl Scope {
                 bindings: RefCell::default(),
                 function: true,
                 this_escaped: Cell::new(false),
+                context: Rc::default(),
             }),
         }
     }
 
     /// Creates a new scope.
     #[must_use]
     pub fn new(parent: Self, function: bool) -> Self {
-        let index = parent.inner.index.get() + 1;
         Self {
             inner: Rc::new(Inner {
-                unique_id: index,
-                outer: Some(parent),
-                index: Cell::new(index),
+                unique_id: parent.inner.context.next_unique_id(),
+                index: Cell::new(parent.inner.index.get() + 1),
                 bindings: RefCell::default(),
                 function,
                 this_escaped: Cell::new(false),
+                context: parent.inner.context.clone(),
+                outer: Some(parent),
             }),
         }
     }
@@ -494,6 +497,33 @@ impl Scope {
     pub fn outer(&self) -> Option<Self> {
         self.inner.outer.clone()
     }
+
+    /// Returns the unique ID of this scope.
+    #[must_use]
+    pub fn unique_id(&self) -> u32 {
+        self.inner.unique_id
+    }
+}
+
+/// Additional state that all Scopes of a single AST share for bookkeeping.
+#[derive(Debug, PartialEq, Default)]
+struct ScopeContext {
+    /// A counter for unique IDs for Scopes generated as part of this scope tree.
+    ///
+    /// The value is the highest unique ID assigned so far (initialized with 0
+    /// which is also the unique ID of the global scope).
+    ///
+    /// Only used in the root scope.
+    unique_id_ctr: Cell<u32>,
+}
+
+impl ScopeContext {
+    /// Returns the next unique ID for a scope in this scope tree.
+    fn next_unique_id(&self) -> u32 {
+        let id = self.unique_id_ctr.get() + 1;
+        self.unique_id_ctr.set(id);
+        id
+    }
 }
 
 /// A reference to an identifier in a scope.
@@ -624,6 +654,12 @@ impl BindingLocator {
     pub fn set_binding_index(&mut self, index: u32) {
         self.binding_index = index;
     }
+
+    /// Returns the unique scope ID of the binding.
+    #[must_use]
+    pub fn unique_scope_id(&self) -> u32 {
+        self.unique_scope_id
+    }
 }
 
 /// Action that is returned when a fallible binding operation.
@@ -637,7 +673,7 @@ pub enum BindingLocatorError {
 }
 
 /// The scope in which a binding is located.
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum BindingLocatorScope {
     /// The binding is located on the global object.
     GlobalObject,

core/ast/tests/scope.rs

@@ -279,3 +279,37 @@ fn script_function_mapped_arguments_accessed() {
     assert!(a.is_lexical());
     assert!(a.local());
 }
+
+#[test]
+fn multiple_scopes_with_same_parent_have_distinct_ids() {
+    let global = Scope::new_global();
+    let child1 = Scope::new(global.clone(), false);
+    let child2 = Scope::new(global.clone(), false);
+    let child3 = Scope::new(child1.clone(), false);
+    let child4 = Scope::new(child1.clone(), false);
+
+    // Check uniqueness by inserting in a set and asserting that there are
+    // 5 elements numbered from 0 to 4.
+    let mut ids = [global, child1, child2, child3, child4]
+        .into_iter()
+        .map(|scope| scope.unique_id())
+        .collect::<Vec<u32>>();
+    ids.sort_unstable();
+
+    assert_eq!(ids, vec![0, 1, 2, 3, 4]);
+}
+
+#[test]
+fn can_correlate_binding_to_scope() {
+    // GIVEN
+    let global = Scope::new_global();
+    let child1 = Scope::new(global.clone(), false);
+    let child2 = Scope::new(child1.clone(), false);
+    let _unused = child1.create_mutable_binding("x".into(), false);
+
+    // WHEN
+    let binding = child2.get_identifier_reference("x".into());
+
+    // THEN
+    assert_eq!(binding.locator().unique_scope_id(), child1.unique_id());
+}