Skip to content

Commit

Permalink
fix(semantic): consider update expressions in implicit returns as reads
Browse files Browse the repository at this point in the history
  • Loading branch information
DonIsaac committed Aug 23, 2024
1 parent 8833061 commit c9f7308
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 1 deletion.
2 changes: 2 additions & 0 deletions crates/oxc_ast/src/ast/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1826,6 +1826,8 @@ pub struct ArrowFunctionExpression<'a> {
#[serde(flatten)]
pub span: Span,
/// Is the function body an arrow expression? i.e. `() => expr` instead of `() => {}`
///
/// When `true`, this function implicitly returns the expression.
pub expression: bool,
pub r#async: bool,
pub type_parameters: Option<Box<'a, TSTypeParameterDeclaration<'a>>>,
Expand Down
6 changes: 5 additions & 1 deletion crates/oxc_linter/src/rules/eslint/no_unused_vars/allowed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ impl<'s, 'a> Symbol<'s, 'a> {
let Some(AstKind::FunctionBody(body)) = self.nodes().parent_kind(parent.id()) else {
return false;
};
return body.span.contains_inclusive(self.span()) && body.statements.len() == 1 && !self.get_snippet(body.span).starts_with('{')
return self.is_implicit_return(body);
}
_ => {
parent.kind().debug_name();
Expand Down Expand Up @@ -103,6 +103,10 @@ impl<'s, 'a> Symbol<'s, 'a> {
.map(|node_id| nodes.get_node(node_id))
.any(|node| matches!(node.kind(), AstKind::TSModuleDeclaration(namespace) if is_ambient_namespace(namespace)))
}

pub fn is_implicit_return(&self, body: &FunctionBody) -> bool {
body.statements.len() == 1 && !self.get_snippet(body.span).starts_with('{')
}
}

#[inline]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ fn test_vars_reassignment() {
",
"let i = 0; i > 0 ? 'positive' : 'negative';",
"let i = 0; i > 0 && console.log('positive');",
"export function counter() { let i = 0; return () => ++i }",
];

let fail = vec![
Expand Down
30 changes: 30 additions & 0 deletions crates/oxc_semantic/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ pub struct SemanticBuilder<'a> {
// and when we reach a value declaration we set it
// to value like
pub(crate) namespace_stack: Vec<SymbolId>,
pub(crate) is_in_return: bool,
current_reference_flags: ReferenceFlags,
pub(crate) hoisting_variables: FxHashMap<ScopeId, FxHashMap<Atom<'a>, SymbolId>>,

Expand Down Expand Up @@ -135,6 +136,7 @@ impl<'a> SemanticBuilder<'a> {
current_scope_id,
function_stack: vec![],
namespace_stack: vec![],
is_in_return: false,
nodes: AstNodes::default(),
hoisting_variables: FxHashMap::default(),
scope,
Expand Down Expand Up @@ -1230,7 +1232,10 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
/* cfg */

let ret_kind = if let Some(arg) = &stmt.argument {
let in_return = self.is_in_return;
self.is_in_return = true;
self.visit_expression(arg);
self.is_in_return = in_return;
ReturnInstructionKind::NotImplicitUndefined
} else {
ReturnInstructionKind::ImplicitUndefined
Expand Down Expand Up @@ -1625,7 +1630,10 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
self.visit_ts_type_annotation(return_type);
}
if let Some(body) = &func.body {
let in_return = self.is_in_return;
self.is_in_return = false;
self.visit_function_body(body);
self.is_in_return = in_return;
}

/* cfg */
Expand Down Expand Up @@ -1678,7 +1686,10 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
self.visit_ts_type_annotation(return_type);
}

let in_return = self.is_in_return;
self.is_in_return = expr.expression;
self.visit_function_body(&expr.body);
self.is_in_return = in_return;

/* cfg */
control_flow!(self, |cfg| {
Expand All @@ -1692,6 +1703,23 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> {
self.leave_node(kind);
self.leave_scope();
}

fn visit_sequence_expression(&mut self, expr: &SequenceExpression<'a>) {
let kind = AstKind::SequenceExpression(self.alloc(expr));
self.enter_node(kind);

let last = expr.expressions.len().saturating_sub(0);
let in_return = self.is_in_return;
self.is_in_return = false;
for (i, expr) in expr.expressions.iter().enumerate() {
if i == last {
self.is_in_return = in_return;
}
self.visit_expression(expr);
}

self.leave_node(kind);
}
}

impl<'a> SemanticBuilder<'a> {
Expand Down Expand Up @@ -2000,6 +2028,8 @@ impl<'a> SemanticBuilder<'a> {
fn resolve_reference_usages(&self) -> ReferenceFlags {
if self.current_reference_flags.is_empty() {
ReferenceFlags::Read
} else if self.is_in_return {
self.current_reference_flags | ReferenceFlags::Read
} else {
self.current_reference_flags
}
Expand Down
48 changes: 48 additions & 0 deletions crates/oxc_semantic/tests/integration/symbols.rs
Original file line number Diff line number Diff line change
Expand Up @@ -355,3 +355,51 @@ fn test_ts_interface_heritage() {
.has_number_of_references(1)
.test();
}

#[test]
fn test_arrow_implicit_return() {
SemanticTester::js("let i = 0; const x = () => i")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(0)
.test();

SemanticTester::js("let i = 0; const x = () => ++i")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(1)
.test();

SemanticTester::js("let i = 0; const x = () => { ++i }")
.has_root_symbol("i")
.has_number_of_reads(0)
.has_number_of_writes(1)
.test();

SemanticTester::js("let i = 0; const x = () => (0, ++i)")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(1)
.test();

SemanticTester::js("let i = 0; const x = () => (++i, 0)")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(1)
.test();
}

#[test]
fn test_arrow_explicit_return() {
SemanticTester::js("let i = 0; const x = () => { return i }")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(0)
.test();

SemanticTester::js("let i = 0; const x = () => { return ++i }")
.has_root_symbol("i")
.has_number_of_reads(1)
.has_number_of_writes(1)
.test();
}

0 comments on commit c9f7308

Please sign in to comment.