From cd8cff204b2627a2c375e3297675a1dbdfb5f46f Mon Sep 17 00:00:00 2001
From: y21 <30553356+y21@users.noreply.github.com>
Date: Tue, 30 Jul 2024 00:48:39 +0200
Subject: [PATCH] clarify declarations/locals and unnameable semantics

---
 crates/dash_compiler/src/lib.rs            | 17 ++++-------------
 crates/dash_middle/src/compiler/scope.rs   |  8 +++++++-
 crates/dash_middle/src/parser/statement.rs |  3 ++-
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/crates/dash_compiler/src/lib.rs b/crates/dash_compiler/src/lib.rs
index f3671e4d..54985e47 100644
--- a/crates/dash_compiler/src/lib.rs
+++ b/crates/dash_compiler/src/lib.rs
@@ -288,13 +288,10 @@ impl<'interner> FunctionCompiler<'interner> {
     fn add_external_to_func(&mut self, func_id: ScopeId, external_id: u16, is_nested_external: bool) -> usize {
         let fun = self.function_stack.iter_mut().rev().find(|f| f.id == func_id);
         let externals = &mut fun.unwrap().externals;
-        if let Some(id) = externals.iter().position(|ext| {
-            ext.id == external_id && {
-                // assert_eq!(ext.is_nested_external, is_nested_external);
-                // true
-                ext.is_nested_external == is_nested_external
-            }
-        }) {
+        if let Some(id) = externals
+            .iter()
+            .position(|ext| ext.id == external_id && ext.is_nested_external == is_nested_external)
+        {
             id
         } else {
             externals.push(External {
@@ -348,7 +345,6 @@ impl<'interner> FunctionCompiler<'interner> {
 
     /// This is the same as `find_local` but should be used
     /// when type_infer must have discovered/registered the local variable in the current scope.
-    /// It would be *wrong* to go up scopes sometimes.
     fn find_local_from_binding(&mut self, binding: Binding) -> u16 {
         self.decl_to_slot.slot_from_local(binding.id)
     }
@@ -1849,11 +1845,6 @@ impl<'interner> Visitor<Result<(), Error>> for FunctionCompiler<'interner> {
         ib.current_function_mut()
             .finally_labels
             .push(finally.as_ref().map(|&(finally_id, _)| Label::Finally { finally_id }));
-        // if true {
-        //     panic!(
-        //         "add a test for ensuring we dont really need to enter a scope here because try_ is already a block stmt"
-        //     );
-        // }
         let res = ib.accept(*try_);
         ib.current_function_mut().try_depth -= 1;
         res?;
diff --git a/crates/dash_middle/src/compiler/scope.rs b/crates/dash_middle/src/compiler/scope.rs
index 6dc43e3b..62615c33 100644
--- a/crates/dash_middle/src/compiler/scope.rs
+++ b/crates/dash_middle/src/compiler/scope.rs
@@ -12,7 +12,10 @@ pub struct LimitExceededError;
 
 #[derive(Debug, Clone)]
 pub struct FunctionScope {
-    // TODO: document the difference between this and `declarations`
+    /// All locals that are declared in this function.
+    /// Every scope that this function encloses (including the function's `Scope` itself) will have its own declarations.
+    /// Some scopes may not have any declarations at all, such as unnameable locals.
+    /// There can also be multiple locals with the same name.
     pub locals: Vec<Local>,
 }
 impl FunctionScope {
@@ -40,6 +43,9 @@ pub struct Scope {
     pub kind: ScopeKind,
     pub parent: Option<ScopeId>,
     pub subscopes: Vec<ScopeId>,
+    /// All variable declarations in this scope.
+    /// It contains the identifier, as well as the local slot
+    /// (index into `functions` of the enclosing function scope).
     pub declarations: Vec<(Symbol, u16)>,
 }
 
diff --git a/crates/dash_middle/src/parser/statement.rs b/crates/dash_middle/src/parser/statement.rs
index 9cccc48c..4ae537ad 100644
--- a/crates/dash_middle/src/parser/statement.rs
+++ b/crates/dash_middle/src/parser/statement.rs
@@ -685,7 +685,8 @@ pub enum VariableDeclarationKind {
 
     /// Unnameable variables cannot be referred to by JavaScript code directly and are created by the compiler
     ///
-    /// TODO: be more detailed about the semantics here
+    /// Multiple unnamed variables of the same name can exist in a function's `locals` vec and all `find` operations
+    /// on the `ScopeGraph` will never consider unnamed variables.
     #[display(fmt = "__intrinsic_var")]
     Unnameable,
 }