From 08f0c8f1078d31ec5764532dccfeec84146560f1 Mon Sep 17 00:00:00 2001 From: Victorien Elvinger Date: Wed, 10 Jul 2024 18:26:55 +0200 Subject: [PATCH] refactor(js_semantic): use range start to identify the declaration in read/write (#3404) --- crates/biome_js_semantic/src/events.rs | 56 ++++++++------- .../src/semantic_model/builder.rs | 22 +++--- .../biome_js_semantic/src/tests/assertions.rs | 72 +++++++++++-------- xtask/coverage/src/symbols/msts.rs | 24 +++---- 4 files changed, 95 insertions(+), 79 deletions(-) diff --git a/crates/biome_js_semantic/src/events.rs b/crates/biome_js_semantic/src/events.rs index 41ae8d6fce75..cef9cfc99038 100644 --- a/crates/biome_js_semantic/src/events.rs +++ b/crates/biome_js_semantic/src/events.rs @@ -6,6 +6,7 @@ use biome_js_syntax::{ TsTypeParameterName, }; use biome_js_syntax::{AnyJsImportClause, AnyJsNamedImportSpecifier, AnyTsType}; +use biome_rowan::TextSize; use biome_rowan::{syntax::Preorder, AstNode, SyntaxNodeOptionExt, TokenText}; use rustc_hash::FxHashMap; use std::collections::VecDeque; @@ -33,7 +34,7 @@ pub enum SemanticEvent { /// - All reference identifiers Read { range: TextRange, - declared_at: TextRange, + declaration_at: TextSize, scope_id: u32, }, @@ -42,7 +43,7 @@ pub enum SemanticEvent { /// - All reference identifiers HoistedRead { range: TextRange, - declared_at: TextRange, + declaration_at: TextSize, scope_id: u32, }, @@ -51,7 +52,7 @@ pub enum SemanticEvent { /// - All identifier assignments Write { range: TextRange, - declared_at: TextRange, + declaration_at: TextSize, scope_id: u32, }, @@ -61,7 +62,7 @@ pub enum SemanticEvent { /// - All identifier assignments HoistedWrite { range: TextRange, - declared_at: TextRange, + declaration_at: TextSize, scope_id: u32, }, @@ -94,7 +95,10 @@ pub enum SemanticEvent { /// Tracks where a symbol is exported. /// The range points to the binding that is being exported. - Exported { range: TextRange }, + Export { + range: TextRange, + declaration_at: TextSize, + }, } impl SemanticEvent { @@ -108,7 +112,7 @@ impl SemanticEvent { | Self::Write { range, .. } | Self::HoistedWrite { range, .. } | Self::UnresolvedReference { range, .. } - | Self::Exported { range } => *range, + | Self::Export { range, .. } => *range, } } } @@ -537,14 +541,16 @@ impl SemanticEventExtractor { false }; let scope_id = self.current_scope_mut().scope_id; + let range = node.syntax().text_trimmed_range(); self.stash.push_back(SemanticEvent::DeclarationFound { scope_id, hoisted_scope_id, - range: node.syntax().text_trimmed_range(), + range, }); if is_exported { - self.stash.push_back(SemanticEvent::Exported { - range: node.syntax().text_trimmed_range(), + self.stash.push_back(SemanticEvent::Export { + range, + declaration_at: range.start(), }); } } @@ -804,28 +810,30 @@ impl SemanticEventExtractor { // Bind references to declarations for (name, mut references) in scope.references { if let Some(&BindingInfo { - range: declared_at, + range: declaration_range, declaration_kind, }) = self.bindings.get(&name) { + let declaration_at = declaration_range.start(); // We know the declaration of these reference. for reference in references { - let declaration_before_reference = - declared_at.start() < reference.range().start(); + let declaration_before_reference = declaration_at < reference.range().start(); let event = match reference { Reference::Export(range) => { - self.stash - .push_back(SemanticEvent::Exported { range: declared_at }); + self.stash.push_back(SemanticEvent::Export { + range, + declaration_at, + }); if declaration_before_reference { SemanticEvent::Read { range, - declared_at, + declaration_at, scope_id, } } else { SemanticEvent::HoistedRead { range, - declared_at, + declaration_at, scope_id, } } @@ -848,13 +856,13 @@ impl SemanticEventExtractor { if declaration_before_reference { SemanticEvent::Read { range, - declared_at, + declaration_at, scope_id, } } else { SemanticEvent::HoistedRead { range, - declared_at, + declaration_at, scope_id, } } @@ -863,13 +871,13 @@ impl SemanticEventExtractor { if declaration_before_reference { SemanticEvent::Write { range, - declared_at, + declaration_at, scope_id, } } else { SemanticEvent::HoistedWrite { range, - declared_at, + declaration_at, scope_id, } } @@ -898,19 +906,19 @@ impl SemanticEventExtractor { Reference::AmbientRead(range) if info.is_imported() => { // An ambient read can only read a value, // but also an imported value as a type (with the `type` modifier) - let declared_at = info.range; + let declaration_at = info.range.start(); let declaration_before_reference = - declared_at.start() < reference.range().start(); + declaration_at < reference.range().start(); let event = if declaration_before_reference { SemanticEvent::Read { range, - declared_at, + declaration_at, scope_id: 0, } } else { SemanticEvent::HoistedRead { range, - declared_at, + declaration_at, scope_id: 0, } }; diff --git a/crates/biome_js_semantic/src/semantic_model/builder.rs b/crates/biome_js_semantic/src/semantic_model/builder.rs index 6af5d7d78f52..44a00e39f4f4 100644 --- a/crates/biome_js_semantic/src/semantic_model/builder.rs +++ b/crates/biome_js_semantic/src/semantic_model/builder.rs @@ -201,10 +201,10 @@ impl SemanticModelBuilder { } Read { range, - declared_at: declaration_at, //TODO change to binding_id like we do with scope_id + declaration_at, scope_id, } => { - let binding_id = self.bindings_by_start[&declaration_at.start()]; + let binding_id = self.bindings_by_start[&declaration_at]; let binding = &mut self.bindings[binding_id as usize]; let reference_index = binding.references.len() as u32; @@ -224,13 +224,13 @@ impl SemanticModelBuilder { } HoistedRead { range, - declared_at: declaration_at, + declaration_at, scope_id, } => { - let binding_id = self.bindings_by_start[&declaration_at.start()]; + let binding_id = self.bindings_by_start[&declaration_at]; let binding = &mut self.bindings[binding_id as usize]; - let reference_index = binding.references.len() as u32; + binding.references.push(SemanticModelReference { index: (binding.id, reference_index).into(), range, @@ -247,10 +247,10 @@ impl SemanticModelBuilder { } Write { range, - declared_at: declaration_at, + declaration_at, scope_id, } => { - let binding_id = self.bindings_by_start[&declaration_at.start()]; + let binding_id = self.bindings_by_start[&declaration_at]; let binding = &mut self.bindings[binding_id as usize]; let reference_index = binding.references.len() as u32; @@ -270,10 +270,10 @@ impl SemanticModelBuilder { } HoistedWrite { range, - declared_at: declaration_at, + declaration_at, scope_id, } => { - let binding_id = self.bindings_by_start[&declaration_at.start()]; + let binding_id = self.bindings_by_start[&declaration_at]; let binding = &mut self.bindings[binding_id as usize]; let reference_index = binding.references.len() as u32; @@ -327,8 +327,8 @@ impl SemanticModelBuilder { .push(SemanticModelUnresolvedReference { range }), } } - Exported { range } => { - self.exported.insert(range.start()); + Export { declaration_at, .. } => { + self.exported.insert(declaration_at); } } } diff --git a/crates/biome_js_semantic/src/tests/assertions.rs b/crates/biome_js_semantic/src/tests/assertions.rs index fd9d703d9212..19d4ed4668ab 100644 --- a/crates/biome_js_semantic/src/tests/assertions.rs +++ b/crates/biome_js_semantic/src/tests/assertions.rs @@ -124,16 +124,27 @@ pub fn assert(code: &str, test_name: &str) { // Extract semantic events and index by range let mut events_by_pos: FxHashMap> = FxHashMap::default(); - let mut scope_start_by_id: FxHashMap = FxHashMap::default(); + let mut declaration_range_by_start: FxHashMap = FxHashMap::default(); + let mut scope_range_by_id: FxHashMap = FxHashMap::default(); for event in semantic_events(r.syntax()) { - let pos = if let SemanticEvent::ScopeEnded { - range, scope_id, .. - } = event - { - scope_start_by_id.insert(scope_id, range.start()); - range.end() - } else { - event.range().start() + let pos = match event { + SemanticEvent::DeclarationFound { range, .. } => { + declaration_range_by_start.insert(range.start(), range); + range.start() + } + SemanticEvent::ScopeStarted { + range, scope_id, .. + } => { + scope_range_by_id.insert(scope_id, range); + range.start() + } + SemanticEvent::Read { range, .. } + | SemanticEvent::HoistedRead { range, .. } + | SemanticEvent::Write { range, .. } + | SemanticEvent::HoistedWrite { range, .. } + | SemanticEvent::UnresolvedReference { range, .. } => range.start(), + SemanticEvent::ScopeEnded { range, .. } => range.end(), + SemanticEvent::Export { .. } => continue, }; let v = events_by_pos.entry(pos).or_default(); v.push(event); @@ -143,7 +154,13 @@ pub fn assert(code: &str, test_name: &str) { // check - assertions.check(code, test_name, events_by_pos, scope_start_by_id); + assertions.check( + code, + test_name, + events_by_pos, + declaration_range_by_start, + scope_range_by_id, + ); } #[derive(Debug, Diagnostic)] @@ -447,7 +464,8 @@ impl SemanticAssertions { code: &str, test_name: &str, events_by_pos: FxHashMap>, - scope_start: FxHashMap, + declaration_range_by_start: FxHashMap, + scope_range_by_id: FxHashMap, ) { // Check every declaration assertion is ok @@ -503,23 +521,19 @@ impl SemanticAssertions { let mut unused_match = None; let at_least_one_match = events.iter().any(|e| { let declaration_at_range = match &e { - SemanticEvent::Read { - declared_at: declaration_at, - .. - } => Some(*declaration_at), - SemanticEvent::HoistedRead { - declared_at: declaration_at, - .. - } => Some(*declaration_at), + SemanticEvent::Read { declaration_at, .. } + | SemanticEvent::HoistedRead { declaration_at, .. } => { + declaration_range_by_start.get(declaration_at) + } _ => None, }; if let Some(declaration_at_range) = declaration_at_range { unused_match = Some(format!( "{} != {}", - &code[declaration_at_range], &code[decl.range] + &code[*declaration_at_range], &code[decl.range] )); - code[declaration_at_range] == code[decl.range] + code[*declaration_at_range] == code[decl.range] } else { false } @@ -567,19 +581,15 @@ impl SemanticAssertions { let at_least_one_match = events.iter().any(|e| { let declaration_at_range = match &e { - SemanticEvent::Write { - declared_at: declaration_at, - .. - } => Some(*declaration_at), - SemanticEvent::HoistedWrite { - declared_at: declaration_at, - .. - } => Some(*declaration_at), + SemanticEvent::Write { declaration_at, .. } + | SemanticEvent::HoistedWrite { declaration_at, .. } => { + declaration_range_by_start.get(declaration_at) + } _ => None, }; if let Some(declaration_at_range) = declaration_at_range { - code[declaration_at_range] == code[decl.range] + code[*declaration_at_range] == code[decl.range] } else { false } @@ -614,7 +624,7 @@ impl SemanticAssertions { { Some(scope_start_assertion) => { let scope_started_at = - &scope_start[&hoisted_scope_id.unwrap_or(*scope_id)]; + &scope_range_by_id[&hoisted_scope_id.unwrap_or(*scope_id)].start(); if scope_start_assertion.range.start() != *scope_started_at { show_all_events(test_name, code, events_by_pos, is_scope_event); show_unmatched_assertion( diff --git a/xtask/coverage/src/symbols/msts.rs b/xtask/coverage/src/symbols/msts.rs index 6b768a3e5ae0..cf6aac2b76f8 100644 --- a/xtask/coverage/src/symbols/msts.rs +++ b/xtask/coverage/src/symbols/msts.rs @@ -93,24 +93,22 @@ impl TestCase for SymbolsMicrosoftTestCase { // We do the same below because TS classifies some string literals as symbols and we also // filter them below. match x { - SemanticEvent::DeclarationFound { .. } - | SemanticEvent::Read { .. } - | SemanticEvent::HoistedRead { .. } - | SemanticEvent::Write { .. } - | SemanticEvent::HoistedWrite { .. } - | SemanticEvent::UnresolvedReference { .. } => { - let name = &code[x.range()]; - !name.contains('\"') && !name.contains('\'') + SemanticEvent::DeclarationFound { range, .. } + | SemanticEvent::Read { range, .. } + | SemanticEvent::HoistedRead { range, .. } + | SemanticEvent::Write { range, .. } + | SemanticEvent::HoistedWrite { range, .. } + | SemanticEvent::UnresolvedReference { range, .. } => { + let name = &code[*range]; + !name.contains('\"') && !name.contains('\'') && + // Ignore the current event if one was already processed for the same range. + prev_starts.insert(range.start()) } SemanticEvent::ScopeStarted { .. } | SemanticEvent::ScopeEnded { .. } - | SemanticEvent::Exported { .. } => false, + | SemanticEvent::Export { .. } => false, } }) - .filter(|x| { - // Ignore the current event if one was already processed for the same range. - prev_starts.insert(x.range().start()) - }) .collect(); actual.sort_by_key(|x| x.range().start());