diff --git a/.changeset/fix-binding-resolution-and-mutations.md b/.changeset/fix-binding-resolution-and-mutations.md new file mode 100644 index 000000000..392217460 --- /dev/null +++ b/.changeset/fix-binding-resolution-and-mutations.md @@ -0,0 +1,11 @@ +--- +"oxlint-plugin-react-doctor": patch +--- + +Fix cache/navigation binding resolution and unsafe mutation parallelization (issue #1810) + +- **server-cache-with-object-literal**: Properly resolve React.cache imports through aliases and check all argument positions for fresh objects/arrays. Shadowed or non-React cache functions no longer trigger false positives. + +- **nextjs-no-redirect-in-try-catch**: Recognize Next.js `unstable_rethrow(error)` as a valid error forwarding pattern, suppressing the diagnostic when the caught error is correctly rethrown. + +- **server-sequential-independent-await** and **async-parallel**: Detect mutating HTTP requests (POST, PUT, PATCH, DELETE) and preserve their ordering, preventing incorrect parallelization suggestions for operations that must run sequentially. diff --git a/packages/fuzz/corpus/regressions/async-parallel--ordered-http-mutations.tsx b/packages/fuzz/corpus/regressions/async-parallel--ordered-http-mutations.tsx new file mode 100644 index 000000000..846da269d --- /dev/null +++ b/packages/fuzz/corpus/regressions/async-parallel--ordered-http-mutations.tsx @@ -0,0 +1,10 @@ +// rule: async-parallel +// verdict: pass +// Source: issue #1810 and PR #1811 review. +// Weakness: HTTP writes must preserve their sequential order. +export async function update() { + const first = await fetch("/create", { method: "POST" }); + const second = await fetch("/update", { method: "PATCH" }); + const third = await fetch("/delete", { method: "DELETE" }); + return [first, second, third]; +} diff --git a/packages/fuzz/corpus/regressions/nextjs-no-redirect-in-try-catch--framework-rethrow.tsx b/packages/fuzz/corpus/regressions/nextjs-no-redirect-in-try-catch--framework-rethrow.tsx new file mode 100644 index 000000000..8aa88a3f2 --- /dev/null +++ b/packages/fuzz/corpus/regressions/nextjs-no-redirect-in-try-catch--framework-rethrow.tsx @@ -0,0 +1,12 @@ +// rule: nextjs-no-redirect-in-try-catch +// verdict: pass +// Source: issue #1810 and PR #1811 review. +// Weakness: Framework rethrows must forward the actual caught binding. +import { redirect, unstable_rethrow } from "next/navigation"; +export function Page() { + try { + redirect("/done"); + } catch (error) { + unstable_rethrow(error); + } +} diff --git a/packages/fuzz/corpus/regressions/server-cache-with-object-literal--mutable-alias.tsx b/packages/fuzz/corpus/regressions/server-cache-with-object-literal--mutable-alias.tsx new file mode 100644 index 000000000..6862e24f4 --- /dev/null +++ b/packages/fuzz/corpus/regressions/server-cache-with-object-literal--mutable-alias.tsx @@ -0,0 +1,9 @@ +// rule: server-cache-with-object-literal +// verdict: pass +// Source: issue #1810 and PR #1811 review. +// Weakness: A reassigned alias no longer identifies a cached function. +import { cache } from "react"; +const read = cache(load); +let alias = read; +alias = other; +export const result = alias({ id: 1 }); diff --git a/packages/fuzz/corpus/regressions/server-sequential-independent-await--ordered-http-mutations.tsx b/packages/fuzz/corpus/regressions/server-sequential-independent-await--ordered-http-mutations.tsx new file mode 100644 index 000000000..34fc9a4de --- /dev/null +++ b/packages/fuzz/corpus/regressions/server-sequential-independent-await--ordered-http-mutations.tsx @@ -0,0 +1,9 @@ +// rule: server-sequential-independent-await +// verdict: pass +// Source: issue #1810 and PR #1811 review. +// Weakness: HTTP writes must preserve their sequential order. +export async function update() { + const first = await fetch("/create", { method: "POST" }); + const second = await fetch("/update", { method: "PATCH" }); + return [first, second]; +} diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.regressions.test.ts index d897d7f1c..cf0614097 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.regressions.test.ts @@ -399,4 +399,70 @@ async function loadDashboard(api) { `, ); }); + + it("stays silent when the first await is a POST mutation", () => { + expectPass( + `export default async function handler() { + const created = await fetch("/api/users", { method: "POST", body: data }); + const user = await fetch("/api/user"); + const posts = await fetch("/api/posts"); + return { created, user, posts }; +}`, + ); + }); + + it("stays silent when any await is a PUT mutation", () => { + expectPass( + `export default async function handler() { + const user = await fetch("/api/user"); + const updated = await fetch("/api/users/1", { method: "PUT", body: data }); + const posts = await fetch("/api/posts"); + return { user, updated, posts }; +}`, + ); + }); + + it("stays silent when any await is a PATCH mutation", () => { + expectPass( + `export default async function handler() { + const user = await fetch("/api/user"); + const posts = await fetch("/api/posts"); + const patched = await fetch("/api/users/1", { method: "PATCH", body: data }); + return { user, posts, patched }; +}`, + ); + }); + + it("stays silent when any await is a DELETE mutation", () => { + expectPass( + `export default async function handler() { + const user = await fetch("/api/user"); + const posts = await fetch("/api/posts"); + const deleted = await fetch("/api/users/1", { method: "DELETE" }); + return { user, posts, deleted }; +}`, + ); + }); + + it("stays silent for lowercase mutating method names", () => { + expectPass( + `export default async function handler() { + const created = await fetch("/api/users", { method: "post", body: data }); + const user = await fetch("/api/user"); + const posts = await fetch("/api/posts"); + return { created, user, posts }; +}`, + ); + }); + + it("still flags when all fetches are GET", () => { + expectFail( + `export default async function handler() { + const user = await fetch("/api/user", { method: "GET" }); + const posts = await fetch("/api/posts", { method: "GET" }); + const comments = await fetch("/api/comments", { method: "GET" }); + return { user, posts, comments }; +}`, + ); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.ts index 50da4f4c4..feb36aa04 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/js-performance/async-parallel.ts @@ -7,6 +7,7 @@ import { import { SEQUENTIAL_AWAIT_THRESHOLD } from "../../constants/thresholds.js"; import { defineRule } from "../../utils/define-rule.js"; import { expressionReadsPatternBinding } from "../../utils/expression-reads-pattern-binding.js"; +import { findSideEffect } from "../../utils/find-side-effect.js"; import { isFunctionLike } from "../../utils/is-function-like.js"; import { normalizeFilename } from "../../utils/normalize-filename.js"; import { getCalleeIdentifierTrail } from "../../utils/get-callee-identifier-trail.js"; @@ -92,11 +93,11 @@ const isNonCallAwait = (statement: EsTreeNode): boolean => { // Skip a consecutive-await block whenever any one of its awaits is an // ordered-UI-flow call, an intentional sequencing call, a bare -// side-effect await, or an await of an already-started promise. A single -// `await page.click(...)` in the middle of three otherwise-independent -// awaits is enough to mark the whole sequence as deliberately -// serialized — collapsing it into `Promise.all([...])` would change -// observable behavior. +// side-effect await, an await of an already-started promise, or a mutating +// HTTP request. A single `await page.click(...)` in the middle of three +// otherwise-independent awaits is enough to mark the whole sequence as +// deliberately serialized — collapsing it into `Promise.all([...])` would +// change observable behavior. const sequenceContainsSerializationSignal = ( statements: EsTreeNode[], context: RuleContext, @@ -106,6 +107,8 @@ const sequenceContainsSerializationSignal = ( if (isNonCallAwait(statement)) return true; const awaitedCall = getAwaitedCall(statement); if (awaitedCall && hasPossibleStaticMemberCallWrite(awaitedCall, context.scopes)) return true; + if (awaitedCall && findSideEffect(awaitedCall, { shouldTraverseNestedFunction: () => false })) + return true; const orderIndependentFunction = awaitedCall ? getOrderIndependentLocalFunction(awaitedCall, context.scopes) : null; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.regressions.test.ts index 5463e97ad..1b9f4ab4a 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.regressions.test.ts @@ -246,4 +246,153 @@ export default async function Page() { expect(result.parseErrors).toEqual([]); expect(result.diagnostics).toEqual([]); }); + + it("stays silent when unstable_rethrow forwards the caught error", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import { redirect, unstable_rethrow } from "next/navigation"; +export default async function Page() { + try { + await save(); + redirect("/done"); + } catch (error) { + unstable_rethrow(error); + console.error(error); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent when unstable_rethrow is used as a namespace import", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import * as nav from "next/navigation"; +export default async function Page() { + try { + await save(); + nav.redirect("/done"); + } catch (error) { + nav.unstable_rethrow(error); + console.error(error); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags when unstable_rethrow is called with a different binding", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import { redirect, unstable_rethrow } from "next/navigation"; +const savedError = new Error("saved"); +export default async function Page() { + try { + redirect("/done"); + } catch (error) { + unstable_rethrow(savedError); + console.error(error); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("stays silent when a renamed unstable_rethrow forwards the caught error", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import { redirect, unstable_rethrow as rethrow } from "next/navigation"; +export default async function Page() { + try { + await save(); + redirect("/done"); + } catch (error) { + rethrow(error); + console.error(error); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags when unstable_rethrow is deferred in a callback", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import { redirect, unstable_rethrow } from "next/navigation"; +export default async function Page() { + try { + redirect("/done"); + } catch (error) { + setTimeout(() => unstable_rethrow(error), 0); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("reports when a local unstable_rethrow shadows the import", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + `import { redirect } from "next/navigation"; +const unstable_rethrow = (e) => { /* noop */ }; +export default async function Page() { + try { + redirect("/done"); + } catch (error) { + unstable_rethrow(error); + } +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + it.each([ + `try { unstable_rethrow(error); } catch (inner) { console.error(inner); }`, + `{ const error = new Error("other"); unstable_rethrow(error); }`, + ])("reports when the framework rethrow does not forward the caught error: %s", (catchBody) => { + const result = runRule( + nextjsNoRedirectInTryCatch, + ` + import { redirect, unstable_rethrow } from "next/navigation"; + export function Page() { + try { redirect("/done"); } catch (error) { ${catchBody} } + } + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + it("reports namespace navigation when the catch swallows the error", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + ` + import * as navigation from "next/navigation"; + export function Page() { + try { navigation.redirect("/done"); } catch (error) { console.error(error); } + } + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + it("accepts a framework rethrow forwarded through a nested catch", () => { + const result = runRule( + nextjsNoRedirectInTryCatch, + ` + import { redirect, unstable_rethrow } from "next/navigation"; + export function Page() { + try { redirect("/done"); } catch (error) { + try { unstable_rethrow(error); } catch (inner) { unstable_rethrow(inner); } + } + } + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts index 34e234221..f964988ea 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/nextjs/nextjs-no-redirect-in-try-catch.ts @@ -1,10 +1,10 @@ import { NEXTJS_NAVIGATION_FUNCTIONS } from "../../constants/nextjs.js"; import { defineRule } from "../../utils/define-rule.js"; import { findGuardingTryStatement } from "../../utils/find-guarding-try-statement.js"; -import { getImportedNameFromModule } from "../../utils/find-import-source-for-name.js"; import type { RuleContext } from "../../utils/rule-context.js"; import { isNodeOfType } from "../../utils/is-node-of-type.js"; import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; +import { resolveImportedApiReference } from "../../utils/resolve-imported-api-reference.js"; export const nextjsNoRedirectInTryCatch = defineRule({ id: "nextjs-no-redirect-in-try-catch", @@ -16,21 +16,36 @@ export const nextjsNoRedirectInTryCatch = defineRule({ "Move `redirect()` or `notFound()` outside the try block, or rethrow in `catch`, because these APIs throw control-flow errors that catch blocks swallow.", create: (context: RuleContext) => ({ CallExpression(node: EsTreeNodeOfType<"CallExpression">) { - if (!isNodeOfType(node.callee, "Identifier")) return; - // Resolve to the actual next/navigation export so a local function of the - // same name (`const redirect = ...`) is never flagged. - const importedName = getImportedNameFromModule(node, node.callee.name, "next/navigation"); - if (!importedName || !NEXTJS_NAVIGATION_FUNCTIONS.has(importedName)) return; + const navigationReference = resolveImportedApiReference(node.callee, context.scopes); + if ( + navigationReference?.source !== "next/navigation" || + !navigationReference.importedName || + !NEXTJS_NAVIGATION_FUNCTIONS.has(navigationReference.importedName) + ) + return; - // findGuardingTryStatement resolves the try/catch that actually - // swallows the thrown control-flow error, climbing past re-throwing - // catches, bare try/finally, and IIFE boundaries. - const guardingTry = findGuardingTryStatement(node); + const frameworkRethrowPredicate = ( + expression: EsTreeNodeOfType<"CallExpression">, + caughtBinding: EsTreeNodeOfType<"Identifier">, + ): boolean => { + const rethrowReference = resolveImportedApiReference(expression.callee, context.scopes); + if ( + rethrowReference?.source !== "next/navigation" || + rethrowReference.importedName !== "unstable_rethrow" + ) + return false; + const argument = expression.arguments[0]; + if (!isNodeOfType(argument, "Identifier")) return false; + const caughtSymbol = context.scopes.symbolFor(caughtBinding); + return Boolean(caughtSymbol && context.scopes.symbolFor(argument) === caughtSymbol); + }; + + const guardingTry = findGuardingTryStatement(node, frameworkRethrowPredicate); if (!guardingTry) return; context.report({ node, - message: `${node.callee.name}() inside try-catch gets swallowed, so the redirect silently fails.`, + message: `${navigationReference.importedName}() inside try-catch gets swallowed, so the redirect silently fails.`, }); }, }), diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.regressions.test.ts index dbaeb9d3f..9e9f0fdd6 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.regressions.test.ts @@ -81,4 +81,135 @@ export const loadUser = async () => getUser(Object.freeze({ id: 1 }));`, expect(result.parseErrors).toEqual([]); expect(result.diagnostics).toEqual([]); }); + + it("flags a fresh object in any argument position, not just first", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const getUser = cache(async (id, params) => db.user.find(id, params)); +export const loadUser = async () => getUser(1, { sort: "name" });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("flags a fresh array argument", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const getUsers = cache(async (ids) => db.user.findMany(ids)); +export const loadUsers = async () => getUsers([1, 2, 3]);`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("flags calling a renamed cache import with an object literal", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache as memoize } from "react"; +const getUser = memoize(async (params) => db.user.find(params)); +export const loadUser = async () => getUser({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("flags calling an immutable alias with an object literal", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const memoize = cache; +const getUser = memoize(async (params) => db.user.find(params)); +export const loadUser = async () => getUser({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("stays silent when cache import is shadowed by a local function", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache as reactCache } from "react"; +const cache = (fn) => fn; +const getUser = cache(async (params) => db.user.find(params)); +export const loadUser = async () => getUser({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent when the cached function name is shadowed by a parameter", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const read = cache(async (params) => db.user.find(params)); +export const loadUser = async (read) => read({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags the real cached function when a parameter shadows its name elsewhere", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const read = cache(async (params) => db.user.find(params)); +const unrelated = (read) => read({ id: 1 }); +export const loadUser = async () => read({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("stays silent when cache is imported from a non-React module", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "memory-cache"; +const getUser = cache(async (params) => db.user.find(params)); +export const loadUser = async () => getUser({ id: 1 });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("reports once per call, not once per fresh argument", () => { + const result = runRule( + serverCacheWithObjectLiteral, + `import { cache } from "react"; +const getUser = cache(async (params, options) => db.user.find(params, options)); +export const loadUser = async () => getUser({ id: 1 }, { include: ["posts"] });`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); + it("stays silent when a cached function alias is reassigned", () => { + const result = runRule( + serverCacheWithObjectLiteral, + ` + import { cache } from "react"; + const read = cache(load); + let alias = read; + alias = other; + export const result = alias({ id: 1 }); + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("reports through a chain of immutable cached function aliases", () => { + const result = runRule( + serverCacheWithObjectLiteral, + ` + import { cache } from "react"; + const read = cache(load); + const alias = read; + const secondAlias = alias; + export const result = secondAlias({ id: 1 }); + `, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(1); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.ts index d2dc4268f..bd753bd11 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-cache-with-object-literal.ts @@ -6,6 +6,8 @@ import { OBJECT_FREEZE_OR_SEAL_METHOD_NAMES, unwrapObjectIntegrityExpression, } from "../../utils/unwrap-object-integrity-expression.js"; +import { resolveImportedApiReference } from "../../utils/resolve-imported-api-reference.js"; +import { resolveConstIdentifierAlias } from "../../utils/resolve-const-identifier-alias.js"; // HACK: `cache(fn)` from React keys deduplication on REFERENCE equality // of the function arguments. Calling the cached function with object @@ -20,43 +22,31 @@ export const serverCacheWithObjectLiteral = defineRule({ severity: "warn", recommendation: "Pass plain values like strings or numbers, not an object. React.cache() matches the exact value, so a new `{}` each render misses the cache.", - create: (context: RuleContext) => { - const cachedFunctionNames = new Set(); - - return { - VariableDeclarator(node: EsTreeNodeOfType<"VariableDeclarator">) { - if (!isNodeOfType(node.id, "Identifier")) return; - const init = node.init; - if (!isNodeOfType(init, "CallExpression")) return; - const callee = init.callee; - const isCacheCall = - (isNodeOfType(callee, "Identifier") && callee.name === "cache") || - (isNodeOfType(callee, "MemberExpression") && - isNodeOfType(callee.object, "Identifier") && - callee.object.name === "React" && - isNodeOfType(callee.property, "Identifier") && - callee.property.name === "cache"); - if (!isCacheCall) return; - cachedFunctionNames.add(node.id.name); - }, - CallExpression(node: EsTreeNodeOfType<"CallExpression">) { - if (cachedFunctionNames.size === 0) return; - if (!isNodeOfType(node.callee, "Identifier")) return; - if (!cachedFunctionNames.has(node.callee.name)) return; - const firstArg = node.arguments?.[0]; - if (!firstArg || isNodeOfType(firstArg, "SpreadElement")) return; + create: (context: RuleContext) => ({ + CallExpression(node: EsTreeNodeOfType<"CallExpression">) { + const cachedFunction = resolveConstIdentifierAlias(node.callee, context.scopes); + if (cachedFunction?.kind !== "const") return; + const initializer = cachedFunction.initializer; + if (!isNodeOfType(initializer, "CallExpression") || initializer.arguments.length !== 1) + return; + const cacheReference = resolveImportedApiReference(initializer.callee, context.scopes); + if (cacheReference?.source !== "react" || cacheReference.importedName !== "cache") return; + const hasFreshArgument = node.arguments.some((argument) => { + if (isNodeOfType(argument, "SpreadElement")) return false; const cacheKey = unwrapObjectIntegrityExpression( - firstArg, + argument, context.scopes, OBJECT_FREEZE_OR_SEAL_METHOD_NAMES, ); - if (!isNodeOfType(cacheKey, "ObjectExpression")) return; - - context.report({ - node, - message: `Passing a new object to React.cache() each render misses the cache, so it refetches every request.`, - }); - }, - }; - }, + return ( + isNodeOfType(cacheKey, "ObjectExpression") || isNodeOfType(cacheKey, "ArrayExpression") + ); + }); + if (!hasFreshArgument) return; + context.report({ + node, + message: `Passing a new object to React.cache() each render misses the cache, so it refetches every request.`, + }); + }, + }), }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.regressions.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.regressions.test.ts index 134c396ee..6dd7c15e5 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.regressions.test.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.regressions.test.ts @@ -323,4 +323,95 @@ export default async function Page() { expect(result.parseErrors).toEqual([]); expect(result.diagnostics.length).toBeGreaterThan(0); }); + + it("stays silent when the first await is a POST mutation", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const created = await fetch("/api/users", { method: "POST", body: data }); + const users = await fetch("/api/users"); + return { created, users }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent when the first await is a PUT mutation", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const updated = await fetch("/api/users/1", { method: "PUT", body: data }); + const users = await fetch("/api/users"); + return { updated, users }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent when the first await is a PATCH mutation", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const patched = await fetch("/api/users/1", { method: "PATCH", body: data }); + const users = await fetch("/api/users"); + return { patched, users }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent when the first await is a DELETE mutation", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const deleted = await fetch("/api/users/1", { method: "DELETE" }); + const users = await fetch("/api/users"); + return { deleted, users }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("stays silent for lowercase mutating method names", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const created = await fetch("/api/users", { method: "post", body: data }); + const users = await fetch("/api/users"); + return { created, users }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); + + it("still flags two GET fetches", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const user = await fetch("/api/user", { method: "GET" }); + const posts = await fetch("/api/posts", { method: "GET" }); + return { user, posts }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it("stays silent when the second await mutates data read by the first", () => { + const result = runRule( + serverSequentialIndependentAwait, + `export default async function handler() { + const users = await fetch("/api/users"); + const created = await fetch("/api/users", { method: "POST", body: data }); + return { users, created }; +}`, + ); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toEqual([]); + }); }); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts index 4376f55ef..e32b8e2d6 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/server/server-sequential-independent-await.ts @@ -1,6 +1,7 @@ import { INTENTIONAL_SEQUENCING_CALLEE_NAMES } from "../../constants/js.js"; import { defineRule } from "../../utils/define-rule.js"; import { expressionReadsPatternBinding } from "../../utils/expression-reads-pattern-binding.js"; +import { findSideEffect } from "../../utils/find-side-effect.js"; import { getCalleeName } from "../../utils/get-callee-name.js"; import { getOrderIndependentLocalFunction } from "../../utils/get-order-independent-local-function.js"; import { hasPossibleStaticMemberCallWrite } from "../../utils/has-static-property-write-before.js"; @@ -132,9 +133,6 @@ const declarationAwaitsGate = (declaration: EsTreeNode, context: RuleContext): b if (!isNodeOfType(declaration, "VariableDeclaration")) return false; for (const declarator of declaration.declarations ?? []) { if (!isNodeOfType(declarator.init, "AwaitExpression")) continue; - // Only a function call is a gate — an awaited constructor (`await new X()`) - // must not suppress, so keep this CallExpression-only (getCalleeName also - // resolves NewExpression, which would over-suppress here). const argument = declarator.init.argument; if (!isNodeOfType(argument, "CallExpression")) continue; if (hasPossibleStaticMemberCallWrite(argument, context.scopes)) return true; @@ -173,6 +171,7 @@ const declarationAwaitsIntentionalSequence = ( if (!isNodeOfType(declarator.init, "AwaitExpression")) continue; const argument = declarator.init.argument; if (!isNodeOfType(argument, "CallExpression")) continue; + if (findSideEffect(argument, { shouldTraverseNestedFunction: () => false })) return true; const localFunction = getOrderIndependentLocalFunction(argument, context.scopes); const calleeName = getCalleeName(argument); if ( diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/catch-clause-rethrows-caught.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/catch-clause-rethrows-caught.ts index ed0429ab6..0ea700bff 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/catch-clause-rethrows-caught.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/catch-clause-rethrows-caught.ts @@ -4,9 +4,17 @@ import { isFunctionLike } from "./is-function-like.js"; import { isNodeOfType } from "./is-node-of-type.js"; import { walkAst } from "./walk-ast.js"; +export interface RethrowPredicate { + ( + call: EsTreeNodeOfType<"CallExpression">, + caughtBinding: EsTreeNodeOfType<"Identifier">, + ): boolean; +} + const doesThrowEscapeCatchClause = ( - throwStatement: EsTreeNodeOfType<"ThrowStatement">, + throwStatement: EsTreeNode, handler: EsTreeNodeOfType<"CatchClause">, + frameworkRethrowPredicate?: RethrowPredicate, ): boolean => { let child: EsTreeNode = throwStatement; let ancestor: EsTreeNode | null | undefined = throwStatement.parent; @@ -15,7 +23,7 @@ const doesThrowEscapeCatchClause = ( isNodeOfType(ancestor, "TryStatement") && ancestor.block === child && ancestor.handler && - !catchClauseRethrowsCaught(ancestor.handler) + !catchClauseRethrowsCaught(ancestor.handler, frameworkRethrowPredicate) ) { return false; } @@ -37,18 +45,31 @@ const doesThrowEscapeCatchClause = ( // callback doesn't count, and a rethrow nested inside a try (within the // catch body) whose own catch swallows it doesn't count either — that // error never escapes the catch clause. -export const catchClauseRethrowsCaught = (handler: EsTreeNodeOfType<"CatchClause">): boolean => { +// +// An optional frameworkRethrowPredicate can recognize framework-specific +// rethrow patterns (e.g. Next.js's `unstable_rethrow(error)`), which are +// treated as equivalent to `throw error`. +export const catchClauseRethrowsCaught = ( + handler: EsTreeNodeOfType<"CatchClause">, + frameworkRethrowPredicate?: RethrowPredicate, +): boolean => { const caughtBindingName = isNodeOfType(handler.param, "Identifier") ? handler.param.name : null; if (!caughtBindingName) return false; let didRethrow = false; walkAst(handler.body, (child: EsTreeNode) => { if (didRethrow) return false; if (child !== handler.body && isFunctionLike(child)) return false; - if ( + const isCaughtThrow = isNodeOfType(child, "ThrowStatement") && isNodeOfType(child.argument, "Identifier") && - child.argument.name === caughtBindingName && - doesThrowEscapeCatchClause(child, handler) + child.argument.name === caughtBindingName; + const isFrameworkRethrow = + isNodeOfType(child, "CallExpression") && + isNodeOfType(handler.param, "Identifier") && + frameworkRethrowPredicate?.(child, handler.param); + if ( + (isCaughtThrow || isFrameworkRethrow) && + doesThrowEscapeCatchClause(child, handler, frameworkRethrowPredicate) ) { didRethrow = true; return false; diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/utils/find-guarding-try-statement.ts b/packages/oxlint-plugin-react-doctor/src/plugin/utils/find-guarding-try-statement.ts index 3fbc9553d..7dce9ce41 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/utils/find-guarding-try-statement.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/utils/find-guarding-try-statement.ts @@ -1,3 +1,4 @@ +import type { RethrowPredicate } from "./catch-clause-rethrows-caught.js"; import { catchClauseRethrowsCaught } from "./catch-clause-rethrows-caught.js"; import type { EsTreeNode } from "./es-tree-node.js"; import type { EsTreeNodeOfType } from "./es-tree-node-of-type.js"; @@ -17,8 +18,13 @@ import { isNodeOfType } from "./is-node-of-type.js"; // stops at the first function boundary — unless the function is the callee // of an immediately-invoked call (IIFE), which executes synchronously // inside the try. +// +// An optional frameworkRethrowPredicate recognizes framework-specific rethrow +// patterns (e.g. Next.js's `unstable_rethrow(error)`), which silence the +// diagnostic even when they don't use literal `throw`. export const findGuardingTryStatement = ( node: EsTreeNode, + frameworkRethrowPredicate?: RethrowPredicate, ): EsTreeNodeOfType<"TryStatement"> | null => { let child: EsTreeNode = node; let ancestor: EsTreeNode | null | undefined = node.parent; @@ -30,7 +36,7 @@ export const findGuardingTryStatement = ( isNodeOfType(ancestor, "TryStatement") && ancestor.block === child && ancestor.handler && - !catchClauseRethrowsCaught(ancestor.handler) + !catchClauseRethrowsCaught(ancestor.handler, frameworkRethrowPredicate) ) { return ancestor; }