Skip to content

Commit fea905f

Browse files
qraqrasqraqras
authored
fix(lint/useOptionalChain): fix incorrect suggestions for typeof checks on global objects (#7529)
1 parent c294644 commit fea905f

21 files changed

+2099
-1555
lines changed

‎.changeset/tidy-teams-mix.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
"@biomejs/biome": patch
3+
---
4+
5+
Fixed [#7517](https://github.com/biomejs/biome/issues/7517): the [`useOptionalChain`](https://biomejs.dev/linter/rules/use-optional-chain/) rule no longer suggests changes for typeof checks on global objects.
6+
7+
```ts
8+
// ok
9+
typeof window !== 'undefined' && window.location;
10+
```

‎crates/biome_js_analyze/src/lint/complexity/use_optional_chain.rs‎

Lines changed: 177 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
use biome_analyze::RuleSource;
2-
use biome_analyze::{Ast, FixKind, Rule, RuleDiagnostic, context::RuleContext, declare_lint_rule};
2+
use biome_analyze::{FixKind, Rule, RuleDiagnostic, context::RuleContext, declare_lint_rule};
33
use biome_console::markup;
44
use biome_diagnostics::Severity;
55
use biome_js_factory::make;
6+
use biome_js_semantic::{BindingExtensions, SemanticModel};
67
use biome_js_syntax::{
7-
AnyJsExpression, AnyJsMemberExpression, AnyJsName, JsLogicalExpression, JsLogicalOperator,
8-
OperatorPrecedence, T,
8+
AnyJsExpression, AnyJsMemberExpression, AnyJsName, JsBinaryExpression, JsBinaryOperator,
9+
JsLogicalExpression, JsLogicalOperator, JsUnaryOperator, OperatorPrecedence, T,
910
};
1011
use biome_rowan::{AstNode, AstNodeExt, BatchMutationExt, SyntaxResult};
1112
use biome_rule_options::use_optional_chain::UseOptionalChainOptions;
@@ -14,6 +15,7 @@ use std::collections::VecDeque;
1415
use std::iter;
1516

1617
use crate::JsRuleAction;
18+
use crate::services::semantic::Semantic;
1719

1820
declare_lint_rule! {
1921
/// Enforce using concise optional chain instead of chained logical expressions.
@@ -89,22 +91,24 @@ pub enum UseOptionalChainState {
8991
}
9092

9193
impl Rule for UseOptionalChain {
92-
type Query = Ast<JsLogicalExpression>;
94+
type Query = Semantic<JsLogicalExpression>;
9395
type State = UseOptionalChainState;
9496
type Signals = Option<Self::State>;
9597
type Options = UseOptionalChainOptions;
9698

9799
fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
100+
let model = ctx.model();
98101
let logical = ctx.query();
99102
let operator = logical.operator().ok()?;
100103
match operator {
101104
JsLogicalOperator::LogicalAnd => {
102105
let head = logical.right().ok()?;
103106
let chain = LogicalAndChain::from_expression(head).ok()?;
104-
if chain.is_inside_another_chain().ok()? {
107+
if chain.is_inside_another_chain(model).ok()? {
105108
return None;
106109
}
107-
let optional_chain_expression_nodes = chain.optional_chain_expression_nodes()?;
110+
let optional_chain_expression_nodes =
111+
chain.optional_chain_expression_nodes(model)?;
108112
Some(UseOptionalChainState::LogicalAnd(
109113
optional_chain_expression_nodes,
110114
))
@@ -262,15 +266,169 @@ impl Rule for UseOptionalChain {
262266

263267
/// Normalize optional chain like.
264268
/// E.g. `foo != null` is normalized to `foo`
265-
fn normalized_optional_chain_like(expression: AnyJsExpression) -> SyntaxResult<AnyJsExpression> {
269+
fn normalized_optional_chain_like(
270+
expression: AnyJsExpression,
271+
model: &SemanticModel,
272+
) -> SyntaxResult<AnyJsExpression> {
266273
if let AnyJsExpression::JsBinaryExpression(binary_expression) = &expression
267-
&& let Some(expr) = binary_expression.extract_optional_chain_like()?
274+
&& let Some(expr) = extract_optional_chain_like(binary_expression, model)?
268275
{
269276
return Ok(expr);
270277
}
271278
Ok(expression)
272279
}
273280

281+
/// Extract the left or right operand of an optional chain-like expression.
282+
/// ```js
283+
/// foo !== undefined;
284+
/// typeof foo !== 'undefined';
285+
///```
286+
pub fn extract_optional_chain_like(
287+
binary: &JsBinaryExpression,
288+
model: &SemanticModel,
289+
) -> SyntaxResult<Option<AnyJsExpression>> {
290+
if matches!(
291+
binary.operator(),
292+
Ok(JsBinaryOperator::StrictInequality | JsBinaryOperator::Inequality)
293+
) {
294+
let left = binary.left()?;
295+
let right = binary.right()?;
296+
// nullish check: `foo !== undefined` -> return foo
297+
if let Some(expr) = extract_optional_chain_like_nullish(&left, &right)? {
298+
return Ok(Some(expr));
299+
}
300+
// typeof check: `typeof foo !== 'undefined'` -> return foo
301+
if let Some(expr) = extract_optional_chain_like_typeof(&left, &right, model)? {
302+
return Ok(Some(expr));
303+
}
304+
Ok(None)
305+
} else {
306+
Ok(None)
307+
}
308+
}
309+
310+
/// Extract the left or right operand of an optional chain-like expression comparing nullish.
311+
/// ```js
312+
/// foo !== undefined; // -> Some(foo)
313+
/// foo != undefined; // -> Some(foo)
314+
/// foo !== null; // -> Some(foo)
315+
/// foo != null; // -> Some(foo)
316+
/// undefined !== foo; // -> Some(foo)
317+
/// undefined != foo; // -> Some(foo)
318+
/// null !== foo; // -> Some(foo)
319+
/// null != foo; // -> Some(foo)
320+
/// foo !== bar; // -> None
321+
/// foo != bar; // -> None
322+
/// undefined !== null; // -> None
323+
/// undefined != null; // -> None
324+
/// null !== undefined; // -> None
325+
/// null != undefined; // -> None
326+
/// undefined !== undefined; // -> None
327+
/// undefined != undefined; // -> None
328+
/// null !== null; // -> None
329+
/// null != null; // -> None
330+
///```
331+
fn extract_optional_chain_like_nullish(
332+
left: &AnyJsExpression,
333+
right: &AnyJsExpression,
334+
) -> SyntaxResult<Option<AnyJsExpression>> {
335+
fn is_nullish(expression: &AnyJsExpression) -> bool {
336+
expression
337+
.as_static_value()
338+
.is_some_and(|x| x.is_null_or_undefined())
339+
}
340+
let left_is_nullish = is_nullish(left);
341+
let right_is_nullish = is_nullish(right);
342+
// right only nullish: `foo !== undefined` -> return foo (left)
343+
if !left_is_nullish && right_is_nullish {
344+
return Ok(Some(left.clone()));
345+
}
346+
// left only nullish: `undefined !== foo` -> return foo (right)
347+
if left_is_nullish && !right_is_nullish {
348+
return Ok(Some(right.clone()));
349+
}
350+
Ok(None)
351+
}
352+
353+
/// Extract the left or right operand of an optional chain-like expression using `typeof`.
354+
/// ```js
355+
/// typeof foo !== 'undefined'; // -> Some(foo)
356+
/// typeof foo != 'undefined'; // -> Some(foo)
357+
/// 'undefined' !== typeof foo; // -> Some(foo)
358+
/// 'undefined' != typeof foo; // -> Some(foo)
359+
/// ”undefined” != typeof foo; // -> Some(foo)
360+
/// `undefined` != typeof foo; // -> Some(foo)
361+
/// typeof foo !== undefined; // -> None
362+
/// typeof foo != undefined; // -> None
363+
/// undefined !== typeof foo; // -> None
364+
/// undefined != typeof foo; // -> None
365+
///```
366+
fn extract_optional_chain_like_typeof(
367+
left: &AnyJsExpression,
368+
right: &AnyJsExpression,
369+
model: &SemanticModel,
370+
) -> SyntaxResult<Option<AnyJsExpression>> {
371+
fn is_string_literal_undefined(expression: &AnyJsExpression) -> bool {
372+
expression
373+
.as_static_value()
374+
.is_some_and(|x| matches!(x.as_string_constant(), Some(s) if s == "undefined"))
375+
}
376+
fn typeof_argument(expression: &AnyJsExpression) -> SyntaxResult<Option<AnyJsExpression>> {
377+
if let Some(unary) = expression.as_js_unary_expression() {
378+
return Ok(match unary.operator()? {
379+
JsUnaryOperator::Typeof => Some(unary.argument()?.omit_parentheses()),
380+
_ => None,
381+
});
382+
}
383+
Ok(None)
384+
}
385+
fn is_unbound_root(model: &SemanticModel, expr: &AnyJsExpression) -> SyntaxResult<bool> {
386+
let mut current = expr.clone().omit_parentheses();
387+
loop {
388+
current = match current {
389+
AnyJsExpression::JsStaticMemberExpression(e) => e.object()?,
390+
AnyJsExpression::JsComputedMemberExpression(e) => e.object()?,
391+
AnyJsExpression::JsCallExpression(e) => e.callee()?,
392+
AnyJsExpression::JsParenthesizedExpression(e) => e.expression()?,
393+
_ => {
394+
break;
395+
}
396+
}
397+
}
398+
Ok(match current.as_js_reference_identifier() {
399+
Some(ident) => ident.binding(model).is_none(),
400+
None => false,
401+
})
402+
}
403+
let left_is_string_undefined = is_string_literal_undefined(left);
404+
let right_is_string_undefined = is_string_literal_undefined(right);
405+
// `typeof foo !== "undefined"` -> return foo
406+
if !left_is_string_undefined && right_is_string_undefined {
407+
let arg = typeof_argument(left);
408+
// Unbound references are treated as global references and are not subject to optional chaining.
409+
// `typeof window !== "undefined"` should not be converted to `window?.foo`
410+
if let Ok(Some(arg)) = &arg
411+
&& is_unbound_root(model, arg)?
412+
{
413+
return Ok(None);
414+
}
415+
return arg;
416+
}
417+
// `"undefined" !== typeof foo` -> return foo
418+
if left_is_string_undefined && !right_is_string_undefined {
419+
let arg = typeof_argument(right);
420+
// Unbound references are treated as global references and are not subject to optional chaining.
421+
// `"undefined" !== typeof window` should not be converted to `window?.foo`
422+
if let Ok(Some(arg)) = &arg
423+
&& is_unbound_root(model, arg)?
424+
{
425+
return Ok(None);
426+
}
427+
return arg;
428+
}
429+
Ok(None)
430+
}
431+
274432
/// `LogicalAndChainOrdering` is the result of a comparison between two logical
275433
/// AND chains.
276434
enum LogicalAndChainOrdering {
@@ -411,7 +569,7 @@ impl LogicalAndChain {
411569

412570
/// This function checks if `LogicalAndChain` is inside another parent
413571
/// `LogicalAndChain` and the chain is a sub-chain of the parent chain.
414-
fn is_inside_another_chain(&self) -> SyntaxResult<bool> {
572+
fn is_inside_another_chain(&self, model: &SemanticModel) -> SyntaxResult<bool> {
415573
// Because head of the chain is right expression of logical expression
416574
// we need to take a parent and a grand-parent.
417575
// E.g. `foo && foo.bar && foo.bar.baz`
@@ -429,8 +587,9 @@ impl LogicalAndChain {
429587
// Here we check that we came from the left side of the logical expression.
430588
// Because only the left-hand parts can be sub-chains.
431589
if grand_parent_logical_left.as_js_logical_expression() == Some(&parent) {
432-
let grand_parent_right_chain =
433-
Self::from_expression(normalized_optional_chain_like(grand_parent.right()?)?)?;
590+
let grand_parent_right_chain = Self::from_expression(
591+
normalized_optional_chain_like(grand_parent.right()?, model)?,
592+
)?;
434593
let result = grand_parent_right_chain.cmp_chain(self)?;
435594
return match result {
436595
LogicalAndChainOrdering::SubChain | LogicalAndChainOrdering::Equal => Ok(true),
@@ -539,7 +698,10 @@ impl LogicalAndChain {
539698

540699
/// This function returns a list of `JsAnyExpression` which we need to
541700
/// transform into an optional chain expression.
542-
fn optional_chain_expression_nodes(mut self) -> Option<VecDeque<AnyJsExpression>> {
701+
fn optional_chain_expression_nodes(
702+
mut self,
703+
model: &SemanticModel,
704+
) -> Option<VecDeque<AnyJsExpression>> {
543705
let mut optional_chain_expression_nodes = VecDeque::with_capacity(self.buf.len());
544706
// Take a head of a next sub-chain
545707
// E.g. `foo && foo.bar && foo.bar.baz`
@@ -562,7 +724,7 @@ impl LogicalAndChain {
562724
// foo && foo.bar;
563725
// ```
564726
AnyJsExpression::JsBinaryExpression(expression) => {
565-
expression.extract_optional_chain_like().ok()??
727+
extract_optional_chain_like(&expression, model).ok()??
566728
}
567729
expression => expression,
568730
};
@@ -582,7 +744,8 @@ impl LogicalAndChain {
582744
| AnyJsExpression::JsCallExpression(_) => expression,
583745
_ => return None,
584746
};
585-
let branch = Self::from_expression(normalized_optional_chain_like(head).ok()?).ok()?;
747+
let branch =
748+
Self::from_expression(normalized_optional_chain_like(head, model).ok()?).ok()?;
586749
match self.cmp_chain(&branch).ok()? {
587750
LogicalAndChainOrdering::SubChain => {
588751
// If the previous branch had other expressions that already

‎crates/biome_js_analyze/tests/specs/complexity/useOptionalChain/global_typeofLogicalAndCases1.ts‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
// chained members
2+
typeof foo !== 'undefined' && foo.bar;
3+
typeof foo.bar !== 'undefined' && foo.bar.baz;
4+
typeof foo !== 'undefined' && foo();
5+
typeof foo.bar !== 'undefined' && foo.bar();
6+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && foo.bar.baz.buzz;
7+
typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && foo.bar.baz.buzz;
8+
9+
// case with a jump (i.e. a non-'undefined'ish prop)
10+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && foo.bar.baz.buzz;
11+
typeof foo.bar !== 'undefined' && foo.bar.baz.buzz;
12+
13+
// case where for some reason there is a doubled up expression
14+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && typeof foo.bar.baz !== 'undefined' && foo.bar.baz.buzz;
15+
typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && typeof foo.bar.baz !== 'undefined' && foo.bar.baz.buzz;
16+
17+
// chained members with element access
18+
typeof foo !== 'undefined' && typeof foo[bar] !== 'undefined' && typeof foo[bar].baz !== 'undefined' && foo[bar].baz.buzz;
19+
20+
// case with a jump (i.e. a non-'undefined'ish prop)
21+
typeof foo !== 'undefined' && typeof foo[bar].baz !== 'undefined' && foo[bar].baz.buzz;
22+
23+
// chained calls
24+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && foo.bar.baz.buzz();
25+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && typeof foo.bar.baz.buzz !== 'undefined' && foo.bar.baz.buzz();
26+
typeof foo.bar !== 'undefined' && typeof foo.bar.baz !== 'undefined' && typeof foo.bar.baz.buzz !== 'undefined' && foo.bar.baz.buzz();
27+
28+
// case with a jump (i.e. a non-'undefined'ish prop)
29+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && foo.bar.baz.buzz();
30+
typeof foo.bar !== 'undefined' && foo.bar.baz.buzz();
31+
32+
// case with a jump (i.e. a non-'undefined'ish prop)
33+
typeof foo !== 'undefined' && typeof foo.bar !== 'undefined' && typeof foo.bar.baz.buzz !== 'undefined' && foo.bar.baz.buzz();
34+
35+
// case with a call expr inside the chain for some inefficient reason
36+
typeof foo !== 'undefined' && typeof foo.bar() !== 'undefined' && typeof foo.bar().baz !== 'undefined' && typeof foo.bar().baz.buzz !== 'undefined' && foo.bar().baz.buzz();
37+
38+
39+
// chained members (double quotes)
40+
typeof foo !== "undefined" && foo.bar;
41+
typeof foo.bar !== "undefined" && foo.bar.baz;
42+
typeof foo !== "undefined" && foo();
43+
typeof foo.bar !== "undefined" && foo.bar();
44+
typeof foo !== "undefined" && typeof foo.bar !== "undefined" && typeof foo.bar.baz !== "undefined" && foo.bar.baz.buzz;
45+
typeof foo.bar !== "undefined" && typeof foo.bar.baz !== "undefined" && foo.bar.baz.buzz;
46+
47+
// chained members (backticks)
48+
typeof foo !== `undefined` && foo.bar;
49+
typeof foo.bar !== `undefined` && foo.bar.baz;
50+
typeof foo !== `undefined` && foo();
51+
typeof foo.bar !== `undefined` && foo.bar();
52+
typeof foo !== `undefined` && typeof foo.bar !== `undefined` && typeof foo.bar.baz !== `undefined` && foo.bar.baz.buzz;
53+
typeof foo.bar !== `undefined` && typeof foo.bar.baz !== `undefined` && foo.bar.baz.buzz;

0 commit comments

Comments
 (0)