Merge pull request #3127 from veryl-lang/fix/drop-dangling-symbol-refs

fix(analyzer): drop scope tree bindings together with their symbols

Author: dalance

crates/analyzer/src/analyzer.rs

@@ -2,6 +2,7 @@ use crate::analyzer_error::{AnalyzerError, ExceedLimitKind};
 use crate::attribute_table;
 use crate::comb_loop_detect;
 use crate::conv::{Context, Conv};
+use crate::definition_table;
 use crate::generic_inference_table;
 use crate::handlers::*;
 use crate::ir::{Ir, IrResult};
@@ -13,10 +14,12 @@ use crate::scope;
 use crate::symbol::{DocComment, ProjectPropertyValueProperty, Symbol, SymbolKind};
 use crate::symbol_table;
 use crate::type_dag;
+use crate::unsafe_table;
 use std::collections::BTreeMap;
 use veryl_metadata::{Build, Lint, Metadata, ProjectProperty};
 use veryl_parser::doc_comment_table;
-use veryl_parser::resource_table::{self, StrId};
+use veryl_parser::resource_table::{self, PathId, StrId};
+use veryl_parser::text_table;
 use veryl_parser::veryl_grammar_trait::*;
 use veryl_parser::veryl_token::Token;
 use veryl_parser::veryl_walker::{Handler, VerylWalker};
@@ -251,6 +254,28 @@ impl Analyzer {
         ret
     }
 
+    /// Removes what one file registered in the global tables, so it can be
+    /// re-analyzed or a partial fragment restore rolled back.
+    ///
+    /// `prj` scopes the drop to one project: the same source can be registered
+    /// under several at once — two dependencies aliasing one path — and each
+    /// registration must only remove what it added. `None` removes all of them,
+    /// which is what a deleted file needs.
+    ///
+    /// `text_table` / `attribute_table` / `unsafe_table` are keyed by position
+    /// rather than by project, so they are dropped whole and re-registered
+    /// identically by the next parse.
+    pub fn drop_file(path: PathId, prj: Option<StrId>) {
+        // Must precede `drop_tokens`: it matches reference tokens against
+        // their token scope.
+        symbol_table::drop(path, prj);
+        scope::drop_tokens(path, prj);
+        text_table::drop(path);
+        attribute_table::drop(path);
+        unsafe_table::drop(path);
+        definition_table::drop(path, prj);
+    }
+
     pub fn clear(&self) {
         attribute_table::clear();
         crate::component_manifest_table::clear();

crates/analyzer/src/scope.rs

@@ -1,4 +1,5 @@
 use crate::HashMap;
+use crate::HashSet;
 use crate::SVec;
 use crate::namespace::{DefineContext, Namespace};
 use crate::symbol::{SymbolId, SymbolKind};
@@ -52,6 +53,14 @@ pub fn scope_kind_of(kind: &SymbolKind) -> Option<ScopeKind> {
     }
 }
 
+/// A symbol whose scope-tree bindings must go away with it.
+pub struct DroppedSymbol {
+    /// Scope the symbol was inserted into (`Symbol::scope`).
+    pub scope: ScopeId,
+    pub name: StrId,
+    pub id: SymbolId,
+}
+
 /// An explicit `import pkg::name` binding local to a scope.
 #[derive(Debug, Clone)]
 pub struct ImportBinding {
@@ -254,6 +263,47 @@ impl ScopeArena {
             .push(symbol);
     }
 
+    /// Removes the tree's own copies of dropped `SymbolId`s. Without this a
+    /// lookup dereferences an id the symbol table no longer holds.
+    fn drop_symbols(&mut self, symbols: &[DroppedSymbol]) {
+        if symbols.is_empty() {
+            return;
+        }
+
+        for symbol in symbols {
+            let name = resource_table::canonical_str_id(symbol.name);
+
+            if let Some(scope) = self.scopes.get_mut(symbol.scope.0 as usize)
+                && let Some(ids) = scope.locals.get_mut(&name)
+            {
+                ids.retain(|x| *x != symbol.id);
+                if ids.is_empty() {
+                    scope.locals.remove(&name);
+                }
+            }
+
+            // The inner scope a symbol opens is the child named after it. The
+            // owner check matters for ifdef-exclusive declarations, which share
+            // that child.
+            if let Some(&owned) = self.intern.get(&(symbol.scope.0, name))
+                && let Some(scope) = self.scopes.get_mut(owned as usize)
+                && scope.owner == Some(symbol.id)
+            {
+                scope.owner = None;
+            }
+        }
+
+        // An import binds the id in the importing file's scope, so there is no
+        // reverse route and this has to scan. `apply_import` re-adds them.
+        let dropped: HashSet<SymbolId> = symbols.iter().map(|x| x.id).collect();
+        for scope in &mut self.scopes {
+            scope.imports.retain(|_, bindings| {
+                bindings.retain(|x| !dropped.contains(&x.symbol));
+                !bindings.is_empty()
+            });
+        }
+    }
+
     fn add_import(
         &mut self,
         scope: ScopeId,
@@ -587,6 +637,10 @@ pub fn add_local(scope: ScopeId, name: StrId, symbol: SymbolId) {
     SCOPE_ARENA.with(|f| f.borrow_mut().add_local(scope, name, symbol))
 }
 
+pub fn drop_symbols(symbols: &[DroppedSymbol]) {
+    SCOPE_ARENA.with(|f| f.borrow_mut().drop_symbols(symbols))
+}
+
 pub fn set_kind_owner(scope: ScopeId, kind: ScopeKind, owner: SymbolId) {
     SCOPE_ARENA.with(|f| f.borrow_mut().set_kind_owner(scope, kind, owner))
 }
@@ -841,6 +895,25 @@ pub fn wildcards_get(scope: ScopeId) -> SVec<WildcardImport> {
     })
 }
 
+/// Every symbol the tree binds, for asserting a drop left no dangling id.
+#[cfg(test)]
+pub(crate) fn bound_symbols() -> Vec<SymbolId> {
+    SCOPE_ARENA.with(|f| {
+        f.borrow()
+            .scopes
+            .iter()
+            .flat_map(|s| {
+                s.locals
+                    .values()
+                    .flatten()
+                    .chain(s.imports.values().flatten().map(|x| &x.symbol))
+                    .chain(s.owner.iter())
+                    .copied()
+            })
+            .collect()
+    })
+}
+
 pub fn mixin_get(scope: ScopeId) -> SVec<Mixin> {
     SCOPE_ARENA.with(|f| {
         f.borrow()
@@ -895,6 +968,46 @@ mod tests {
         assert_eq!(owner_of(a), Some(SymbolId(42)));
     }
 
+    #[test]
+    fn drop_symbols_removes_locals_and_owner() {
+        clear();
+        let prj = intern_child(ScopeId(0), name("prj"), ScopeKind::Project);
+        let owned = intern_child(prj, name("Pkg"), ScopeKind::Package);
+        set_kind_owner(owned, ScopeKind::Package, SymbolId(1));
+        add_local(prj, name("Pkg"), SymbolId(1));
+        add_local(prj, name("Mod"), SymbolId(2));
+
+        drop_symbols(&[DroppedSymbol {
+            scope: prj,
+            name: name("Pkg"),
+            id: SymbolId(1),
+        }]);
+
+        assert!(locals_get(prj, name("Pkg")).is_empty());
+        assert_eq!(owner_of(owned), None);
+        assert_eq!(locals_get(prj, name("Mod")).as_slice(), [SymbolId(2)]);
+    }
+
+    #[test]
+    fn drop_symbols_keeps_an_owner_claimed_by_another_symbol() {
+        clear();
+        let prj = intern_child(ScopeId(0), name("prj"), ScopeKind::Project);
+        let owned = intern_child(prj, name("Pkg"), ScopeKind::Package);
+        // Two ifdef-exclusive declarations share the scope they open.
+        set_kind_owner(owned, ScopeKind::Package, SymbolId(2));
+        add_local(prj, name("Pkg"), SymbolId(1));
+        add_local(prj, name("Pkg"), SymbolId(2));
+
+        drop_symbols(&[DroppedSymbol {
+            scope: prj,
+            name: name("Pkg"),
+            id: SymbolId(1),
+        }]);
+
+        assert_eq!(locals_get(prj, name("Pkg")).as_slice(), [SymbolId(2)]);
+        assert_eq!(owner_of(owned), Some(SymbolId(2)));
+    }
+
     #[test]
     fn intern_namespace_builds_and_dedups_the_chain() {
         clear();

crates/analyzer/src/symbol_table.rs

@@ -889,7 +889,9 @@ impl SymbolTable {
         let mut max_depth = 0;
         let mut found = None;
         for id in candidates {
-            let symbol = self.symbol_table.get(&id).unwrap();
+            let Some(symbol) = self.symbol_table.get(&id) else {
+                continue;
+            };
             let matched = self.match_nested_generic_instance(context, symbol)
                 || (context.scope == symbol.scope
                     && !context
@@ -910,7 +912,9 @@ impl SymbolTable {
                 continue;
             }
             for id in scope::locals_get(mixin.source, name) {
-                let symbol = self.symbol_table.get(&id).unwrap();
+                let Some(symbol) = self.symbol_table.get(&id) else {
+                    continue;
+                };
                 if !symbol
                     .namespace
                     .define_context
@@ -1682,6 +1686,20 @@ impl SymbolTable {
             .map(|x| *x.0)
             .collect();
 
+        // Read while the symbols are still in the table.
+        let dropped: Vec<_> = drop_list
+            .iter()
+            .filter_map(|id| {
+                self.symbol_table
+                    .get(id)
+                    .map(|symbol| scope::DroppedSymbol {
+                        scope: symbol.scope,
+                        name: symbol.token.text,
+                        id: *id,
+                    })
+            })
+            .collect();
+
         for id in &drop_list {
             if let Some(symbol) = self.symbol_table.get(id)
                 && let Some(ids) = self.namespace_index.get_mut(&symbol.namespace.paths)
@@ -1699,6 +1717,8 @@ impl SymbolTable {
         for tokens in self.reference_table.values_mut() {
             tokens.retain(|x| !is_drop_token(x, file_path, prj));
         }
+
+        scope::drop_symbols(&dropped);
     }
 
     pub fn add_reference(&mut self, target: SymbolId, token: &Token) {

crates/analyzer/src/tests.rs

@@ -18388,3 +18388,59 @@ fn partially_driven_output_port() {
         "{errors:?}"
     );
 }
+
+/// The language server drops a file's state and re-analyzes it on every edit.
+/// The scope tree mirrors each insertion, so the drop must take its bindings.
+#[test]
+fn reanalyze_after_drop() {
+    const CODE: &str = r#"package P {
+    const A: u32 = 1;
+}
+package Q::<W: u32> {
+    const B: u32 = W;
+}
+module M {
+    import P::A;
+    const C: u32 = A;
+    const D: u32 = P::A;
+    const E: u32 = Q::<2>::B;
+}
+"#;
+
+    symbol_table::clear();
+    attribute_table::clear();
+    doc_comment_table::clear();
+
+    let metadata = Metadata::create_default("prj").unwrap();
+    let path = std::path::PathBuf::from("reanalyze_after_drop.veryl");
+
+    let analyze = || {
+        let parser = Parser::parse(CODE, &path).unwrap();
+        let analyzer = Analyzer::new(&metadata);
+        let mut context = Context::default();
+        let mut ir = Ir::default();
+        let mut errors = analyzer.analyze_pass1("prj", &parser.veryl);
+        errors.append(&mut Analyzer::analyze_post_pass1());
+        errors.append(&mut analyzer.analyze_pass2(&parser.veryl, &mut context, Some(&mut ir)));
+        errors.append(&mut Analyzer::analyze_post_pass2(&ir));
+        errors
+    };
+
+    let errors = analyze();
+    assert!(errors.is_empty(), "{errors:?}");
+
+    Analyzer::drop_file(
+        veryl_parser::resource_table::insert_path(&path),
+        Some("prj".into()),
+    );
+
+    for id in crate::scope::bound_symbols() {
+        assert!(
+            symbol_table::get(id).is_some(),
+            "scope tree still binds dropped symbol {id:?}"
+        );
+    }
+
+    let errors = analyze();
+    assert!(errors.is_empty(), "{errors:?}");
+}

crates/languageserver/src/incremental.rs

@@ -9,15 +9,13 @@
 
 use std::collections::HashMap;
 use std::fs;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use veryl_analyzer::fragment_cache::{self, Fragment, FragmentWatermark};
-use veryl_analyzer::{
-    attribute_table, definition_table, scope, symbol_table, type_dag, unsafe_table,
-};
+use veryl_analyzer::{Analyzer, scope, type_dag};
 use veryl_cache::Store;
 use veryl_metadata::Metadata;
+use veryl_parser::resource_table;
 use veryl_parser::resource_table::StrId;
-use veryl_parser::{resource_table, text_table};
 use veryl_path::PathSet;
 
 pub struct LsIncremental {
@@ -65,7 +63,7 @@ impl LsIncremental {
         // Clear any stale state from a previous analysis of this file
         // before re-registering it.
         let prj: StrId = path.prj.as_str().into();
-        drop_file_state(prj, &path.src);
+        Analyzer::drop_file(resource_table::insert_path(&path.src), Some(prj));
 
         let is_root = path.prj == self.root_project;
         scope::set_project(prj, is_root);
@@ -77,7 +75,7 @@ impl LsIncremental {
                 true
             }
             Err(_) => {
-                drop_file_state(prj, &path.src);
+                Analyzer::drop_file(resource_table::insert_path(&path.src), Some(prj));
                 false
             }
         }
@@ -142,19 +140,6 @@ fn global_key(metadata: &Metadata) -> Option<String> {
     ]))
 }
 
-/// Removes everything a file may have registered in the global tables
-/// (same set as the server's `drop_tables`).
-fn drop_file_state(prj: StrId, src: &Path) {
-    let path = resource_table::insert_path(src);
-    let prj = Some(prj);
-    symbol_table::drop(path, prj);
-    scope::drop_tokens(path, prj);
-    text_table::drop(path);
-    attribute_table::drop(path);
-    unsafe_table::drop(path);
-    definition_table::drop(path, prj);
-}
-
 /// Per-project stores, so a server handling files from several projects
 /// keeps one store each. Keyed by metadata path.
 #[derive(Default)]
@@ -175,7 +160,7 @@ impl LsIncrementalMap {
 mod tests {
     use super::*;
     use std::thread;
-    use veryl_analyzer::Analyzer;
+    use veryl_analyzer::symbol_table;
     use veryl_parser::Parser;
 
     const FILE_A: &str = "package P { const W: u32 = 8; }\n";