From 2edf66ecdfa31bbae9384b0f8993a0fbe6e0b025 Mon Sep 17 00:00:00 2001 From: Joe Savona Date: Tue, 15 Aug 2023 12:33:45 -0700 Subject: [PATCH] [rust][sema] Resolve all references after traversing program Previously we attempted to resolve each reference at the close of its defining scope, and if it couldn't be resolved yet we bubbled the unresolved reference up to the parent scope. That approach isn't ideal for two reasons: * First, it's inefficient since we may have to make multiple attempts to resolve the same reference. * Second, it's incorrect. There can be cases where we think we can resolve a reference to a value defined in an outer scope, but there is a hoisted declaration from an intermediate scope that we haven't seen yet. The safest and most optimal thing is to just queue all references and resolve them at the end. --- .../forget_semantic_analysis/src/analyzer.rs | 97 ++++--- .../src/scope_manager.rs | 64 +---- ...s_test__fixtures@function-hoisting.js.snap | 6 +- .../analysis_test__fixtures@labels.js.snap | 12 +- ...alysis_test__fixtures@var-hoisting.js.snap | 241 +++++++++--------- 5 files changed, 176 insertions(+), 244 deletions(-) diff --git a/compiler/forget/crates/forget_semantic_analysis/src/analyzer.rs b/compiler/forget/crates/forget_semantic_analysis/src/analyzer.rs index 55972e4015..33ae17bedd 100644 --- a/compiler/forget/crates/forget_semantic_analysis/src/analyzer.rs +++ b/compiler/forget/crates/forget_semantic_analysis/src/analyzer.rs @@ -14,13 +14,23 @@ use crate::{ pub fn analyze(ast: &Program) -> ScopeManager { let mut analyzer = Analyzer::new(ast); analyzer.visit_program(ast); - analyzer.manager + analyzer.complete() } struct Analyzer { manager: ScopeManager, labels: Vec, current: ScopeId, + unresolved: Vec, +} + +#[derive(Debug, Clone)] +pub struct UnresolvedReference { + pub scope: ScopeId, + pub ast: AstNode, + pub name: String, + pub kind: ReferenceKind, + pub range: Option, } impl Analyzer { @@ -32,9 +42,30 @@ impl Analyzer { manager, labels, current, + unresolved: Default::default(), } } + fn complete(mut self) -> ScopeManager { + for reference in self.unresolved { + if let Some(declaration) = self + .manager + .lookup_declaration(reference.scope, &reference.name) + { + let id = + self.manager + .add_reference(reference.scope, reference.kind, declaration.id); + self.manager.node_references.insert(reference.ast, id); + } else { + self.manager.diagnostics.push(Diagnostic::invalid_syntax( + "Undefined variable", + reference.range, + )); + } + } + self.manager + } + fn enter_label(&mut self, id: LabelId, mut f: F) where F: FnMut(&mut Self) -> (), @@ -103,24 +134,7 @@ impl Analyzer { assert_eq!(self.current, id, "Mismatched enter_scope/close_scope"); let scope = self.manager.mut_scope(self.current); let parent = scope.parent.unwrap(); - let unresolved = std::mem::take(&mut scope.unresolved); - drop(scope); self.current = parent; - - // Lookup unresolved nodes from the child scope in the (now-current) parent scope - for reference in unresolved { - if let Some(declaration) = self - .manager - .lookup_declaration(reference.scope, &reference.name) - { - let id = - self.manager - .add_reference(reference.scope, reference.kind, declaration.id); - self.manager.node_references.insert(reference.ast, id); - } else { - self.manager.push_unresolved_reference(parent, reference); - } - } } fn visit_function(&mut self, node: &T) { @@ -168,8 +182,13 @@ impl Analyzer { kind: ReferenceKind, range: Option, ) { - self.manager - .add_unresolved_reference(self.current, ast, name.to_string(), kind, range); + self.unresolved.push(UnresolvedReference { + scope: self.current, + ast, + name: name.to_string(), + kind, + range, + }); } fn visit_declaration_identifier( @@ -199,13 +218,13 @@ impl Analyzer { .insert(AstNode::from(ast), id); } else { // Re-assigning a variable - self.manager.add_unresolved_reference( - self.current, - AstNode::from(ast), - ast.name.to_string(), - ReferenceKind::Write, - ast.range, - ); + self.unresolved.push(UnresolvedReference { + scope: self.current, + ast: AstNode::from(ast), + name: ast.name.to_string(), + kind: ReferenceKind::Write, + range: ast.range, + }); } } @@ -642,30 +661,6 @@ impl Visitor for Analyzer { ) } - fn visit_program(&mut self, ast: &forget_estree::Program) { - for item in &ast.body { - self.visit_module_item(item); - } - let scope = self.manager.mut_scope(self.current); - let unresolved = std::mem::take(&mut scope.unresolved); - for reference in unresolved { - if let Some(declaration) = self - .manager - .lookup_declaration(reference.scope, &reference.name) - { - let id = - self.manager - .add_reference(reference.scope, reference.kind, declaration.id); - self.manager.node_references.insert(reference.ast, id); - } else { - self.manager.diagnostics.push(Diagnostic::invalid_syntax( - "Undefined variable", - reference.range, - )); - } - } - } - fn visit_property(&mut self, ast: &forget_estree::Property) { if ast.is_computed { self.visit_expression(&ast.key); diff --git a/compiler/forget/crates/forget_semantic_analysis/src/scope_manager.rs b/compiler/forget/crates/forget_semantic_analysis/src/scope_manager.rs index 38d7d3e2e6..6a69a6814b 100644 --- a/compiler/forget/crates/forget_semantic_analysis/src/scope_manager.rs +++ b/compiler/forget/crates/forget_semantic_analysis/src/scope_manager.rs @@ -1,6 +1,6 @@ use forget_diagnostics::Diagnostic; use forget_estree::{ - BreakStatement, ContinueStatement, ESTreeNode, LabeledStatement, SourceRange, SourceType, + BreakStatement, ContinueStatement, ESTreeNode, LabeledStatement, SourceType, VariableDeclarationKind, }; use forget_utils::PointerAddress; @@ -48,7 +48,6 @@ impl ScopeManager { declarations: Default::default(), references: Default::default(), children: Default::default(), - unresolved: Default::default(), }], labels: Default::default(), declarations: Default::default(), @@ -202,7 +201,6 @@ impl ScopeManager { declarations: Default::default(), references: Default::default(), children: Default::default(), - unresolved: Default::default(), }); self.scopes[parent.0].children.push(id); id @@ -256,31 +254,14 @@ impl ScopeManager { | DeclarationKind::Const | DeclarationKind::CatchClause | DeclarationKind::For => scope, - DeclarationKind::Var => { - let mut current = scope; - loop { - let scope = self.scope(current); - match scope.kind { - ScopeKind::Function | ScopeKind::Global | ScopeKind::StaticBlock => { - return current; - } - _ => { /* no-op */ } - } - if let Some(parent) = &scope.parent { - current = *parent - } else { - unreachable!("Expected scope without a parent to be a Global scope"); - } - } - } - DeclarationKind::FunctionDeclaration => { + DeclarationKind::Var | DeclarationKind::FunctionDeclaration => { let mut current = scope; loop { let scope = self.scope(current); match scope.kind { ScopeKind::Function - | ScopeKind::Module | ScopeKind::Global + | ScopeKind::Module | ScopeKind::StaticBlock => { return current; } @@ -289,7 +270,9 @@ impl ScopeManager { if let Some(parent) = &scope.parent { current = *parent } else { - unreachable!("Expected scope without a parent to be a Global scope"); + unreachable!( + "Expected scope without a parent to be a Global or Module scope" + ); } } } @@ -312,31 +295,6 @@ impl ScopeManager { self.scopes[scope.0].references.push(id); id } - - pub(crate) fn push_unresolved_reference( - &mut self, - scope: ScopeId, - reference: UnresolvedReference, - ) { - self.scopes[scope.0].unresolved.push(reference); - } - - pub(crate) fn add_unresolved_reference( - &mut self, - scope: ScopeId, - ast: AstNode, - name: String, - kind: ReferenceKind, - range: Option, - ) { - self.scopes[scope.0].unresolved.push(UnresolvedReference { - ast, - scope, - name, - kind, - range, - }); - } } #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone)] @@ -372,16 +330,6 @@ pub struct Scope { pub declarations: IndexMap, pub references: Vec, pub children: Vec, - pub unresolved: Vec, -} - -#[derive(Debug, Clone)] -pub struct UnresolvedReference { - pub scope: ScopeId, - pub ast: AstNode, - pub name: String, - pub kind: ReferenceKind, - pub range: Option, } #[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Copy, Clone)] diff --git a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@function-hoisting.js.snap b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@function-hoisting.js.snap index e479b96fec..abf73efd83 100644 --- a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@function-hoisting.js.snap +++ b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@function-hoisting.js.snap @@ -75,7 +75,7 @@ Scope { references: [ Reference { id: ReferenceId( - 1, + 0, ), kind: Read, declaration: DeclarationId( @@ -88,7 +88,7 @@ Scope { }, Reference { id: ReferenceId( - 2, + 1, ), kind: Read, declaration: DeclarationId( @@ -150,7 +150,7 @@ Scope { references: [ Reference { id: ReferenceId( - 0, + 2, ), kind: Read, declaration: DeclarationId( diff --git a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@labels.js.snap b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@labels.js.snap index 473076fd8e..c945d7994b 100644 --- a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@labels.js.snap +++ b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@labels.js.snap @@ -98,7 +98,7 @@ Scope { references: [ Reference { id: ReferenceId( - 4, + 0, ), kind: Read, declaration: DeclarationId( @@ -111,7 +111,7 @@ Scope { }, Reference { id: ReferenceId( - 5, + 1, ), kind: Read, declaration: DeclarationId( @@ -133,7 +133,7 @@ Scope { references: [ Reference { id: ReferenceId( - 0, + 2, ), kind: Read, declaration: DeclarationId( @@ -146,7 +146,7 @@ Scope { }, Reference { id: ReferenceId( - 1, + 3, ), kind: Write, declaration: DeclarationId( @@ -159,7 +159,7 @@ Scope { }, Reference { id: ReferenceId( - 2, + 4, ), kind: Read, declaration: DeclarationId( @@ -172,7 +172,7 @@ Scope { }, Reference { id: ReferenceId( - 3, + 5, ), kind: Read, declaration: DeclarationId( diff --git a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@var-hoisting.js.snap b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@var-hoisting.js.snap index 74d599a008..56c671d488 100644 --- a/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@var-hoisting.js.snap +++ b/compiler/forget/crates/forget_semantic_analysis/tests/snapshots/analysis_test__fixtures@var-hoisting.js.snap @@ -30,8 +30,17 @@ Scope { id: ScopeId( 0, ), - kind: Global, + kind: Module, declarations: { + "Component": Declaration { + id: DeclarationId( + 0, + ), + kind: FunctionDeclaration, + scope: ScopeId( + 0, + ), + }, "baz": Declaration { id: DeclarationId( 5, @@ -48,19 +57,103 @@ Scope { id: ScopeId( 1, ), - kind: Module, + kind: Function, declarations: { - "Component": Declaration { + "props": Declaration { id: DeclarationId( - 0, + 1, ), kind: FunctionDeclaration, scope: ScopeId( 1, ), }, + "foo": Declaration { + id: DeclarationId( + 2, + ), + kind: FunctionDeclaration, + scope: ScopeId( + 1, + ), + }, + "bar": Declaration { + id: DeclarationId( + 4, + ), + kind: Var, + scope: ScopeId( + 1, + ), + }, }, - references: [], + references: [ + Reference { + id: ReferenceId( + 0, + ), + kind: Read, + declaration: DeclarationId( + 4, + ), + declaration (name): "bar", + scope: ScopeId( + 1, + ), + }, + Reference { + id: ReferenceId( + 1, + ), + kind: Write, + declaration: DeclarationId( + 4, + ), + declaration (name): "bar", + scope: ScopeId( + 1, + ), + }, + Reference { + id: ReferenceId( + 2, + ), + kind: Read, + declaration: DeclarationId( + 5, + ), + declaration (name): "baz", + scope: ScopeId( + 1, + ), + }, + Reference { + id: ReferenceId( + 3, + ), + kind: Write, + declaration: DeclarationId( + 5, + ), + declaration (name): "baz", + scope: ScopeId( + 1, + ), + }, + Reference { + id: ReferenceId( + 7, + ), + kind: Read, + declaration: DeclarationId( + 1, + ), + declaration (name): "props", + scope: ScopeId( + 1, + ), + }, + ], children: [ Scope { id: ScopeId( @@ -68,27 +161,9 @@ Scope { ), kind: Function, declarations: { - "props": Declaration { - id: DeclarationId( - 1, - ), - kind: FunctionDeclaration, - scope: ScopeId( - 2, - ), - }, - "foo": Declaration { - id: DeclarationId( - 2, - ), - kind: FunctionDeclaration, - scope: ScopeId( - 2, - ), - }, "bar": Declaration { id: DeclarationId( - 4, + 3, ), kind: Var, scope: ScopeId( @@ -99,24 +174,11 @@ Scope { references: [ Reference { id: ReferenceId( - 3, + 4, ), kind: Read, declaration: DeclarationId( - 4, - ), - declaration (name): "bar", - scope: ScopeId( - 2, - ), - }, - Reference { - id: ReferenceId( - 4, - ), - kind: Write, - declaration: DeclarationId( - 4, + 3, ), declaration (name): "bar", scope: ScopeId( @@ -127,11 +189,11 @@ Scope { id: ReferenceId( 5, ), - kind: Read, + kind: Write, declaration: DeclarationId( - 1, + 3, ), - declaration (name): "props", + declaration (name): "bar", scope: ScopeId( 2, ), @@ -142,22 +204,9 @@ Scope { ), kind: Read, declaration: DeclarationId( - 5, + 1, ), - declaration (name): "baz", - scope: ScopeId( - 2, - ), - }, - Reference { - id: ReferenceId( - 7, - ), - kind: Write, - declaration: DeclarationId( - 5, - ), - declaration (name): "baz", + declaration (name): "props", scope: ScopeId( 2, ), @@ -168,75 +217,6 @@ Scope { id: ScopeId( 3, ), - kind: Function, - declarations: { - "bar": Declaration { - id: DeclarationId( - 3, - ), - kind: Var, - scope: ScopeId( - 3, - ), - }, - }, - references: [ - Reference { - id: ReferenceId( - 0, - ), - kind: Read, - declaration: DeclarationId( - 3, - ), - declaration (name): "bar", - scope: ScopeId( - 3, - ), - }, - Reference { - id: ReferenceId( - 1, - ), - kind: Write, - declaration: DeclarationId( - 3, - ), - declaration (name): "bar", - scope: ScopeId( - 3, - ), - }, - Reference { - id: ReferenceId( - 2, - ), - kind: Read, - declaration: DeclarationId( - 1, - ), - declaration (name): "props", - scope: ScopeId( - 3, - ), - }, - ], - children: [ - Scope { - id: ScopeId( - 4, - ), - kind: Block, - declarations: {}, - references: [], - children: [], - }, - ], - }, - Scope { - id: ScopeId( - 5, - ), kind: Block, declarations: {}, references: [], @@ -244,6 +224,15 @@ Scope { }, ], }, + Scope { + id: ScopeId( + 4, + ), + kind: Block, + declarations: {}, + references: [], + children: [], + }, ], }, ],