diff --git a/.changeset/dry-files-sniff.md b/.changeset/dry-files-sniff.md new file mode 100644 index 000000000000..8c982bfcdca9 --- /dev/null +++ b/.changeset/dry-files-sniff.md @@ -0,0 +1,23 @@ +--- +"@biomejs/biome": patch +--- + +Fixed [#8802](https://github.com/biomejs/biome/issues/8802): `useExhaustiveDependencies` now correctly suggests dependencies without including callback-scoped variables or method names. + +When accessing object properties with a callback-scoped variable, only the object path is suggested: + +```js +// Now correctly suggests `props.value` instead of `props.value[day]` +useMemo(() => { + return WeekdayValues.filter((day) => props.value[day]); +}, [props.value]); +``` + +When calling methods on objects, only the object is suggested as a dependency: + +```js +// Now correctly suggests `props.data` instead of `props.data.forEach` +useMemo(() => { + props.data.forEach((item) => console.log(item)); +}, [props.data]); +``` diff --git a/.changeset/ninety-tigers-fetch.md b/.changeset/ninety-tigers-fetch.md new file mode 100644 index 000000000000..d926e0160c4a --- /dev/null +++ b/.changeset/ninety-tigers-fetch.md @@ -0,0 +1,16 @@ +--- +"@biomejs/biome": patch +--- + +Fixed [#8883](https://github.com/biomejs/biome/issues/8883): `useExhaustiveDependencies` no longer produces false positives when props are destructured in the function body of arrow function components without parentheses around the parameter. + +```tsx +type Props = { msg: string }; + +// Arrow function without parentheses around `props` +const Component: React.FC = props => { + const { msg } = props; + // Previously, this incorrectly reported `msg` as unnecessary + useEffect(() => console.log(msg), [msg]); +}; +``` diff --git a/.changeset/tame-eggs-judge.md b/.changeset/tame-eggs-judge.md new file mode 100644 index 000000000000..3899acb21eaa --- /dev/null +++ b/.changeset/tame-eggs-judge.md @@ -0,0 +1,19 @@ +--- +"@biomejs/biome": patch +--- + +Fixed [#8885](https://github.com/biomejs/biome/issues/8885): `useExhaustiveDependencies` no longer incorrectly reports variables as unnecessary dependencies when they are derived from expressions containing post/pre-increment operators (`++`/`--`) or compound assignment operators (`+=`, `-=`, etc.). + +```js +let renderCount = 0; + +export const MyComponent = () => { + // `count` is now correctly recognized as a required dependency + // because `renderCount++` can produce different values between renders + const count = renderCount++; + + useEffect(() => { + console.log(count); + }, [count]); // no longer reports `count` as unnecessary +}; +``` diff --git a/crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs b/crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs index 0d48534bc368..9201c8b1b86b 100644 --- a/crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs +++ b/crates/biome_js_analyze/src/lint/correctness/use_exhaustive_dependencies.rs @@ -7,14 +7,15 @@ use biome_analyze::{Rule, RuleDiagnostic, RuleDomain, context::RuleContext, decl use biome_console::markup; use biome_diagnostics::Severity; use biome_js_factory::make; -use biome_js_semantic::{CanBeImportedExported, ClosureExtensions, SemanticModel}; +use biome_js_semantic::{CanBeImportedExported, ClosureExtensions, SemanticModel, is_constant}; use biome_js_syntax::binding_ext::AnyJsIdentifierBinding; use biome_js_syntax::{ - AnyJsArrayElement, AnyJsExpression, AnyJsMemberExpression, AnyJsObjectBindingPatternMember, - JsArrayBindingPattern, JsArrayBindingPatternElement, JsArrayBindingPatternElementList, - JsArrayExpression, JsComputedMemberExpression, JsObjectBindingPattern, - JsObjectBindingPatternPropertyList, JsReferenceIdentifier, JsVariableDeclarator, T, - TsTypeofType, is_transparent_expression_wrapper, + AnyJsArrayElement, AnyJsArrowFunctionParameters, AnyJsBinding, AnyJsExpression, + AnyJsMemberExpression, AnyJsObjectBindingPatternMember, JsArrayBindingPattern, + JsArrayBindingPatternElement, JsArrayBindingPatternElementList, JsArrayExpression, + JsComputedMemberExpression, JsObjectBindingPattern, JsObjectBindingPatternPropertyList, + JsReferenceIdentifier, JsVariableDeclarator, T, TsTypeofType, + is_transparent_expression_wrapper, }; use biome_js_syntax::{ JsCallExpression, JsSyntaxKind, JsSyntaxNode, JsVariableDeclaration, TextRange, @@ -540,8 +541,8 @@ fn get_expression_candidates(node: JsSyntaxNode) -> Vec parent.kind(), JsSyntaxKind::JS_SHORTHAND_PROPERTY_OBJECT_MEMBER ) { - if let Some(sequence_expression) = AnyExpressionCandidate::cast_ref(&prev_node) { - result.push(sequence_expression.clone()); + if let Some(expression) = AnyExpressionCandidate::cast_ref(&prev_node) { + result.push(expression.clone()); } return result; } @@ -557,7 +558,27 @@ fn get_expression_candidates(node: JsSyntaxNode) -> Vec if let Some(computed_member_expression) = JsComputedMemberExpression::cast_ref(&parent) && let Ok(object) = computed_member_expression.object() - && !prev_node.eq(object.syntax()) + { + if !prev_node.eq(object.syntax()) { + return result; + } + // collect only constant member access expressions + if let Ok(member) = computed_member_expression.member() + && !is_constant(&member) + { + return result; + } + } + + // Follow eslint React plugin behavior: + // When calling a method of an object, only the object should be included in the dependency list. + if matches!( + parent.kind(), + JsSyntaxKind::JS_STATIC_MEMBER_EXPRESSION | JsSyntaxKind::JS_COMPUTED_MEMBER_EXPRESSION + ) && let Some(wrapper) = parent.parent() + && let Some(call_expression) = JsCallExpression::cast(wrapper) + && let Ok(callee) = call_expression.callee() + && callee.syntax().eq(&parent) { return result; } @@ -849,8 +870,20 @@ fn is_stable_expression( && let Some(binding) = model.binding(&name) { let binding = &binding.tree(); - if let Some(declaration_node) = - &binding.declaration().map(|decl| decl.syntax().clone()) + let declaration = binding.declaration(); + let declaration_node = if let Some( + AnyJsBindingDeclaration::JsArrowFunctionExpression(arrow_function), + ) = &declaration + && let Ok(AnyJsArrowFunctionParameters::AnyJsBinding( + AnyJsBinding::JsIdentifierBinding(identifier), + )) = arrow_function.parameters() + && identifier.syntax().eq(binding.syntax()) + { + Some(identifier.syntax().clone()) + } else { + declaration.map(|decl| decl.syntax().clone()) + }; + if let Some(declaration_node) = &declaration_node && identifier .syntax() .ancestors() diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts new file mode 100644 index 000000000000..81dac2f4b70d --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts @@ -0,0 +1,31 @@ +import { useMemo } from "react"; + +export interface Days { + mon: boolean; + tue: boolean; + wed: boolean; + thu: boolean; + fri: boolean; +} + +const WeekdayValues: (keyof Days)[] = ["mon", "tue", "wed", "thu", "fri"]; + +// "day" doesn't exist outside the memoized function. +// Biome should report "props.value" as a missing dependency, +// NOT "props.value[day]" since "day" is a callback-scoped variable. +function Component1(props: { value: Days }) { + useMemo(() => { + return WeekdayValues.filter((day) => props.value[day]); + }, []); +} + +// Example with forEach and index +// Biome should report "props.data" as missing dependency, +// NOT "props.data.forEach" to follow eslint React plugin behavior. +function Component2(props: { data: number[] }) { + useMemo(() => { + props.data.forEach((value, index) => { + console.log(value, index); + }); + }, []); +} diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts.snap b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts.snap new file mode 100644 index 000000000000..fedd8ea9ed78 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8802.ts.snap @@ -0,0 +1,109 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 +expression: issue8802.ts +--- +# Input +```ts +import { useMemo } from "react"; + +export interface Days { + mon: boolean; + tue: boolean; + wed: boolean; + thu: boolean; + fri: boolean; +} + +const WeekdayValues: (keyof Days)[] = ["mon", "tue", "wed", "thu", "fri"]; + +// "day" doesn't exist outside the memoized function. +// Biome should report "props.value" as a missing dependency, +// NOT "props.value[day]" since "day" is a callback-scoped variable. +function Component1(props: { value: Days }) { + useMemo(() => { + return WeekdayValues.filter((day) => props.value[day]); + }, []); +} + +// Example with forEach and index +// Biome should report "props.data" as missing dependency, +// NOT "props.data.forEach" to follow eslint React plugin behavior. +function Component2(props: { data: number[] }) { + useMemo(() => { + props.data.forEach((value, index) => { + console.log(value, index); + }); + }, []); +} + +``` + +# Diagnostics +``` +issue8802.ts:17:5 lint/correctness/useExhaustiveDependencies FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This hook does not specify its dependency on props.value. + + 15 │ // NOT "props.value[day]" since "day" is a callback-scoped variable. + 16 │ function Component1(props: { value: Days }) { + > 17 │ useMemo(() => { + │ ^^^^^^^ + 18 │ return WeekdayValues.filter((day) => props.value[day]); + 19 │ }, []); + + i This dependency is being used here, but is not specified in the hook dependency list. + + 16 │ function Component1(props: { value: Days }) { + 17 │ useMemo(() => { + > 18 │ return WeekdayValues.filter((day) => props.value[day]); + │ ^^^^^^^^^^^ + 19 │ }, []); + 20 │ } + + i React relies on hook dependencies to determine when to re-compute Effects. + Failing to specify dependencies can result in Effects not updating correctly when state changes. + These "stale closures" are a common source of surprising bugs. + + i Either include it or remove the dependency array. + + i Unsafe fix: Add the missing dependency to the list. + + 19 │ ····},·[props.value]); + │ +++++++++++ + +``` + +``` +issue8802.ts:26:5 lint/correctness/useExhaustiveDependencies FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This hook does not specify its dependency on props.data. + + 24 │ // NOT "props.data.forEach" to follow eslint React plugin behavior. + 25 │ function Component2(props: { data: number[] }) { + > 26 │ useMemo(() => { + │ ^^^^^^^ + 27 │ props.data.forEach((value, index) => { + 28 │ console.log(value, index); + + i This dependency is being used here, but is not specified in the hook dependency list. + + 25 │ function Component2(props: { data: number[] }) { + 26 │ useMemo(() => { + > 27 │ props.data.forEach((value, index) => { + │ ^^^^^^^^^^ + 28 │ console.log(value, index); + 29 │ }); + + i React relies on hook dependencies to determine when to re-compute Effects. + Failing to specify dependencies can result in Effects not updating correctly when state changes. + These "stale closures" are a common source of surprising bugs. + + i Either include it or remove the dependency array. + + i Unsafe fix: Add the missing dependency to the list. + + 30 │ ····},·[props.data]); + │ ++++++++++ + +``` diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx new file mode 100644 index 000000000000..9f488824371d --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx @@ -0,0 +1,19 @@ +import React, { useEffect } from 'react' + +type ExampleProps = { + msg: string; +}; + +const DestructuredInParams: React.FC = ({ msg }) => { + useEffect(() => console.log(msg), [msg]); // correct behavior +}; + +const DestructuredInBody1: React.FC = props => { + const { msg } = props; + useEffect(() => console.log(msg), [msg]); // should NOT trigger useExhaustiveDependencies +}; + +const DestructuredInBody2: React.FC = props => { + const { msg } = props; + useEffect(() => console.log(msg), []); // should trigger useExhaustiveDependencies +}; diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx.snap b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx.snap new file mode 100644 index 000000000000..92b31d5d39b3 --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8883.tsx.snap @@ -0,0 +1,63 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 +expression: issue8883.tsx +--- +# Input +```tsx +import React, { useEffect } from 'react' + +type ExampleProps = { + msg: string; +}; + +const DestructuredInParams: React.FC = ({ msg }) => { + useEffect(() => console.log(msg), [msg]); // correct behavior +}; + +const DestructuredInBody1: React.FC = props => { + const { msg } = props; + useEffect(() => console.log(msg), [msg]); // should NOT trigger useExhaustiveDependencies +}; + +const DestructuredInBody2: React.FC = props => { + const { msg } = props; + useEffect(() => console.log(msg), []); // should trigger useExhaustiveDependencies +}; + +``` + +# Diagnostics +``` +issue8883.tsx:18:5 lint/correctness/useExhaustiveDependencies FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × This hook does not specify its dependency on msg. + + 16 │ const DestructuredInBody2: React.FC = props => { + 17 │ const { msg } = props; + > 18 │ useEffect(() => console.log(msg), []); // should trigger useExhaustiveDependencies + │ ^^^^^^^^^ + 19 │ }; + 20 │ + + i This dependency is being used here, but is not specified in the hook dependency list. + + 16 │ const DestructuredInBody2: React.FC = props => { + 17 │ const { msg } = props; + > 18 │ useEffect(() => console.log(msg), []); // should trigger useExhaustiveDependencies + │ ^^^ + 19 │ }; + 20 │ + + i React relies on hook dependencies to determine when to re-compute Effects. + Failing to specify dependencies can result in Effects not updating correctly when state changes. + These "stale closures" are a common source of surprising bugs. + + i Either include it or remove the dependency array. + + i Unsafe fix: Add the missing dependency to the list. + + 18 │ ····useEffect(()·=>·console.log(msg),·[msg]);·//·should·trigger·useExhaustiveDependencies + │ +++ + +``` diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js new file mode 100644 index 000000000000..29f74513009f --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js @@ -0,0 +1,27 @@ +/* should not generate diagnostics */ +import { useEffect } from "react"; + +let renderCount = 0; + +// This should not report `count` as unnecessary dependency. +// `count` is derived from a global variable and can change between renders. +export const MyComponent = () => { + const count = renderCount++; + + useEffect(() => { + console.log(count); + }, [count]); +}; + +// Reading from a global variable without modification is considered stable. +let globalConfig = { debug: false }; + +export const Component2 = () => { + const debug = globalConfig.debug; + + useEffect(() => { + if (debug) { + console.log("Debug mode enabled"); + } + }, []); +}; diff --git a/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js.snap b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js.snap new file mode 100644 index 000000000000..55806285a7cb --- /dev/null +++ b/crates/biome_js_analyze/tests/specs/correctness/useExhaustiveDependencies/issue8885.js.snap @@ -0,0 +1,36 @@ +--- +source: crates/biome_js_analyze/tests/spec_tests.rs +assertion_line: 152 +expression: issue8885.js +--- +# Input +```js +/* should not generate diagnostics */ +import { useEffect } from "react"; + +let renderCount = 0; + +// This should not report `count` as unnecessary dependency. +// `count` is derived from a global variable and can change between renders. +export const MyComponent = () => { + const count = renderCount++; + + useEffect(() => { + console.log(count); + }, [count]); +}; + +// Reading from a global variable without modification is considered stable. +let globalConfig = { debug: false }; + +export const Component2 = () => { + const debug = globalConfig.debug; + + useEffect(() => { + if (debug) { + console.log("Debug mode enabled"); + } + }, []); +}; + +``` diff --git a/crates/biome_js_semantic/src/semantic_model/is_constant.rs b/crates/biome_js_semantic/src/semantic_model/is_constant.rs index 8c2a0cea0954..b481990431e6 100644 --- a/crates/biome_js_semantic/src/semantic_model/is_constant.rs +++ b/crates/biome_js_semantic/src/semantic_model/is_constant.rs @@ -3,7 +3,20 @@ use biome_rowan::AstNode; pub fn is_constant(expr: &AnyJsExpression) -> bool { for node in expr.syntax().descendants() { - if matches!(node.kind(), JsSyntaxKind::JS_REFERENCE_IDENTIFIER) { + if matches!( + node.kind(), + JsSyntaxKind::JS_REFERENCE_IDENTIFIER + | JsSyntaxKind::JS_POST_UPDATE_EXPRESSION + | JsSyntaxKind::JS_PRE_UPDATE_EXPRESSION + ) { + return false; + } + if matches!(node.kind(), JsSyntaxKind::JS_ASSIGNMENT_EXPRESSION) + && !node + .children_with_tokens() + .nth(1) + .is_some_and(|child| matches!(child.kind(), JsSyntaxKind::EQ)) + { return false; } } @@ -47,4 +60,32 @@ mod tests { assert_is_const("const a = `${a}`;", false); assert_is_const("const a = b = 1 + f();", false); } + + #[test] + pub fn ok_semantic_model_is_constant_with_pre_post_updates() { + assert_is_const("const a = b++;", false); + assert_is_const("const a = b--;", false); + assert_is_const("const a = ++b;", false); + assert_is_const("const a = --b;", false); + } + + #[test] + pub fn ok_semantic_model_is_constant_with_assignments() { + assert_is_const("const a = (b = 1);", true); + assert_is_const("const a = (b += 1);", false); + assert_is_const("const a = (b -= 1);", false); + assert_is_const("const a = (b *= 1);", false); + assert_is_const("const a = (b /= 1);", false); + assert_is_const("const a = (b %= 1);", false); + assert_is_const("const a = (b **= 1);", false); + assert_is_const("const a = (b &= 1);", false); + assert_is_const("const a = (b |= 1);", false); + assert_is_const("const a = (b ^= 1);", false); + assert_is_const("const a = (b <<= 1);", false); + assert_is_const("const a = (b >>= 1);", false); + assert_is_const("const a = (b >>>= 1);", false); + assert_is_const("const a = (b &&= 1);", false); + assert_is_const("const a = (b ||= 1);", false); + assert_is_const("const a = (b ??= 1);", false); + } }