Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(semantic): always use SymbolFlags::Function for function id #7479

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions crates/oxc_ast/src/ast_impl/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -968,11 +968,7 @@ impl<'a> Function<'a> {

/// `true` for overload signatures and `declare function` statements.
pub fn is_typescript_syntax(&self) -> bool {
matches!(
self.r#type,
FunctionType::TSDeclareFunction | FunctionType::TSEmptyBodyFunctionExpression
) || self.body.is_none()
|| self.declare
self.r#type.is_typescript_syntax() || self.body.is_none() || self.declare
}

/// `true` for function expressions
Expand Down Expand Up @@ -1001,6 +997,13 @@ impl<'a> Function<'a> {
}
}

impl FunctionType {
/// Returns `true` if it is a [`FunctionType::TSDeclareFunction`] or [`FunctionType::TSEmptyBodyFunctionExpression`].
pub fn is_typescript_syntax(&self) -> bool {
matches!(self, Self::TSDeclareFunction | Self::TSEmptyBodyFunctionExpression)
}
}

impl<'a> FormalParameters<'a> {
/// Number of parameters bound in this parameter list.
pub fn parameters_count(&self) -> usize {
Expand Down
12 changes: 11 additions & 1 deletion crates/oxc_linter/src/rules/eslint/no_setter_return.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use oxc_ast::AstKind;
use oxc_diagnostics::OxcDiagnostic;
use oxc_macros::declare_oxc_lint;
use oxc_semantic::ScopeFlags;
use oxc_span::Span;

use crate::{context::LintContext, rule::Rule, AstNode};
Expand Down Expand Up @@ -41,7 +42,16 @@ impl Rule for NoSetterReturn {
let AstKind::ReturnStatement(stmt) = node.kind() else {
return;
};
if stmt.argument.is_some() && ctx.scopes().get_flags(node.scope_id()).is_set_accessor() {

if stmt.argument.is_some() {
for scope_id in ctx.scopes().ancestors(node.scope_id()) {
let flags = ctx.scopes().get_flags(scope_id);
if flags.is_set_accessor() {
break;
} else if flags.intersects(ScopeFlags::Function | ScopeFlags::Top) {
return;
}
}
ctx.diagnostic(no_setter_return_diagnostic(stmt.span));
}
}
Expand Down
3 changes: 0 additions & 3 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/usage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -614,9 +614,6 @@ impl<'s, 'a> Symbol<'s, 'a> {
let Some(ref_node) = self.get_ref_relevant_node(reference) else {
return false;
};
if !matches!(ref_node.kind(), AstKind::CallExpression(_) | AstKind::NewExpression(_)) {
return false;
}

// Do the easy/fast path if possible. If we know its a class/fn from
// flags, that means it's declared within this file in an understandable
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/oxc_linter/src/tester.rs
snapshot_kind: text
---
⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:10]
Expand All @@ -24,16 +23,16 @@ snapshot_kind: text
help: Shadowing of global properties 'NaN'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:40]
╭─[no_shadow_restricted_names.tsx:1:44]
1 │ function NaN(NaN) { var NaN; !function NaN(NaN) { try {} catch(NaN) {} }; }
· ───
· ───
╰────
help: Shadowing of global properties 'NaN'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:44]
╭─[no_shadow_restricted_names.tsx:1:40]
1 │ function NaN(NaN) { var NaN; !function NaN(NaN) { try {} catch(NaN) {} }; }
· ───
· ───
╰────
help: Shadowing of global properties 'NaN'.

Expand All @@ -59,16 +58,16 @@ snapshot_kind: text
help: Shadowing of global properties 'undefined'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:43]
╭─[no_shadow_restricted_names.tsx:1:53]
1 │ function undefined(undefined) { !function undefined(undefined) { try {} catch(undefined) {} }; }
· ─────────
· ─────────
╰────
help: Shadowing of global properties 'undefined'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:53]
╭─[no_shadow_restricted_names.tsx:1:43]
1 │ function undefined(undefined) { !function undefined(undefined) { try {} catch(undefined) {} }; }
· ─────────
· ─────────
╰────
help: Shadowing of global properties 'undefined'.

Expand Down Expand Up @@ -101,16 +100,16 @@ snapshot_kind: text
help: Shadowing of global properties 'Infinity'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:55]
╭─[no_shadow_restricted_names.tsx:1:64]
1 │ function Infinity(Infinity) { var Infinity; !function Infinity(Infinity) { try {} catch(Infinity) {} }; }
· ────────
· ────────
╰────
help: Shadowing of global properties 'Infinity'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:64]
╭─[no_shadow_restricted_names.tsx:1:55]
1 │ function Infinity(Infinity) { var Infinity; !function Infinity(Infinity) { try {} catch(Infinity) {} }; }
· ────────
· ────────
╰────
help: Shadowing of global properties 'Infinity'.

Expand Down Expand Up @@ -143,16 +142,16 @@ snapshot_kind: text
help: Shadowing of global properties 'arguments'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:58]
╭─[no_shadow_restricted_names.tsx:1:68]
1 │ function arguments(arguments) { var arguments; !function arguments(arguments) { try {} catch(arguments) {} }; }
· ─────────
· ─────────
╰────
help: Shadowing of global properties 'arguments'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:68]
╭─[no_shadow_restricted_names.tsx:1:58]
1 │ function arguments(arguments) { var arguments; !function arguments(arguments) { try {} catch(arguments) {} }; }
· ─────────
· ─────────
╰────
help: Shadowing of global properties 'arguments'.

Expand Down Expand Up @@ -185,16 +184,16 @@ snapshot_kind: text
help: Shadowing of global properties 'eval'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:43]
╭─[no_shadow_restricted_names.tsx:1:48]
1 │ function eval(eval) { var eval; !function eval(eval) { try {} catch(eval) {} }; }
· ────
· ────
╰────
help: Shadowing of global properties 'eval'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:48]
╭─[no_shadow_restricted_names.tsx:1:43]
1 │ function eval(eval) { var eval; !function eval(eval) { try {} catch(eval) {} }; }
· ────
· ────
╰────
help: Shadowing of global properties 'eval'.

Expand Down Expand Up @@ -227,16 +226,16 @@ snapshot_kind: text
help: Shadowing of global properties 'eval'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:44]
╭─[no_shadow_restricted_names.tsx:1:49]
1 │ var eval = (eval) => { var eval; !function eval(eval) { try {} catch(eval) {} }; }
· ────
· ────
╰────
help: Shadowing of global properties 'eval'.

⚠ eslint(no-shadow-restricted-names): Shadowing of global properties such as 'undefined' is not allowed.
╭─[no_shadow_restricted_names.tsx:1:49]
╭─[no_shadow_restricted_names.tsx:1:44]
1 │ var eval = (eval) => { var eval; !function eval(eval) { try {} catch(eval) {} }; }
· ────
· ────
╰────
help: Shadowing of global properties 'eval'.

Expand Down
4 changes: 2 additions & 2 deletions crates/oxc_linter/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -132,15 +132,15 @@ source: crates/oxc_linter/src/tester.rs
╰────
help: Consider removing this declaration.

⚠ eslint(no-unused-vars): Variable 'a' is declared but never used.
⚠ eslint(no-unused-vars): Function 'a' is declared but never used.
╭─[no_unused_vars.tsx:1:29]
1 │ function f(){var x;function a(){x=42;}function b(){alert(x);}}
· ┬
· ╰── 'a' is declared here
╰────
help: Consider removing this declaration.

⚠ eslint(no-unused-vars): Variable 'b' is declared but never used.
⚠ eslint(no-unused-vars): Function 'b' is declared but never used.
╭─[no_unused_vars.tsx:1:48]
1 │ function f(){var x;function a(){x=42;}function b(){alert(x);}}
· ┬
Expand Down
3 changes: 1 addition & 2 deletions crates/oxc_linter/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/oxc_linter/src/tester.rs
snapshot_kind: text
---
⚠ eslint(no-unused-vars): Function 'foo' is declared but never used.
╭─[no_unused_vars.tsx:1:10]
Expand All @@ -18,7 +17,7 @@ snapshot_kind: text
╰────
help: Consider removing this declaration.

⚠ eslint(no-unused-vars): Variable 'bar' is declared but never used.
⚠ eslint(no-unused-vars): Function 'bar' is declared but never used.
╭─[no_unused_vars.tsx:1:30]
1 │ const foo = () => { function bar() { } }
· ─┬─
Expand Down
58 changes: 32 additions & 26 deletions crates/oxc_semantic/src/binder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,16 @@ fn function_as_var(flags: ScopeFlags, source_type: SourceType) -> bool {
flags.is_function() || (source_type.is_script() && flags.is_top())
}

/// Check if the function is not allowed to be redeclared.
pub fn is_function_redeclared_not_allowed(
func: &Function<'_>,
builder: &SemanticBuilder<'_>,
) -> bool {
let current_scope_flags = builder.current_scope_flags();
(current_scope_flags.is_strict_mode() || func.r#async || func.generator)
&& !function_as_var(current_scope_flags, builder.source_type)
}

/// Check for Annex B `if (foo) function a() {} else function b() {}`
fn is_function_part_of_if_statement(function: &Function, builder: &SemanticBuilder) -> bool {
if builder.current_scope_flags().is_strict_mode() {
Expand All @@ -150,8 +160,9 @@ fn is_function_part_of_if_statement(function: &Function, builder: &SemanticBuild

impl<'a> Binder<'a> for Function<'a> {
fn bind(&self, builder: &mut SemanticBuilder) {
let current_scope_id = builder.current_scope_id;
let scope_flags = builder.current_scope_flags();
if self.r#type.is_typescript_syntax() {
return;
}
if let Some(ident) = &self.id {
if is_function_part_of_if_statement(self, builder) {
let symbol_id = builder.symbols.create_symbol(
Expand All @@ -162,35 +173,30 @@ impl<'a> Binder<'a> for Function<'a> {
builder.current_node_id,
);
ident.symbol_id.set(Some(symbol_id));
} else if self.r#type == FunctionType::FunctionDeclaration {
// The visitor is already inside the function scope,
// retrieve the parent scope for the function id to bind to.

let (includes, excludes) =
if (scope_flags.is_strict_mode() || self.r#async || self.generator)
&& !function_as_var(scope_flags, builder.source_type)
{
(
SymbolFlags::Function | SymbolFlags::BlockScopedVariable,
SymbolFlags::BlockScopedVariableExcludes,
)
} else {
let excludes = if self.is_declaration() {
// The visitor is already inside the function scope,
// retrieve the parent scope for the function id to bind to.
if is_function_redeclared_not_allowed(self, builder) {
SymbolFlags::BlockScopedVariableExcludes
} else {
(
SymbolFlags::FunctionScopedVariable,
SymbolFlags::FunctionScopedVariableExcludes,
)
};
SymbolFlags::FunctionScopedVariableExcludes
}
} else if self.is_expression() {
// https://tc39.es/ecma262/#sec-runtime-semantics-instantiateordinaryfunctionexpression
// 5. Perform ! funcEnv.CreateImmutableBinding(name, false).
SymbolFlags::empty()
} else {
unreachable!(
"Currently we haven't create a symbol for typescript syntax function"
);
};

let symbol_id = builder.declare_symbol(ident.span, &ident.name, includes, excludes);
ident.symbol_id.set(Some(symbol_id));
} 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(),
excludes,
);
ident.symbol_id.set(Some(symbol_id));
}
Expand All @@ -200,7 +206,7 @@ impl<'a> Binder<'a> for Function<'a> {
if let Some(AstKind::ObjectProperty(prop)) =
builder.nodes.parent_kind(builder.current_node_id)
{
let flags = builder.scope.get_flags_mut(current_scope_id);
let flags = builder.scope.get_flags_mut(builder.current_scope_id);
match prop.kind {
PropertyKind::Get => *flags |= ScopeFlags::GetAccessor,
PropertyKind::Set => *flags |= ScopeFlags::SetAccessor,
Expand Down
35 changes: 26 additions & 9 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use oxc_span::{Atom, CompactStr, SourceType, Span};
use oxc_syntax::module_record::ModuleRecord;

use crate::{
binder::Binder,
binder::{is_function_redeclared_not_allowed, Binder},
checker,
class::ClassTableBuilder,
diagnostics::redeclaration,
Expand Down Expand Up @@ -461,9 +461,23 @@ impl<'a> SemanticBuilder<'a> {
let symbol_id = self.scope.get_binding(scope_id, name).or_else(|| {
self.hoisting_variables.get(&scope_id).and_then(|symbols| symbols.get(name).copied())
})?;
if report_error && self.symbols.get_flags(symbol_id).intersects(excludes) {
let symbol_span = self.symbols.get_span(symbol_id);
self.error(redeclaration(name, symbol_span, span));
if report_error {
let flags = self.symbols.get_flags(symbol_id);
if flags.intersects(excludes)
// Needs to further check if the previous declaration is a function and the function
// is not allowed to be redeclared.
// For example: `async function goo() var goo;`
// ^^^ Redeclare
|| (excludes == SymbolFlags::FunctionScopedVariableExcludes
&& flags.is_function() // The previous declaration is a function
&& matches!( // Check the previous function whether it is allowed to be redeclared
self.nodes.kind(self.symbols.get_declaration(symbol_id)),
AstKind::Function(func) if is_function_redeclared_not_allowed(func, self)
))
{
let symbol_span = self.symbols.get_span(symbol_id);
self.error(redeclaration(name, symbol_span, span));
}
}
Some(symbol_id)
}
Expand Down Expand Up @@ -1628,11 +1642,6 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
&func.scope_id,
);

if func.is_expression() {
// We need to bind function expression in the function scope
func.bind(self);
}

if let Some(id) = &func.id {
self.visit_binding_identifier(id);
}
Expand Down Expand Up @@ -1691,6 +1700,14 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
});
/* cfg */

if func.is_expression() {
// We need to bind the function expression in the function scope and place
// it at the end of the function body in order to prevent redeclaration errors,
// as the function body may contain variables with the same name as the function.
// For example: `function foo() { let foo = 1; }` is valid.
func.bind(self);
}

self.leave_scope();
self.leave_node(kind);
}
Expand Down
Loading
Loading