Skip to content

Commit

Permalink
refactor(semantic): should not add symbols of ClassExpression and Fun…
Browse files Browse the repository at this point in the history
…ctionExpression to the scope
  • Loading branch information
Dunqing committed Jul 9, 2024
1 parent daea29c commit adfebfa
Show file tree
Hide file tree
Showing 10 changed files with 115 additions and 59 deletions.
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/nextjs/no_duplicate_head.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ impl Rule for NoDuplicateHead {
return;
}

let scope_id = symbols.get_scope_id(symbol_id);
let Some(scope_id) = symbols.get_scope_id(symbol_id) else { return };
if scope_id != ctx.scopes().root_scope_id() {
return;
}
Expand Down
66 changes: 43 additions & 23 deletions crates/oxc_semantic/src/binder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,12 @@ impl<'a> Binder for VariableDeclarator<'a> {

if self.kind.is_lexical() {
self.id.bound_names(&mut |ident| {
let symbol_id = builder.declare_symbol(ident.span, &ident.name, includes, excludes);
let symbol_id = builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
includes,
excludes,
);
ident.symbol_id.set(Some(symbol_id));
});
return;
Expand Down Expand Up @@ -80,12 +85,16 @@ impl<'a> Binder for Class<'a> {
fn bind(&self, builder: &mut SemanticBuilder) {
let Some(ident) = &self.id else { return };
if !self.declare {
let symbol_id = builder.declare_symbol(
ident.span,
&ident.name,
SymbolFlags::Class,
SymbolFlags::ClassExcludes,
);
let symbol_id = if self.is_declaration() {
builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
SymbolFlags::Class,
SymbolFlags::ClassExcludes,
)
} else {
builder.declare_symbol(ident.span, &ident.name, SymbolFlags::Class)
};
ident.symbol_id.set(Some(symbol_id));
}
}
Expand Down Expand Up @@ -143,12 +152,8 @@ impl<'a> Binder for Function<'a> {
} else if self.r#type == FunctionType::FunctionExpression {
// https://tc39.es/ecma262/#sec-runtime-semantics-instantiateordinaryfunctionexpression
// 5. Perform ! funcEnv.CreateImmutableBinding(name, false).
let symbol_id = builder.declare_symbol(
ident.span,
&ident.name,
SymbolFlags::Function,
SymbolFlags::empty(),
);
let symbol_id =
builder.declare_symbol(ident.span, &ident.name, SymbolFlags::Function);
ident.symbol_id.set(Some(symbol_id));
}
}
Expand Down Expand Up @@ -196,7 +201,12 @@ impl<'a> Binder for BindingRestElement<'a> {
let excludes =
SymbolFlags::FunctionScopedVariable | SymbolFlags::FunctionScopedVariableExcludes;
self.bound_names(&mut |ident| {
let symbol_id = builder.declare_symbol(ident.span, &ident.name, includes, excludes);
let symbol_id = builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
includes,
excludes,
);
ident.symbol_id.set(Some(symbol_id));
});
}
Expand Down Expand Up @@ -235,7 +245,12 @@ impl<'a> Binder for FormalParameter<'a> {
};

self.bound_names(&mut |ident| {
let symbol_id = builder.declare_symbol(ident.span, &ident.name, includes, excludes);
let symbol_id = builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
includes,
excludes,
);
ident.symbol_id.set(Some(symbol_id));
});
}
Expand All @@ -254,7 +269,7 @@ impl<'a> Binder for CatchParameter<'a> {
ident.symbol_id.set(Some(symbol_id));
} else {
self.pattern.bound_names(&mut |ident| {
let symbol_id = builder.declare_symbol(
let symbol_id = builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
SymbolFlags::BlockScopedVariable | SymbolFlags::CatchVariable,
Expand All @@ -267,7 +282,7 @@ impl<'a> Binder for CatchParameter<'a> {
}

fn declare_symbol_for_import_specifier(ident: &BindingIdentifier, builder: &mut SemanticBuilder) {
let symbol_id = builder.declare_symbol(
let symbol_id = builder.declare_symbol_on_current_scope(
ident.span,
&ident.name,
SymbolFlags::ImportBinding,
Expand Down Expand Up @@ -302,7 +317,7 @@ impl<'a> Binder for TSImportEqualsDeclaration<'a> {

impl<'a> Binder for TSTypeAliasDeclaration<'a> {
fn bind(&self, builder: &mut SemanticBuilder) {
let symbol_id = builder.declare_symbol(
let symbol_id = builder.declare_symbol_on_current_scope(
self.id.span,
&self.id.name,
SymbolFlags::TypeAlias,
Expand All @@ -314,7 +329,7 @@ impl<'a> Binder for TSTypeAliasDeclaration<'a> {

impl<'a> Binder for TSInterfaceDeclaration<'a> {
fn bind(&self, builder: &mut SemanticBuilder) {
let symbol_id = builder.declare_symbol(
let symbol_id = builder.declare_symbol_on_current_scope(
self.id.span,
&self.id.name,
SymbolFlags::Interface,
Expand All @@ -333,7 +348,12 @@ impl<'a> Binder for TSEnumDeclaration<'a> {
} else {
SymbolFlags::RegularEnumExcludes
};
let symbol_id = builder.declare_symbol(self.id.span, &self.id.name, includes, excludes);
let symbol_id = builder.declare_symbol_on_current_scope(
self.id.span,
&self.id.name,
includes,
excludes,
);
self.id.symbol_id.set(Some(symbol_id));
}
}
Expand All @@ -350,7 +370,7 @@ impl<'a> Binder for TSEnumMember<'a> {
TSEnumMemberName::StaticNumericLiteral(n) => Cow::Owned(n.value.to_string()),
match_expression!(TSEnumMemberName) => panic!("TODO: implement"),
};
builder.declare_symbol(
builder.declare_symbol_on_current_scope(
self.span,
&name,
SymbolFlags::EnumMember,
Expand All @@ -364,7 +384,7 @@ impl<'a> Binder for TSModuleDeclaration<'a> {
// At declaration time a module has no value declaration it is only when a value declaration
// is made inside a the scope of a module that the symbol is modified
let ambient = if self.declare { SymbolFlags::Ambient } else { SymbolFlags::None };
builder.declare_symbol(
builder.declare_symbol_on_current_scope(
self.span,
self.id.name().as_str(),
SymbolFlags::NameSpaceModule | ambient,
Expand All @@ -375,7 +395,7 @@ impl<'a> Binder for TSModuleDeclaration<'a> {

impl<'a> Binder for TSTypeParameter<'a> {
fn bind(&self, builder: &mut SemanticBuilder) {
let symbol_id = builder.declare_symbol(
let symbol_id = builder.declare_symbol_on_current_scope(
self.name.span,
&self.name.name,
SymbolFlags::TypeParameter,
Expand Down
44 changes: 31 additions & 13 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,15 @@ impl<'a> SemanticBuilder<'a> {
}
}

/// Declares a `Symbol` for the node, adds it to symbol table
///
/// includes: the `SymbolFlags` that node has in addition to its declaration type (eg: export, ambient, etc.)
pub fn declare_symbol(&mut self, span: Span, name: &str, includes: SymbolFlags) -> SymbolId {
let includes = includes | self.current_symbol_flags;
let name = CompactStr::new(name);
self.symbols.create_symbol(span, name, self.current_node_id, includes, None)
}

/// Declares a `Symbol` for the node, adds it to symbol table, and binds it to the scope.
///
/// includes: the `SymbolFlags` that node has in addition to its declaration type (eg: export, ambient, etc.)
Expand All @@ -288,14 +297,13 @@ impl<'a> SemanticBuilder<'a> {
}

let includes = includes | self.current_symbol_flags;
let name = CompactStr::new(name);
let symbol_id = self.symbols.create_symbol(span, name.clone(), includes, scope_id);
self.symbols.add_declaration(self.current_node_id);
self.scope.add_binding(scope_id, name, symbol_id);
let symbol_id = self.declare_symbol(span, name, includes);
self.symbols.set_scope_id(symbol_id, scope_id);
self.scope.add_binding(scope_id, CompactStr::new(name), symbol_id);
symbol_id
}

pub fn declare_symbol(
pub fn declare_symbol_on_current_scope(
&mut self,
span: Span,
name: &str,
Expand Down Expand Up @@ -341,11 +349,9 @@ impl<'a> SemanticBuilder<'a> {
includes: SymbolFlags,
) -> SymbolId {
let includes = includes | self.current_symbol_flags;
let name = CompactStr::new(name);
let symbol_id =
self.symbols.create_symbol(span, name.clone(), includes, self.current_scope_id);
self.symbols.add_declaration(self.current_node_id);
self.scope.get_bindings_mut(scope_id).insert(name, symbol_id);
let symbol_id = self.declare_symbol(span, name, includes);
self.symbols.set_scope_id(symbol_id, scope_id);
self.scope.get_bindings_mut(scope_id).insert(CompactStr::new(name), symbol_id);
symbol_id
}

Expand All @@ -364,7 +370,19 @@ impl<'a> SemanticBuilder<'a> {
let parent_scope_id =
self.scope.get_parent_id(self.current_scope_id).unwrap_or(self.current_scope_id);

if let Some(symbol_id) = self.scope.get_binding(self.current_scope_id, &name) {
if let Some(symbol_id) =
self.scope.get_binding(self.current_scope_id, &name).or_else(|| {
self.symbols.get_symbol_id_from_declaration(self.current_node_id).and_then(
|symbol_id| {
if self.symbols.get_name(symbol_id) == name {
Some(symbol_id)
} else {
None
}
},
)
})
{
for reference_id in &reference_ids {
self.symbols.references[*reference_id].set_symbol_id(symbol_id);
}
Expand Down Expand Up @@ -1495,8 +1513,8 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
if let Some(annotation) = &func.return_type {
self.visit_ts_type_annotation(annotation);
}
self.leave_node(kind);
self.leave_scope();
self.leave_node(kind);
}

fn visit_class(&mut self, class: &Class<'a>) {
Expand Down Expand Up @@ -1534,10 +1552,10 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
}
self.visit_class_body(&class.body);

self.leave_node(kind);
if is_class_expr {
self.leave_scope();
}
self.leave_node(kind);
}

fn visit_static_block(&mut self, block: &StaticBlock<'a>) {
Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_semantic/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ impl<'a> Semantic<'a> {
}

/// Find which scope a symbol is declared in
pub fn symbol_scope(&self, symbol_id: SymbolId) -> ScopeId {
pub fn symbol_scope(&self, symbol_id: SymbolId) -> Option<ScopeId> {
self.symbols.get_scope_id(symbol_id)
}

Expand Down Expand Up @@ -198,7 +198,7 @@ mod tests {
.iter_bindings()
.find(|(_scope_id, _symbol_id, name)| name.as_str() == "Fn")
.unwrap();
assert_eq!(semantic.symbols.get_scope_id(top_level_a.1), top_level_a.0);
assert_eq!(semantic.symbols.get_scope_id(top_level_a.1), Some(top_level_a.0));
}

#[test]
Expand Down
30 changes: 21 additions & 9 deletions crates/oxc_semantic/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub struct SymbolTable {
pub spans: IndexVec<SymbolId, Span>,
pub names: IndexVec<SymbolId, CompactStr>,
pub flags: IndexVec<SymbolId, SymbolFlags>,
pub scope_ids: IndexVec<SymbolId, ScopeId>,
pub scope_ids: IndexVec<SymbolId, Option<ScopeId>>,
/// Pointer to the AST Node where this symbol is declared
pub declarations: IndexVec<SymbolId, AstNodeId>,
pub resolved_references: IndexVec<SymbolId, Vec<ReferenceId>>,
Expand Down Expand Up @@ -73,6 +73,16 @@ impl SymbolTable {
})
}

pub fn get_symbol_id_from_declaration(&self, id: AstNodeId) -> Option<SymbolId> {
self.declarations.iter_enumerated().find_map(|(symbol_id, node_id)| {
if *node_id == id {
Some(symbol_id)
} else {
None
}
})
}

pub fn get_span(&self, symbol_id: SymbolId) -> Span {
self.spans[symbol_id]
}
Expand All @@ -97,16 +107,20 @@ impl SymbolTable {
self.flags[symbol_id] |= includes;
}

pub fn get_scope_id(&self, symbol_id: SymbolId) -> ScopeId {
pub fn set_scope_id(&mut self, symbol_id: SymbolId, scope_id: ScopeId) {
self.scope_ids[symbol_id] = Some(scope_id);
}

pub fn get_scope_id(&self, symbol_id: SymbolId) -> Option<ScopeId> {
self.scope_ids[symbol_id]
}

pub fn get_scope_id_from_span(&self, span: &Span) -> Option<ScopeId> {
self.get_symbol_id_from_span(span).map(|symbol_id| self.get_scope_id(symbol_id))
self.get_symbol_id_from_span(span).and_then(|symbol_id| self.get_scope_id(symbol_id))
}

pub fn get_scope_id_from_name(&self, name: &str) -> Option<ScopeId> {
self.get_symbol_id_from_name(name).map(|symbol_id| self.get_scope_id(symbol_id))
self.get_symbol_id_from_name(name).and_then(|symbol_id| self.get_scope_id(symbol_id))
}

pub fn get_declaration(&self, symbol_id: SymbolId) -> AstNodeId {
Expand All @@ -117,21 +131,19 @@ impl SymbolTable {
&mut self,
span: Span,
name: CompactStr,
declaration: AstNodeId,
flag: SymbolFlags,
scope_id: ScopeId,
scope_id: Option<ScopeId>,
) -> SymbolId {
_ = self.spans.push(span);
_ = self.names.push(name);
_ = self.declarations.push(declaration);
_ = self.flags.push(flag);
_ = self.scope_ids.push(scope_id);
_ = self.resolved_references.push(vec![]);
self.redeclare_variables.push(vec![])
}

pub fn add_declaration(&mut self, node_id: AstNodeId) {
self.declarations.push(node_id);
}

pub fn add_redeclare_variable(&mut self, symbol_id: SymbolId, span: Span) {
self.redeclare_variables[symbol_id].push(span);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/tests/integration/scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ fn test_function_level_strict() {
.is_in_scope(ScopeFlags::StrictMode | ScopeFlags::Function)
.expect(|(semantic, symbol_id)| -> Result<(), &'static str> {
let scope_id = semantic.symbol_scope(symbol_id);
let Some(parent_scope_id) = semantic.scopes().get_parent_id(scope_id) else {
let Some(parent_scope_id) = scope_id.and_then(|scope_id| semantic.scopes().get_parent_id(scope_id)) else {
return Err("Expected x's scope to have a parent")
};
let parent_flags = semantic.scopes().get_flags(parent_scope_id);
Expand Down
4 changes: 0 additions & 4 deletions crates/oxc_semantic/tests/integration/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ fn test_function_simple() {

#[test]
fn test_function_expressions() {
SemanticTester::js("const x = function y() {}")
.has_some_symbol("y")
.contains_flags(SymbolFlags::Function)
.test();
SemanticTester::js("const x = () => {}")
.has_some_symbol("x")
.contains_flags(SymbolFlags::BlockScopedVariable | SymbolFlags::ConstVariable)
Expand Down
10 changes: 6 additions & 4 deletions crates/oxc_semantic/tests/integration/util/symbol_tester.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,9 @@ impl<'a> SymbolTester<'a> {
self.test_result = match self.test_result {
Ok(symbol_id) => {
let scope_id = self.semantic.symbol_scope(symbol_id);
let scope_flags = self.semantic.scopes().get_flags(scope_id);
if scope_flags.contains(expected_flags) {
let scope_flags =
scope_id.map(|scope_id| self.semantic.scopes().get_flags(scope_id));
if scope_flags.is_some_and(|flags| flags.contains(expected_flags)) {
Ok(symbol_id)
} else {
Err(OxcDiagnostic::error(format!(
Expand All @@ -219,8 +220,9 @@ impl<'a> SymbolTester<'a> {
self.test_result = match self.test_result {
Ok(symbol_id) => {
let scope_id = self.semantic.symbol_scope(symbol_id);
let scope_flags = self.semantic.scopes().get_flags(scope_id);
if scope_flags.contains(excluded_flags) {
let scope_flags =
scope_id.map(|scope_id| self.semantic.scopes().get_flags(scope_id));
if scope_flags.is_some_and(|flag| flag.contains(excluded_flags)) {
Err(OxcDiagnostic::error(format!(
"Binding {target_name} is in a scope with excluded flags.\n\tExpected: not {excluded_flags:?}\n\tActual: {scope_flags:?}"
)))
Expand Down
Loading

0 comments on commit adfebfa

Please sign in to comment.