From f6bff7d0aed53f7206fc9800938fabc1f3ed3e0e Mon Sep 17 00:00:00 2001 From: Arend van Beelen jr Date: Thu, 7 Dec 2023 12:16:21 +0100 Subject: [PATCH] fix(lint/useHookAtTopLevel): detect early returns before calls to hooks (#1018) --- .../correctness/use_hook_at_top_level.rs | 214 +++++++++++++++++- .../biome_js_analyze/src/semantic_services.rs | 4 +- .../correctness/useHookAtTopLevel/invalid.js | 36 +++ .../useHookAtTopLevel/invalid.js.snap | 163 +++++++++++++ .../correctness/useHookAtTopLevel/valid.js | 8 + .../useHookAtTopLevel/valid.js.snap | 8 + .../linter/rules/use-hook-at-top-level.md | 37 +++ 7 files changed, 461 insertions(+), 9 deletions(-) diff --git a/crates/biome_js_analyze/src/semantic_analyzers/correctness/use_hook_at_top_level.rs b/crates/biome_js_analyze/src/semantic_analyzers/correctness/use_hook_at_top_level.rs index c2d8d892fd74..1273ffbedfbb 100644 --- a/crates/biome_js_analyze/src/semantic_analyzers/correctness/use_hook_at_top_level.rs +++ b/crates/biome_js_analyze/src/semantic_analyzers/correctness/use_hook_at_top_level.rs @@ -1,12 +1,22 @@ +use crate::react::hooks::react_hook_configuration; use crate::semantic_analyzers::correctness::use_exhaustive_dependencies::{ HooksOptions, ReactExtensiveDependenciesOptions, }; -use crate::{react::hooks::react_hook_configuration, semantic_services::Semantic}; +use crate::semantic_services::{SemanticModelBuilderVisitor, SemanticServices}; use biome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic}; +use biome_analyze::{ + AddVisitor, FromServices, MissingServicesDiagnostic, Phase, Phases, QueryMatch, Queryable, + RuleKey, ServiceBag, Visitor, VisitorContext, VisitorFinishContext, +}; use biome_console::markup; -use biome_js_semantic::CallsExtensions; -use biome_js_syntax::{AnyJsFunction, JsCallExpression, JsFunctionBody, JsSyntaxKind, TextRange}; -use biome_rowan::AstNode; +use biome_js_semantic::{CallsExtensions, SemanticModel}; +use biome_js_syntax::{ + AnyFunctionLike, AnyJsFunction, JsCallExpression, JsFunctionBody, JsLanguage, + JsReturnStatement, JsSyntaxKind, TextRange, +}; +use biome_rowan::{AstNode, Language, SyntaxNode, WalkEvent}; +use rustc_hash::FxHashMap; +use std::ops::{Deref, DerefMut}; declare_rule! { /// Enforce that all React hooks are being called from the Top Level component functions. @@ -25,6 +35,16 @@ declare_rule! { /// } /// ``` /// + /// ```js,expect_diagnostic + /// function Component1({ a }) { + /// if (a != 1) { + /// return; + /// } + /// + /// useEffect(); + /// } + /// ``` + /// /// ## Valid /// /// ```js @@ -69,6 +89,7 @@ pub enum Suggestion { None { hook_name_range: TextRange, path: Vec, + early_return: Option, }, } @@ -100,6 +121,167 @@ fn enclosing_function_if_call_is_at_top_level(call: &JsCallExpression) -> Option .and_then(|body| body.parent::()) } +/// Model for tracking which function calls are preceeded by an early return. +/// +/// The keys in the model are call sites and each value is the text range of an +/// early return that preceeds such call site. Call sites without preceeding +/// early returns are not included in the model. For call sites that are +/// preceeded by multiple early returns, the return statement that we map to is +/// implementation-defined. +#[derive(Clone, Default)] +struct EarlyReturnsModel(FxHashMap); + +impl Deref for EarlyReturnsModel { + type Target = FxHashMap; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl DerefMut for EarlyReturnsModel { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } +} + +#[derive(Default)] +struct EarlyReturnDetectionVisitor { + early_returns: EarlyReturnsModel, + stack: Vec, +} + +#[derive(Default)] +struct EarlyReturnDetectionVisitorStackEntry { + early_return: Option, +} + +impl Visitor for EarlyReturnDetectionVisitor { + type Language = JsLanguage; + + fn visit( + &mut self, + event: &WalkEvent>, + _ctx: VisitorContext, + ) { + match event { + WalkEvent::Enter(node) => { + if AnyFunctionLike::can_cast(node.kind()) { + self.stack + .push(EarlyReturnDetectionVisitorStackEntry::default()); + } + } + WalkEvent::Leave(node) => { + if AnyFunctionLike::can_cast(node.kind()) { + self.stack.pop(); + return; + } + + if let Some(entry) = self.stack.last_mut() { + if JsReturnStatement::can_cast(node.kind()) { + entry.early_return = Some(node.text_range()); + } else if let Some(call) = JsCallExpression::cast_ref(node) { + if let Some(early_return) = entry.early_return { + self.early_returns.insert(call.clone(), early_return); + } + } + } + } + } + } + + fn finish(self: Box, ctx: VisitorFinishContext) { + ctx.services.insert_service(self.early_returns); + } +} + +#[derive(Default)] +struct FunctionCallVisitor; + +impl Visitor for FunctionCallVisitor { + type Language = JsLanguage; + + fn visit( + &mut self, + event: &WalkEvent>, + mut ctx: VisitorContext, + ) { + match event { + WalkEvent::Enter(_) => {} + WalkEvent::Leave(node) => { + if let Some(call) = JsCallExpression::cast_ref(node) { + ctx.match_query(FunctionCall(call)); + } + } + } + } +} + +pub struct FunctionCallServices { + early_returns: EarlyReturnsModel, + semantic_services: SemanticServices, +} + +impl FunctionCallServices { + fn early_returns_model(&self) -> &EarlyReturnsModel { + &self.early_returns + } + + fn semantic_model(&self) -> &SemanticModel { + self.semantic_services.model() + } +} + +impl FromServices for FunctionCallServices { + fn from_services( + rule_key: &RuleKey, + services: &ServiceBag, + ) -> Result { + let early_returns: &EarlyReturnsModel = services.get_service().ok_or_else(|| { + MissingServicesDiagnostic::new(rule_key.rule_name(), &["EarlyReturnsModel"]) + })?; + Ok(Self { + early_returns: early_returns.clone(), + semantic_services: SemanticServices::from_services(rule_key, services)?, + }) + } +} + +impl Phase for FunctionCallServices { + fn phase() -> Phases { + Phases::Semantic + } +} + +#[derive(Clone)] +pub(crate) struct FunctionCall(JsCallExpression); + +impl QueryMatch for FunctionCall { + fn text_range(&self) -> TextRange { + self.0.range() + } +} + +impl Queryable for FunctionCall { + type Input = Self; + type Language = JsLanguage; + type Output = Self; + type Services = FunctionCallServices; + + fn build_visitor( + analyzer: &mut impl AddVisitor, + root: &::Root, + ) { + analyzer.add_visitor(Phases::Syntax, || SemanticModelBuilderVisitor::new(root)); + analyzer.add_visitor(Phases::Syntax, EarlyReturnDetectionVisitor::default); + analyzer.add_visitor(Phases::Semantic, FunctionCallVisitor::default); + } + + fn unwrap_match(_services: &ServiceBag, query: &Self::Input) -> Self::Output { + query.clone() + } +} + #[derive(Debug)] pub struct CallPath { call: JsCallExpression, @@ -107,7 +289,7 @@ pub struct CallPath { } impl Rule for UseHookAtTopLevel { - type Query = Semantic; + type Query = FunctionCall; type State = Suggestion; type Signals = Option; type Options = HooksOptions; @@ -116,10 +298,11 @@ impl Rule for UseHookAtTopLevel { let options = ctx.options(); let options = ReactExtensiveDependenciesOptions::new(options.clone()); - let call = ctx.query(); + let FunctionCall(call) = ctx.query(); let hook_name_range = call.callee().ok()?.syntax().text_trimmed_range(); if react_hook_configuration(call, &options.hooks_config).is_some() { - let model = ctx.model(); + let model = ctx.semantic_model(); + let early_returns = ctx.early_returns_model(); let root = CallPath { call: call.clone(), @@ -135,6 +318,14 @@ impl Rule for UseHookAtTopLevel { if let Some(enclosing_function) = enclosing_function_if_call_is_at_top_level(&call) { + if let Some(early_return) = early_returns.get(&call) { + return Some(Suggestion::None { + hook_name_range, + path, + early_return: Some(*early_return), + }); + } + if let Some(calls_iter) = enclosing_function.all_calls(model) { for call in calls_iter { calls.push(CallPath { @@ -147,6 +338,7 @@ impl Rule for UseHookAtTopLevel { return Some(Suggestion::None { hook_name_range, path, + early_return: None, }); } } @@ -160,6 +352,7 @@ impl Rule for UseHookAtTopLevel { Suggestion::None { hook_name_range, path, + early_return, } => { let call_deep = path.len() - 1; @@ -193,6 +386,13 @@ impl Rule for UseHookAtTopLevel { diag = diag.detail(range, msg); } + if let Some(range) = early_return { + diag = diag.detail( + range, + markup! { "Hooks should not be called after an early return." }, + ) + } + let diag = diag.note( markup! { "For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order." diff --git a/crates/biome_js_analyze/src/semantic_services.rs b/crates/biome_js_analyze/src/semantic_services.rs index fc4e591cf3b4..d958ed646f47 100644 --- a/crates/biome_js_analyze/src/semantic_services.rs +++ b/crates/biome_js_analyze/src/semantic_services.rs @@ -86,13 +86,13 @@ where } } -struct SemanticModelBuilderVisitor { +pub(crate) struct SemanticModelBuilderVisitor { extractor: SemanticEventExtractor, builder: SemanticModelBuilder, } impl SemanticModelBuilderVisitor { - fn new(root: &AnyJsRoot) -> Self { + pub(crate) fn new(root: &AnyJsRoot) -> Self { Self { extractor: SemanticEventExtractor::default(), builder: SemanticModelBuilder::new(root.clone()), diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js index a18ff75c6ac0..671e87fcfd5f 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js @@ -90,3 +90,39 @@ const Component9 = () => { a ? useEffect() : null; a ?? useEffect(); }; + +function Component10() { + return; + + useEffect(); +} + +function Component11() { + if (!a) { + return; + } + + useEffect(); +} + +function Component12() { + if (!a) { + return; + } + + { + useEffect(); + } +} + +function Component13() { + useEffect(); +}; + +function Component14() { + if (!a) { + return; + } + + Component13(); +} diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap index d0c4a735b24a..c1fed0584826 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/invalid.js.snap @@ -97,6 +97,42 @@ const Component9 = () => { a ?? useEffect(); }; +function Component10() { + return; + + useEffect(); +} + +function Component11() { + if (!a) { + return; + } + + useEffect(); +} + +function Component12() { + if (!a) { + return; + } + + { + useEffect(); + } +} + +function Component13() { + useEffect(); +}; + +function Component14() { + if (!a) { + return; + } + + Component13(); +} + ``` # Diagnostics @@ -466,4 +502,131 @@ invalid.js:91:10 lint/correctness/useHookAtTopLevel ━━━━━━━━━ ``` +``` +invalid.js:97:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. + + 95 │ return; + 96 │ + > 97 │ useEffect(); + │ ^^^^^^^^^ + 98 │ } + 99 │ + + i Hooks should not be called after an early return. + + 92 │ }; + 93 │ + > 94 │ function Component10() { + │ + > 95 │ return; + │ ^^^^^^^ + 96 │ + 97 │ useEffect(); + + i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. + + i See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level + + +``` + +``` +invalid.js:105:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. + + 103 │ } + 104 │ + > 105 │ useEffect(); + │ ^^^^^^^^^ + 106 │ } + 107 │ + + i Hooks should not be called after an early return. + + 100 │ function Component11() { + > 101 │ if (!a) { + │ + > 102 │ return; + │ ^^^^^^^ + 103 │ } + 104 │ + + i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. + + i See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level + + +``` + +``` +invalid.js:114:9 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This hook is being called conditionally, but all hooks must be called in the exact same order in every component render. + + 113 │ { + > 114 │ useEffect(); + │ ^^^^^^^^^ + 115 │ } + 116 │ } + + i Hooks should not be called after an early return. + + 108 │ function Component12() { + > 109 │ if (!a) { + │ + > 110 │ return; + │ ^^^^^^^ + 111 │ } + 112 │ + + i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. + + i See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level + + +``` + +``` +invalid.js:119:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render. + + 118 │ function Component13() { + > 119 │ useEffect(); + │ ^^^^^^^^^ + 120 │ }; + 121 │ + + i This is the call path until the hook. + + 123 │ if (!a) { + 124 │ return; + > 125 │ } + │ + > 126 │ + > 127 │ Component13(); + │ ^^^^^^^^^^^^^ + 128 │ } + 129 │ + + i Hooks should not be called after an early return. + + 122 │ function Component14() { + > 123 │ if (!a) { + │ + > 124 │ return; + │ ^^^^^^^ + 125 │ } + 126 │ + + i For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order. + + i See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level + + +``` + diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js index e2a673cf5302..e45d1a0be293 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js @@ -40,3 +40,11 @@ const Component7 = () => { const value = useRef().value; const [_val, _setter] = useState(useMemo('hello')); } + +function Component8() { + const a = () => { + return; + }; + + useEffect(); +}; diff --git a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js.snap b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js.snap index 73138718f5fe..25e14fa6fb5f 100644 --- a/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js.snap +++ b/crates/biome_js_analyze/tests/specs/correctness/useHookAtTopLevel/valid.js.snap @@ -47,6 +47,14 @@ const Component7 = () => { const [_val, _setter] = useState(useMemo('hello')); } +function Component8() { + const a = () => { + return; + }; + + useEffect(); +}; + ``` diff --git a/website/src/content/docs/linter/rules/use-hook-at-top-level.md b/website/src/content/docs/linter/rules/use-hook-at-top-level.md index 466628f11098..04808b1f05b6 100644 --- a/website/src/content/docs/linter/rules/use-hook-at-top-level.md +++ b/website/src/content/docs/linter/rules/use-hook-at-top-level.md @@ -37,6 +37,43 @@ function Component1({ a }) { +```jsx +function Component1({ a }) { + if (a != 1) { + return; + } + + useEffect(); +} +``` + +
correctness/useHookAtTopLevel.js:6:5 lint/correctness/useHookAtTopLevel ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
+  
+    4 │     }
+    5 │ 
+  > 6 │     useEffect();
+      │     ^^^^^^^^^
+    7 │ }
+    8 │ 
+  
+  ℹ Hooks should not be called after an early return.
+  
+    1 │ function Component1({ a }) {
+  > 2 │     if (a != 1) {
+      │                  
+  > 3 │         return;
+      │         ^^^^^^^
+    4 │     }
+    5 │ 
+  
+  ℹ For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
+  
+  ℹ See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
+  
+
+ ## Valid ```jsx