Skip to content

Commit

Permalink
fix(linter/no-unused-vars): false positive for discarded reads within…
Browse files Browse the repository at this point in the history
… sequences (oxc-project#6907)

Fixes a case where no-unused-vars incorectly reports a read as unused in edge
cases where a logical/binary expression is used as a conditional shorthand to
write a variable within sequence expressions.

Some code examples will make this more clear.
```js
function foo(a) {
  let x = somePropertyIWantToCheck();
  (x in a && x.hasPropA = true, console.log(a))
}
```
Since the logical expression is not in the last position within the sequence
expression list, it's getting discarded as unused. However, the right expression
(`x.hasPropA = true`) has side effects, so it _is_ being used.
  • Loading branch information
DonIsaac authored and Orenbek committed Oct 28, 2024
1 parent ecefc14 commit 50348c8
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 0 deletions.
5 changes: 5 additions & 0 deletions crates/oxc_ast/src/ast_impl/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@ impl<'a> Expression<'a> {
false
}
}

/// Returns `true` if this is an [assignment expression](AssignmentExpression).
pub fn is_assignment(&self) -> bool {
matches!(self, Expression::AssignmentExpression(_))
}
}

impl<'a> fmt::Display for IdentifierName<'a> {
Expand Down
24 changes: 24 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/tests/oxc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,30 @@ fn test_vars_discarded_reads() {
new Test();
",
"function foo(a) {
const Bar = require('./bar');
a instanceof Bar && (this.a = a);
}
foo(1)
",
"function foo(a) {
const Bar = require('./bar');
(a instanceof Bar && (this.a = a), this.b = 1);
}
foo(1)
",
"function foo(a) {
const Bar = require('./bar');
(a instanceof Bar && (this.a ||= 2), this.b = 1);
}
foo(1)
",
"function foo(a) {
const bar = require('./bar');
(a in bar && (this.a = a), this.b = 1);
}
foo(1)
",
];

let fail = vec![
Expand Down
17 changes: 17 additions & 0 deletions crates/oxc_linter/src/rules/eslint/no_unused_vars/usage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,23 @@ impl<'s, 'a> Symbol<'s, 'a> {
return false;
}
}
// x && (a = x)
(AstKind::LogicalExpression(expr), _) => {
if expr.left.span().contains_inclusive(ref_span())
&& expr.right.get_inner_expression().is_assignment()
{
return false;
}
}
// x instanceof Foo && (a = x)
(AstKind::BinaryExpression(expr), _) if expr.operator.is_relational() => {
if expr.left.span().contains_inclusive(ref_span())
&& expr.right.get_inner_expression().is_assignment()
{
return false;
}
continue;
}
(parent, AstKind::SequenceExpression(seq)) => {
debug_assert!(
!seq.expressions.is_empty(),
Expand Down

0 comments on commit 50348c8

Please sign in to comment.