diff --git a/.changeset/solid-2-effect-and-ref-rules.md b/.changeset/solid-2-effect-and-ref-rules.md new file mode 100644 index 0000000..f965737 --- /dev/null +++ b/.changeset/solid-2-effect-and-ref-rules.md @@ -0,0 +1,34 @@ +--- +"@murky-web/oxlint-plugin-solid": minor +--- + +Add two rules for Solid 2.0's split effects and unowned ref callbacks, and +repair two existing ones. + +`solid/no-untracked-effect-read` reports a reactive read in the apply phase of a +two-argument `createEffect`. That phase runs untracked, so +`createEffect(() => roomId(), (id) => connect(id, theme()))` never re-runs when +`theme` changes. Nothing reported this: `reactivity` marks the apply phase as a +tracked scope, and its model has no state for "matches, but does not subscribe". + +`solid/no-owned-primitives-in-ref` reports `createEffect`, `onCleanup` and +related primitives inside a ref callback. Solid 2.0 ref callbacks are unowned — +`getOwner()` returns null — so the effect is never disposed and the cleanup +never runs. Directive factories, which create these primitives while they still +have an owner, are unaffected. + +`solid/no-setter-in-effect` now also reports an async apply phase that awaits and +writes the result back into a signal. Its existing check requires every +statement to be a setter call, which an `await` breaks. + +`solid/prefer-class-object` matched only the lowercase `classlist`. JSX prop +names are case-sensitive and the Solid 1 prop is `classList`, so the migration +rule missed every real call site. + +`solid/imports` now knows `createLoadingBoundary`, `createErrorBoundary` and +`createRevealOrder`, so importing one from the wrong module is reported like +every neighbouring primitive. + +`solid/jsx-no-duplicate-props` recommended `classList`, which Solid 2.0 removed +and the neighbouring rule reports. It now points at the object and array forms +of `class`. diff --git a/.changeset/typebuddy-oxlint-autofix-repairs.md b/.changeset/typebuddy-oxlint-autofix-repairs.md new file mode 100644 index 0000000..e501b20 --- /dev/null +++ b/.changeset/typebuddy-oxlint-autofix-repairs.md @@ -0,0 +1,43 @@ +--- +"@murky-web/typebuddy": minor +--- + +Repair the oxlint autofixes and close the rules' blind spots. + +Two fixes were producing broken code. `prefer-maybe-promise` inserted +`return err()` directly before a catch block's closing brace, so +`catch { log("boom") }` became `catch { log("boom") return err(); }`, which does +not parse. `require-try-catch` rebuilt the function body line by line and +stripped leading whitespace from every line — including lines inside template +literals, silently changing string values. It now wraps the body with two +zero-width insertions and leaves it byte for byte intact. + +Six blind spots closed: + +- The catch branch settles on `err()`. It was fed to the same code path as the + try block, so a catch return got two reports with opposite fixes (`ok(value)` + and `err()`), and `--fix` appended an unreachable `return err()` after an + existing return. Returns that hand back a value are now reported without a + fix — rewriting them to `err()` would drop the author's fallback, and where + the default belongs is the author's decision. +- Returns nested inside an `if`, loop or `switch` in a try block are wrapped. + Only the try block's direct children were, so `try { if (flag) return "early"; + return "late"; }` wrapped just `"late"`. +- `Promise` in a type-level signature is reported. `TSDeclareFunction`, + `TSFunctionType` and `TSMethodSignature` were routed through a check for + `async`, which a type signature can never carry, so all three visitors were + unreachable. Ambient `declare` context stays exempt: rewriting a third-party + callback shape would misdescribe that API. +- `MaybePromise` and `AsyncResult` count as awaited return types. Matching only + `Promise` meant a bare `return;` in a `MaybePromise` function was left + alone while its `Promise` twin was fixed — the rule went blind on + already-migrated code, and on the spelling the package recommends. +- Expression-bodied async arrows are reported and fixed. `async () => fetch(url)` + has no block for the try/catch to live in and escaped entirely. +- `null | undefined` is left alone instead of being rewritten to the + equivalent-but-longer `Optional`. + +Also fixed: an inserted helper import landed glued to the following statement in +any file that starts with a comment. Whether to append after an existing import +was inferred from the anchor's byte offset, which only answers that question in +a file whose first statement begins at byte zero. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc8b875..d0313d7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,7 @@ on: pull_request: push: branches: - - master + - main workflow_dispatch: jobs: @@ -43,3 +43,24 @@ jobs: - name: Smoke treeshaking run: bun --cwd packages/typebuddy run smoke:treeshake + + - name: Smoke subpaths + run: bun --cwd packages/typebuddy run smoke:subpath + + oxlint-plugin-solid: + name: Oxlint plugin solid + runs-on: ubuntu-latest + steps: + - name: Checkout repo + uses: actions/checkout@v4 + + - name: Setup Bun + uses: oven-sh/setup-bun@v2 + with: + bun-version-file: "package.json" + + - name: Install dependencies + run: bun install --frozen-lockfile + + - name: Test oxlint-plugin-solid + run: bun --cwd packages/oxlint-plugin-solid run test diff --git a/package.json b/package.json index b0be472..94b12e8 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "release:verify": "bun run release:verify:typebuddy && bun run release:verify:simplelog && bun run release:verify:oxlint-plugin-solid", "release:verify:oxlint-plugin-solid": "bun run --cwd ./packages/oxlint-plugin-solid test", "release:verify:simplelog": "bun run --cwd ./packages/simplelog lint && bun run --cwd ./packages/simplelog typecheck && bun run --cwd ./packages/simplelog test && bun run --cwd ./packages/simplelog smoke:oxlint && bun run --cwd ./packages/simplelog smoke:entries", - "release:verify:typebuddy": "bun run --cwd ./packages/typebuddy lint && bun run --cwd ./packages/typebuddy typecheck && bun run --cwd ./packages/typebuddy test && bun run --cwd ./packages/typebuddy smoke:globals && bun run --cwd ./packages/typebuddy smoke:oxlint && bun run --cwd ./packages/typebuddy smoke:oxlint:fix && bun run --cwd ./packages/typebuddy smoke:treeshake", + "release:verify:typebuddy": "bun run --cwd ./packages/typebuddy lint && bun run --cwd ./packages/typebuddy typecheck && bun run --cwd ./packages/typebuddy test && bun run --cwd ./packages/typebuddy smoke:globals && bun run --cwd ./packages/typebuddy smoke:oxlint && bun run --cwd ./packages/typebuddy smoke:oxlint:fix && bun run --cwd ./packages/typebuddy smoke:subpath && bun run --cwd ./packages/typebuddy smoke:treeshake", "test": "bun run --workspaces --if-present test", "typecheck": "bun run --workspaces --if-present typecheck", "lint": "oxlint -c ./packages/config/oxc/.oxlintrc.jsonc --type-aware ./packages", diff --git a/packages/config/oxc/linting/solid.jsonc b/packages/config/oxc/linting/solid.jsonc index f2e70f2..01dea36 100644 --- a/packages/config/oxc/linting/solid.jsonc +++ b/packages/config/oxc/linting/solid.jsonc @@ -12,10 +12,13 @@ "solid/no-array-handlers": "error", "solid/no-destructure": "error", "solid/no-innerhtml": "error", + "solid/no-owned-primitives-in-ref": "error", "solid/no-proxy-apis": "error", "solid/no-react-deps": "error", "solid/no-react-specific-props": "error", + "solid/no-setter-in-effect": "error", "solid/no-unknown-namespaces": "error", + "solid/no-untracked-effect-read": "error", "solid/prefer-arrow-components": "error", "solid/prefer-class-object": "error", "solid/prefer-for": "error", diff --git a/packages/oxlint-plugin-solid/README.md b/packages/oxlint-plugin-solid/README.md index 6984dbb..7ff35cd 100644 --- a/packages/oxlint-plugin-solid/README.md +++ b/packages/oxlint-plugin-solid/README.md @@ -8,10 +8,25 @@ projektspezifische Regeln wie `solid/prefer-arrow-components`. Die Regelmodule unter `src/rules/` sind aus dem Upstream-Quellstand abgeleitet und laufen ohne `eslint-plugin-solid` als Zielprojekt-Dependency. +Das Paket zieht auch ESLint selbst nicht mehr nach. Die Regeln sind schlichte +Objekte, wie Oxlint sie erwartet -- der `createRule`-Wrapper aus +`@typescript-eslint/utils` war nur eine TypeScript-Typhilfe und ist entfallen. +Geblieben sind fuenf AST-Helfer, die jetzt direkt aus +`@eslint-community/eslint-utils` kommen, statt die komplette +TypeScript-ESLint-Toolchain in den Baum jedes Konsumenten zu ziehen. + Aktuell sind enthalten: - die komplette von `eslint-plugin-solid` exportierte Regelmenge -- die zusaetzliche Projektregel `solid/prefer-arrow-components` +- die zusaetzlichen Projektregeln `solid/prefer-arrow-components`, + `solid/no-setter-in-effect` (meldet Effects, die nur in ein Signal oder einen + Store schreiben, statt den Wert abzuleiten -- und Effects, die etwas awaiten + und das Ergebnis zurueckschreiben, statt es aus einer Derivation zu liefern) + `solid/no-untracked-effect-read` (meldet reaktive Reads in der + apply-Phase eines zweiphasigen `createEffect`, die dort nicht tracken) und + `solid/no-owned-primitives-in-ref` (meldet `createEffect`/`onCleanup` und + Verwandte in einem Ref-Callback -- der laeuft ohne Owner, nichts raeumt sie + je wieder ab) - ein Test-Harness, der die exportierte Rule-Surface und echte Diagnostik gegen Temp-Projekte prueft diff --git a/packages/oxlint-plugin-solid/package.json b/packages/oxlint-plugin-solid/package.json index 8f8c397..4b36e03 100644 --- a/packages/oxlint-plugin-solid/package.json +++ b/packages/oxlint-plugin-solid/package.json @@ -26,8 +26,7 @@ }, "sideEffects": false, "dependencies": { - "@typescript-eslint/utils": "^8.57.1", - "eslint": "^9.38.0", + "@eslint-community/eslint-utils": "^4.9.1", "estraverse": "^5.3.0", "is-html": "^2.0.0", "kebab-case": "^1.0.2", diff --git a/packages/oxlint-plugin-solid/src/compat.mjs b/packages/oxlint-plugin-solid/src/compat.mjs index 5420476..2ee86ec 100644 --- a/packages/oxlint-plugin-solid/src/compat.mjs +++ b/packages/oxlint-plugin-solid/src/compat.mjs @@ -1,4 +1,4 @@ -import { ASTUtils } from "@typescript-eslint/utils"; +import { findVariable as utilsFindVariable } from "@eslint-community/eslint-utils"; export function getSourceCode(context) { if (typeof context.getSourceCode === "function") { @@ -23,7 +23,7 @@ export function getScope(context, node) { } export function findVariable(context, node) { - return ASTUtils.findVariable(getScope(context, node), node); + return utilsFindVariable(getScope(context, node), node); } export function markVariableAsUsed(context, name, node) { diff --git a/packages/oxlint-plugin-solid/src/index.mjs b/packages/oxlint-plugin-solid/src/index.mjs index 65d8207..f66bae1 100644 --- a/packages/oxlint-plugin-solid/src/index.mjs +++ b/packages/oxlint-plugin-solid/src/index.mjs @@ -8,10 +8,13 @@ import { jsxUsesVarsRule } from "./rules/jsx_uses_vars.mjs"; import { noArrayHandlersRule } from "./rules/no_array_handlers.mjs"; import noDestructureRule from "./rules/no_destructure.mjs"; import noInnerhtmlRule from "./rules/no_innerhtml.mjs"; +import noOwnedPrimitivesInRefRule from "./rules/no_owned_primitives_in_ref.mjs"; import noProxyApisRule from "./rules/no_proxy_apis.mjs"; import noReactDepsRule from "./rules/no_react_deps.mjs"; import { noReactSpecificPropsRule } from "./rules/no_react_specific_props.mjs"; +import noSetterInEffectRule from "./rules/no_setter_in_effect.mjs"; import noUnknownNamespacesRule from "./rules/no_unknown_namespaces.mjs"; +import noUntrackedEffectReadRule from "./rules/no_untracked_effect_read.mjs"; import { preferArrowComponentsRule } from "./rules/prefer_arrow_components.mjs"; import preferClassObjectRule from "./rules/prefer_class_object.mjs"; import preferForRule from "./rules/prefer_for.mjs"; @@ -37,10 +40,13 @@ const extendedPlugin = { "no-array-handlers": noArrayHandlersRule, "no-destructure": noDestructureRule, "no-innerhtml": noInnerhtmlRule, + "no-owned-primitives-in-ref": noOwnedPrimitivesInRefRule, "no-proxy-apis": noProxyApisRule, "no-react-deps": noReactDepsRule, "no-react-specific-props": noReactSpecificPropsRule, + "no-setter-in-effect": noSetterInEffectRule, "no-unknown-namespaces": noUnknownNamespacesRule, + "no-untracked-effect-read": noUntrackedEffectReadRule, "prefer-arrow-components": preferArrowComponentsRule, "prefer-class-object": preferClassObjectRule, "prefer-for": preferForRule, diff --git a/packages/oxlint-plugin-solid/src/rules/components_return_once.mjs b/packages/oxlint-plugin-solid/src/rules/components_return_once.mjs index 89a60c3..6e6ff0f 100644 --- a/packages/oxlint-plugin-solid/src/rules/components_return_once.mjs +++ b/packages/oxlint-plugin-solid/src/rules/components_return_once.mjs @@ -1,8 +1,5 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { getSourceCode } from "../compat.mjs"; import { getFunctionName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; const isNothing = (node) => { if (!node) { return true; @@ -17,7 +14,7 @@ const isNothing = (node) => { } }; const getLineLength = (loc) => loc.end.line - loc.start.line + 1; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -199,4 +196,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/event_handlers.mjs b/packages/oxlint-plugin-solid/src/rules/event_handlers.mjs index 1c5b1a8..15811fd 100644 --- a/packages/oxlint-plugin-solid/src/rules/event_handlers.mjs +++ b/packages/oxlint-plugin-solid/src/rules/event_handlers.mjs @@ -1,9 +1,7 @@ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getStaticValue } from "@eslint-community/eslint-utils"; import { getScope, getSourceCode } from "../compat.mjs"; import { isDOMElementName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getStaticValue } = ASTUtils; const COMMON_EVENTS = [ "onAnimationEnd", "onAnimationIteration", @@ -82,7 +80,7 @@ const isNonstandardEventName = (lowercaseEventName) => Boolean(NONSTANDARD_EVENTS_MAP[lowercaseEventName]); const getStandardEventHandlerName = (lowercaseEventName) => NONSTANDARD_EVENTS_MAP[lowercaseEventName]; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -295,4 +293,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/imports.mjs b/packages/oxlint-plugin-solid/src/rules/imports.mjs index 24b6ee0..b405f9e 100644 --- a/packages/oxlint-plugin-solid/src/rules/imports.mjs +++ b/packages/oxlint-plugin-solid/src/rules/imports.mjs @@ -1,8 +1,5 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { getSourceCode } from "../compat.mjs"; import { appendImports, insertImports, removeSpecifier } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; // Solid 2.0 moved the renderers into `@solidjs/*` packages and pulled the store // APIs into the core. Symbols that are legitimately exported from more than one // package (the control-flow components, `ComponentProps`) are deliberately left @@ -14,12 +11,19 @@ for (const primitive of [ "children", "createContext", "createEffect", + // The primitive forms of `Loading`, `Errored` and `Reveal`. Only custom + // boundary components and renderer integrations reach for them, which is + // why they were missing here — and why a wrong-module import of one went + // unreported while every neighbouring primitive was checked. + "createErrorBoundary", + "createLoadingBoundary", "createMemo", "createOptimistic", "createOptimisticStore", "createProjection", "createReaction", "createRenderEffect", + "createRevealOrder", "createRoot", "createSignal", "createStore", @@ -98,7 +102,7 @@ for (const type of ["ClassValue", "IntrinsicElement", "JSX", "RequestEvent"]) { } const sourceRegex = /^(?:solid-js|@solidjs\/(?:web|h|html|universal))$/; const isSource = (source) => sourceRegex.test(source); -export default createRule({ +export default { meta: { type: "suggestion", docs: { @@ -189,4 +193,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/jsx_no_duplicate_props.mjs b/packages/oxlint-plugin-solid/src/rules/jsx_no_duplicate_props.mjs index a409e13..39dfe53 100644 --- a/packages/oxlint-plugin-solid/src/rules/jsx_no_duplicate_props.mjs +++ b/packages/oxlint-plugin-solid/src/rules/jsx_no_duplicate_props.mjs @@ -1,8 +1,5 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { jsxGetAllProps } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -26,7 +23,7 @@ export default createRule({ messages: { noDuplicateProps: "Duplicate props are not allowed.", noDuplicateClass: - "Duplicate `class` props are not allowed; while it might seem to work, it can break unexpectedly. Use `classList` instead.", + "Duplicate `class` props are not allowed; while it might seem to work, it can break unexpectedly. Pass one `class` with the object or array form instead.", noDuplicateChildren: "Using {{used}} at the same time is not allowed.", }, @@ -84,4 +81,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/jsx_no_script_url.mjs b/packages/oxlint-plugin-solid/src/rules/jsx_no_script_url.mjs index ee6390f..6707a09 100644 --- a/packages/oxlint-plugin-solid/src/rules/jsx_no_script_url.mjs +++ b/packages/oxlint-plugin-solid/src/rules/jsx_no_script_url.mjs @@ -1,14 +1,12 @@ -import { ASTUtils, ESLintUtils } from "@typescript-eslint/utils"; +import { getStaticValue } from "@eslint-community/eslint-utils"; import { getScope } from "../compat.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getStaticValue } = ASTUtils; const JAVASCRIPT_PROTOCOL_PATTERN = "^[\\\\u0000-\\\\u001F ]*j[\\\\r\\\\n\\\\t]*a[\\\\r\\\\n\\\\t]*v[\\\\r\\\\n\\\\t]*a[\\\\r\\\\n\\\\t]*s[\\\\r\\\\n\\\\t]*c[\\\\r\\\\n\\\\t]*r[\\\\r\\\\n\\\\t]*i[\\\\r\\\\n\\\\t]*p[\\\\r\\\\n\\\\t]*t[\\\\r\\\\n\\\\t]*:"; const JAVASCRIPT_PROTOCOL_REGEX = new RegExp(JAVASCRIPT_PROTOCOL_PATTERN, "i"); -export const jsxNoScriptUrlRule = createRule({ +export const jsxNoScriptUrlRule = { meta: { docs: { description: "Disallow javascript: URLs.", @@ -51,4 +49,4 @@ export const jsxNoScriptUrlRule = createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/jsx_no_undef.mjs b/packages/oxlint-plugin-solid/src/rules/jsx_no_undef.mjs index b631f27..9376869 100644 --- a/packages/oxlint-plugin-solid/src/rules/jsx_no_undef.mjs +++ b/packages/oxlint-plugin-solid/src/rules/jsx_no_undef.mjs @@ -1,5 +1,3 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { getScope, getSourceCode } from "../compat.mjs"; import { isDOMElementName, @@ -7,7 +5,6 @@ import { appendImports, insertImports, } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; // Currently all of the control flow components are from 'solid-js'. // Solid 2.0 dropped `Index` (use ``) and renamed the // async/error boundaries. All of these are exported from `solid-js`. @@ -22,7 +19,7 @@ const AUTO_COMPONENTS = [ "Reveal", ]; const SOURCE_MODULE = "solid-js"; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -225,4 +222,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/jsx_uses_vars.mjs b/packages/oxlint-plugin-solid/src/rules/jsx_uses_vars.mjs index 898cdb7..3409890 100644 --- a/packages/oxlint-plugin-solid/src/rules/jsx_uses_vars.mjs +++ b/packages/oxlint-plugin-solid/src/rules/jsx_uses_vars.mjs @@ -1,10 +1,6 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { markVariableAsUsed } from "../compat.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; - -export const jsxUsesVarsRule = createRule({ +export const jsxUsesVarsRule = { meta: { docs: { description: @@ -52,4 +48,4 @@ export const jsxUsesVarsRule = createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_array_handlers.mjs b/packages/oxlint-plugin-solid/src/rules/no_array_handlers.mjs index ed50ebc..90eab5c 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_array_handlers.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_array_handlers.mjs @@ -1,10 +1,6 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { isDOMElementName, trace } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; - -export const noArrayHandlersRule = createRule({ +export const noArrayHandlersRule = { meta: { docs: { description: "Disallow usage of type-unsafe event handlers.", @@ -50,4 +46,4 @@ export const noArrayHandlersRule = createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_destructure.mjs b/packages/oxlint-plugin-solid/src/rules/no_destructure.mjs index a6fa799..db2f4e8 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_destructure.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_destructure.mjs @@ -1,8 +1,6 @@ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getStringIfConstant } from "@eslint-community/eslint-utils"; import { getSourceCode } from "../compat.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getStringIfConstant } = ASTUtils; const getName = (node) => { switch (node.type) { case "Literal": @@ -32,7 +30,7 @@ const getPropertyInfo = (prop) => { return null; } }; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -212,4 +210,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_innerhtml.mjs b/packages/oxlint-plugin-solid/src/rules/no_innerhtml.mjs index a5b810d..518faae 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_innerhtml.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_innerhtml.mjs @@ -1,10 +1,8 @@ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getStringIfConstant } from "@eslint-community/eslint-utils"; import isHtml from "is-html"; import { jsxPropName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getStringIfConstant } = ASTUtils; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -142,4 +140,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_owned_primitives_in_ref.mjs b/packages/oxlint-plugin-solid/src/rules/no_owned_primitives_in_ref.mjs new file mode 100644 index 0000000..348615f --- /dev/null +++ b/packages/oxlint-plugin-solid/src/rules/no_owned_primitives_in_ref.mjs @@ -0,0 +1,122 @@ +import { isFunctionNode, jsxPropName, trackImports } from "../utils.mjs"; + +/** + * Primitives that attach to the current owner. + * + * An effect or memo created without an owner is never disposed, and a cleanup + * registered without one never runs. + */ +const OWNED_PRIMITIVES = [ + "createEffect", + "createMemo", + "createProjection", + "createReaction", + "createRenderEffect", + "onCleanup", + "onSettled", +]; + +/** The callbacks a `ref` prop applies, including the ones in a ref array. */ +function getRefCallbacks(expression) { + if (isFunctionNode(expression)) { + return [expression]; + } + + // `ref={[storeElement, autofocus, listen(...)]}` — Solid flattens the array + // and calls each entry, so an inline callback anywhere in it is a ref + // callback like any other. + if (expression?.type === "ArrayExpression") { + return expression.elements.flatMap((element) => + element !== null && isFunctionNode(element) ? [element] : [], + ); + } + + return []; +} + +/** + * Walk a callback body, stopping at nested functions. + * + * Only calls the ref callback makes itself are reported. A nested function may + * be handed to something that supplies an owner, and guessing wrong would flag + * working code. + */ +function walkOwnBody(node, visit) { + if (node === null || typeof node !== "object") { + return; + } + + visit(node); + + for (const [key, value] of Object.entries(node)) { + if (key === "parent") { + continue; + } + + const candidates = Array.isArray(value) ? value : [value]; + for (const candidate of candidates) { + if ( + candidate !== null && + typeof candidate === "object" && + typeof candidate.type === "string" && + !isFunctionNode(candidate) + ) { + walkOwnBody(candidate, visit); + } + } + } +} + +export default { + meta: { + type: "problem", + docs: { + description: + "Disallow creating owner-bound primitives inside a ref callback.", + url: "https://docs.solidjs.com/", + }, + schema: [], + messages: { + ownedPrimitiveInRef: + "`{{name}}` is called inside a ref callback, which runs without an owner — nothing will ever dispose it. Create it in the directive factory instead, and let the returned callback only store the element.", + }, + }, + defaultOptions: [], + create(context) { + const { matchImport, handleImportDeclaration } = trackImports(); + + return { + ImportDeclaration: handleImportDeclaration, + JSXAttribute(node) { + if (jsxPropName(node) !== "ref") { + return; + } + + if (node.value?.type !== "JSXExpressionContainer") { + return; + } + + for (const callback of getRefCallbacks(node.value.expression)) { + walkOwnBody(callback.body, (candidate) => { + if ( + candidate.type !== "CallExpression" || + candidate.callee.type !== "Identifier" || + !matchImport( + OWNED_PRIMITIVES, + candidate.callee.name, + ) + ) { + return; + } + + context.report({ + node: candidate, + messageId: "ownedPrimitiveInRef", + data: { name: candidate.callee.name }, + }); + }); + } + }, + }; + }, +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_proxy_apis.mjs b/packages/oxlint-plugin-solid/src/rules/no_proxy_apis.mjs index 831c01b..4740fc6 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_proxy_apis.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_proxy_apis.mjs @@ -1,20 +1,16 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { isFunctionNode, trackImports, isPropsByName, trace, } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; - // Store APIs that hand back a Proxy. Solid 2.0 exports these from `solid-js`. const PROXY_BACKED_STORE_APIS = new Set([ "createStore", "createProjection", "createOptimisticStore", ]); -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -116,4 +112,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_react_deps.mjs b/packages/oxlint-plugin-solid/src/rules/no_react_deps.mjs index a6ae05f..80360d3 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_react_deps.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_react_deps.mjs @@ -1,8 +1,5 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { isFunctionNode, trace, trackImports } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -14,7 +11,7 @@ export default createRule({ schema: [], messages: { noUselessDep: - "In Solid, `{{name}}` doesn't accept a dependency array because it automatically tracks its dependencies. If you really need to override the list of dependencies, use `on`.", + "In Solid, `{{name}}` doesn't accept a dependency array because it automatically tracks its dependencies. To narrow what is tracked, read only those values in the compute phase.", }, }, defaultOptions: [], @@ -62,4 +59,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_react_specific_props.mjs b/packages/oxlint-plugin-solid/src/rules/no_react_specific_props.mjs index 944b367..4be5f8c 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_react_specific_props.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_react_specific_props.mjs @@ -1,14 +1,11 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { isDOMElementName, jsxGetProp, jsxHasProp } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; const REACT_SPECIFIC_PROPS = [ { from: "className", to: "class" }, { from: "htmlFor", to: "for" }, ]; -export const noReactSpecificPropsRule = createRule({ +export const noReactSpecificPropsRule = { meta: { docs: { description: @@ -68,4 +65,4 @@ export const noReactSpecificPropsRule = createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_setter_in_effect.mjs b/packages/oxlint-plugin-solid/src/rules/no_setter_in_effect.mjs new file mode 100644 index 0000000..322725e --- /dev/null +++ b/packages/oxlint-plugin-solid/src/rules/no_setter_in_effect.mjs @@ -0,0 +1,168 @@ +import { isFunctionNode, trackImports } from "../utils.mjs"; + +const SETTER_INDEX = 1; + +/** `const [value, setValue] = createSignal(...)` -> "setValue". */ +function getSetterName(declarator) { + if (declarator.id.type !== "ArrayPattern") { + return null; + } + + const element = declarator.id.elements[SETTER_INDEX]; + + return element?.type === "Identifier" ? element.name : null; +} + +/** + * The phase that runs untracked and performs the work. Solid 2.0 passes it as + * the second argument; a lone function is still the whole effect. + */ +function getEffectPhase(node) { + const [compute, apply] = node.arguments; + + if (apply) { + return isFunctionNode(apply) ? apply : null; + } + + return isFunctionNode(compute) ? compute : null; +} + +function isSetterCall(node, setterNames) { + return ( + node?.type === "CallExpression" && + node.callee.type === "Identifier" && + setterNames.has(node.callee.name) + ); +} + +/** True when any statement anywhere in the phase writes to a signal or store. */ +function writesSignalsAnywhere(node, setterNames) { + if (node === null || typeof node !== "object") { + return false; + } + + if (isSetterCall(node, setterNames)) { + return true; + } + + return Object.entries(node).some(([key, value]) => { + // `parent` points back up the tree; following it would not terminate. + if (key === "parent") { + return false; + } + + const candidates = Array.isArray(value) ? value : [value]; + + return candidates.some( + (candidate) => + candidate !== null && + typeof candidate === "object" && + !isFunctionNode(candidate) && + writesSignalsAnywhere(candidate, setterNames), + ); + }); +} + +/** True when the phase does nothing but write to signals or stores. */ +function onlyWritesSignals(phase, setterNames) { + if (phase.body.type !== "BlockStatement") { + return isSetterCall(phase.body, setterNames); + } + + const { body } = phase.body; + + return ( + body.length > 0 && + body.every( + (statement) => + statement.type === "ExpressionStatement" && + isSetterCall(statement.expression, setterNames), + ) + ); +} + +export default { + meta: { + type: "problem", + docs: { + description: + "Disallow effects that only copy a reactive value into another signal or store.", + url: "https://docs.solidjs.com/", + }, + schema: [], + messages: { + asyncSetterInEffect: + "This effect awaits and then writes the result into a signal or store. Return the value from a derivation instead: `createMemo(async () => ...)`, or the function form of `createStore` for nested data. Solid coordinates readiness, error propagation, and dropping superseded runs.", + noSetterInEffect: + "This effect only writes to a signal or store. Calculate the value where it is read instead: a derived function, `createMemo`, or a writable derived `createSignal(() => ...)` for a local override.", + }, + }, + defaultOptions: [], + create(context) { + const { matchImport, handleImportDeclaration } = trackImports(); + const setterNames = new Set(); + + return { + ImportDeclaration: handleImportDeclaration, + VariableDeclarator(node) { + if ( + node.init?.type !== "CallExpression" || + node.init.callee.type !== "Identifier" || + !matchImport( + ["createSignal", "createStore"], + node.init.callee.name, + ) + ) { + return; + } + + const setterName = getSetterName(node); + if (setterName) { + setterNames.add(setterName); + } + }, + CallExpression(node) { + if ( + node.callee.type !== "Identifier" || + !matchImport( + ["createEffect", "createRenderEffect"], + node.callee.name, + ) + ) { + return; + } + + const phase = getEffectPhase(node); + if (!phase) { + return; + } + + // Asynchrony is checked first because it is the more specific + // diagnosis: `onlyWritesSignals` also matches a lone + // `setResults(await search(q))`, and the async message is the + // one that names readiness and superseded runs. + // + // `onlyWritesSignals` never sees the multi-statement shape at + // all — the `await` is not a setter call — which is why an + // async phase gets its own search. + if ( + phase.async === true && + writesSignalsAnywhere(phase.body, setterNames) + ) { + context.report({ + node: phase, + messageId: "asyncSetterInEffect", + }); + return; + } + + if (onlyWritesSignals(phase, setterNames)) { + context.report({ + node: phase, + messageId: "noSetterInEffect", + }); + } + }, + }; + }, +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_unknown_namespaces.mjs b/packages/oxlint-plugin-solid/src/rules/no_unknown_namespaces.mjs index 47695d1..93243c6 100644 --- a/packages/oxlint-plugin-solid/src/rules/no_unknown_namespaces.mjs +++ b/packages/oxlint-plugin-solid/src/rules/no_unknown_namespaces.mjs @@ -1,7 +1,4 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { isDOMElementName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; // Solid 2.0 removed every Solid-specific JSX namespace. What is left are the // XML namespaces the DOM itself defines. const xmlNamespaces = ["xmlns", "xlink"]; @@ -17,7 +14,7 @@ const replacements = { style: "the `style` prop with an object value", use: "a ref callback or directive factory, composing with a ref array", }; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -107,4 +104,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/no_untracked_effect_read.mjs b/packages/oxlint-plugin-solid/src/rules/no_untracked_effect_read.mjs new file mode 100644 index 0000000..007885c --- /dev/null +++ b/packages/oxlint-plugin-solid/src/rules/no_untracked_effect_read.mjs @@ -0,0 +1,175 @@ +import { isFunctionNode, isPropsByName, trackImports } from "../utils.mjs"; + +const GETTER_INDEX = 0; + +/** `const [value, setValue] = createSignal(...)` -> "value". */ +function getGetterName(declarator) { + if (declarator.id.type !== "ArrayPattern") { + return null; + } + + const element = declarator.id.elements[GETTER_INDEX]; + + return element?.type === "Identifier" ? element.name : null; +} + +/** Every name a function's parameters bind, however they are destructured. */ +function collectParameterNames(fn, into) { + const pending = [...fn.params]; + + while (pending.length > 0) { + const pattern = pending.pop(); + if (pattern === null || typeof pattern !== "object") { + continue; + } + + if (pattern.type === "Identifier") { + into.add(pattern.name); + continue; + } + + for (const [key, value] of Object.entries(pattern)) { + if (key === "parent") { + continue; + } + + const candidates = Array.isArray(value) ? value : [value]; + for (const candidate of candidates) { + if (candidate !== null && typeof candidate === "object") { + pending.push(candidate); + } + } + } + } +} + +/** + * Walk the phase body, stopping at nested functions. + * + * A read inside a nested function runs whenever that function is called, which + * is a different question from whether this phase subscribes to it. + */ +function walkPhase(node, visit) { + if (node === null || typeof node !== "object") { + return; + } + + visit(node); + + for (const [key, value] of Object.entries(node)) { + if (key === "parent") { + continue; + } + + const candidates = Array.isArray(value) ? value : [value]; + for (const candidate of candidates) { + if ( + candidate !== null && + typeof candidate === "object" && + typeof candidate.type === "string" && + !isFunctionNode(candidate) + ) { + walkPhase(candidate, visit); + } + } + } +} + +export default { + meta: { + type: "problem", + docs: { + description: + "Disallow reading reactive values in the untracked apply phase of an effect.", + url: "https://docs.solidjs.com/", + }, + schema: [], + messages: { + untrackedEffectRead: + "'{{name}}' is read in the effect's apply phase, which runs untracked — changes to it will not re-run this effect. Read it in the compute phase and take it as an argument instead.", + }, + }, + defaultOptions: [], + create(context) { + const { matchImport, handleImportDeclaration } = trackImports(); + const getterNames = new Set(); + + return { + ImportDeclaration: handleImportDeclaration, + VariableDeclarator(node) { + if ( + node.init?.type !== "CallExpression" || + node.init.callee.type !== "Identifier" || + !matchImport(["createSignal"], node.init.callee.name) + ) { + return; + } + + const getterName = getGetterName(node); + if (getterName) { + getterNames.add(getterName); + } + }, + CallExpression(node) { + if ( + node.callee.type !== "Identifier" || + !matchImport( + ["createEffect", "createRenderEffect"], + node.callee.name, + ) + ) { + return; + } + + // Only the two-argument form has an apply phase. A lone + // function is the whole effect and tracks throughout. + const [compute, apply] = node.arguments; + if (!compute || !apply || !isFunctionNode(apply)) { + return; + } + + // Whatever the compute phase handed over arrives as a + // parameter, so those names are already settled values. + const settled = new Set(); + collectParameterNames(apply, settled); + + walkPhase(apply.body, (candidate) => { + // `signal()` — a getter call that did not come through the + // compute phase. + if ( + candidate.type === "CallExpression" && + candidate.callee.type === "Identifier" && + getterNames.has(candidate.callee.name) && + !settled.has(candidate.callee.name) + ) { + context.report({ + node: candidate, + messageId: "untrackedEffectRead", + data: { name: candidate.callee.name }, + }); + return; + } + + // `props.value` — a props read is reactive for the same + // reason and is just as invisible to the tracker here. + if ( + candidate.type === "MemberExpression" && + !candidate.computed && + candidate.object.type === "Identifier" && + isPropsByName(candidate.object.name) && + !settled.has(candidate.object.name) && + candidate.property.type === "Identifier" + ) { + context.report({ + node: candidate, + messageId: "untrackedEffectRead", + data: { + name: `${candidate.object.name}.${candidate.property.name}`, + }, + }); + } + }); + }, + }; + }, +}; diff --git a/packages/oxlint-plugin-solid/src/rules/prefer_class_object.mjs b/packages/oxlint-plugin-solid/src/rules/prefer_class_object.mjs index e8a734b..12e5d88 100644 --- a/packages/oxlint-plugin-solid/src/rules/prefer_class_object.mjs +++ b/packages/oxlint-plugin-solid/src/rules/prefer_class_object.mjs @@ -1,14 +1,11 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { jsxPropName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; const DEFAULT_CLASSNAMES = ["cn", "clsx", "classnames"]; -export default createRule({ +export default { meta: { type: "problem", docs: { description: - "Enforce the `class` object/array form over the removed `classlist` prop or a classnames helper. Solid 2.0 accepts `{ [class: string]: boolean }` on `class` directly.", + "Enforce the `class` object/array form over the removed `classList` prop or a classnames helper. Solid 2.0 accepts `{ [class: string]: boolean }` on `class` directly.", url: "https://docs.solidjs.com/", }, fixable: "code", @@ -33,7 +30,7 @@ export default createRule({ ], messages: { classlistRemoved: - "The `classlist` prop was removed in Solid 2.0. Pass the object to `class` instead.", + "The `classList` prop was removed in Solid 2.0. Pass the object to `class` instead.", preferClassObject: "Pass the object to `class` directly instead of through {{ classnames }}.", }, @@ -46,8 +43,11 @@ export default createRule({ JSXAttribute(node) { const name = jsxPropName(node); - // `classlist={{...}}` no longer exists; the object belongs on `class`. - if (name === "classlist") { + // `classList={{...}}` no longer exists; the object belongs on + // `class`. JSX prop names are case-sensitive, so matching only + // the lowercase spelling missed every real Solid 1 call site — + // which is the code this rule exists to migrate. + if (name === "classList" || name === "classlist") { context.report({ node, messageId: "classlistRemoved", @@ -95,4 +95,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/prefer_for.mjs b/packages/oxlint-plugin-solid/src/rules/prefer_for.mjs index 9bf9395..63c39d4 100644 --- a/packages/oxlint-plugin-solid/src/rules/prefer_for.mjs +++ b/packages/oxlint-plugin-solid/src/rules/prefer_for.mjs @@ -1,9 +1,7 @@ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getPropertyName } from "@eslint-community/eslint-utils"; import { isFunctionNode, isJSXElementOrFragment } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getPropertyName } = ASTUtils; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -89,4 +87,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/prefer_show.mjs b/packages/oxlint-plugin-solid/src/rules/prefer_show.mjs index 74d4126..51114c7 100644 --- a/packages/oxlint-plugin-solid/src/rules/prefer_show.mjs +++ b/packages/oxlint-plugin-solid/src/rules/prefer_show.mjs @@ -1,10 +1,7 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { getSourceCode } from "../compat.mjs"; import { isJSXElementOrFragment } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; const EXPENSIVE_TYPES = ["JSXElement", "JSXFragment", "Identifier"]; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -89,4 +86,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/reactivity.mjs b/packages/oxlint-plugin-solid/src/rules/reactivity.mjs index 19541fb..2cc1ca0 100644 --- a/packages/oxlint-plugin-solid/src/rules/reactivity.mjs +++ b/packages/oxlint-plugin-solid/src/rules/reactivity.mjs @@ -2,7 +2,7 @@ * File overview here, scroll to bottom. * @link https://github.com/solidjs-community/eslint-plugin-solid/blob/main/docs/reactivity.md */ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getFunctionHeadLocation } from "@eslint-community/eslint-utils"; import { traverse } from "estraverse"; import { findVariable, getSourceCode } from "../compat.mjs"; @@ -19,9 +19,6 @@ import { isJSXElementOrFragment, trace, } from "../utils.mjs"; -const { getFunctionHeadLocation } = ASTUtils; -const createRule = ESLintUtils.RuleCreator.withoutDocs; - // Solid 2.0 removed `createResource`, `createMutable`, `indexArray` and // ``. Their branches below are kept structurally but matched against // `__removed_*` names so they can never fire; delete them once no Solid 1 @@ -174,7 +171,7 @@ const getReturnedVar = (id, context) => { } return null; }; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -203,7 +200,7 @@ export default createRule({ noWrite: "The reactive variable '{{name}}' should not be reassigned or altered directly.", untrackedReactive: - "The reactive variable '{{name}}' should be used within JSX, a tracked scope (like createEffect), or inside an event handler function, or else changes will be ignored.", + "The reactive variable '{{name}}' should be used within JSX, a tracked scope (like createEffect), or inside an event handler function, or else changes will be ignored. To keep it reactive, derive it rather than copy it: a plain function, `createMemo`, or `createSignal(() => ...)` / `createStore(() => ...)` for a local override that a new source value should replace.", expectedFunctionGotExpression: "The reactive variable '{{name}}' should be wrapped in a function for reactivity. This includes event handler bindings on native elements, which are not reactive like other JSX props.", badSignal: @@ -223,7 +220,12 @@ export default createRule({ customReactiveFunctions: [], }, ], - create(context, [options]) { + create(context) { + // `RuleCreator` used to merge `defaultOptions` into `context.options` + // and hand the result to a second `create` parameter. Without that + // wrapper the parameter is undefined, so the default is applied here. + const options = context.options[0] ?? { customReactiveFunctions: [] }; + const warnShouldDestructure = (node, nth) => context.report({ node, @@ -1397,4 +1399,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/self_closing_comp.mjs b/packages/oxlint-plugin-solid/src/rules/self_closing_comp.mjs index d69b12f..1236362 100644 --- a/packages/oxlint-plugin-solid/src/rules/self_closing_comp.mjs +++ b/packages/oxlint-plugin-solid/src/rules/self_closing_comp.mjs @@ -1,8 +1,5 @@ -import { ESLintUtils } from "@typescript-eslint/utils"; - import { getSourceCode } from "../compat.mjs"; import { isDOMElementName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; function isComponent(node) { return ( (node.name.type === "JSXIdentifier" && @@ -31,7 +28,7 @@ function childrenIsMultilineSpaces(node) { * This rule is adapted from eslint-plugin-react's self-closing-comp rule under the MIT license, * with some enhancements. Thank you for your work! */ -export default createRule({ +export default { meta: { type: "layout", docs: { @@ -150,4 +147,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/src/rules/style_prop.mjs b/packages/oxlint-plugin-solid/src/rules/style_prop.mjs index efb7719..74983f3 100644 --- a/packages/oxlint-plugin-solid/src/rules/style_prop.mjs +++ b/packages/oxlint-plugin-solid/src/rules/style_prop.mjs @@ -1,15 +1,13 @@ -import { ESLintUtils, ASTUtils } from "@typescript-eslint/utils"; +import { getPropertyName, getStaticValue } from "@eslint-community/eslint-utils"; import kebabCase from "kebab-case"; import { all as allCssProperties } from "known-css-properties"; import parse from "style-to-object"; import { getScope } from "../compat.mjs"; import { jsxPropName } from "../utils.mjs"; -const createRule = ESLintUtils.RuleCreator.withoutDocs; -const { getPropertyName, getStaticValue } = ASTUtils; const lengthPercentageRegex = /\b(?:width|height|margin|padding|border-width|font-size)\b/i; -export default createRule({ +export default { meta: { type: "problem", docs: { @@ -152,4 +150,4 @@ export default createRule({ }, }; }, -}); +}; diff --git a/packages/oxlint-plugin-solid/tests/helpers.mjs b/packages/oxlint-plugin-solid/tests/helpers.mjs index 5d053c6..0b873db 100644 --- a/packages/oxlint-plugin-solid/tests/helpers.mjs +++ b/packages/oxlint-plugin-solid/tests/helpers.mjs @@ -5,8 +5,7 @@ const FIXTURE_FILE_PATH = "src/rule_case.tsx"; const OXLINT_CONFIG_PATH = "./.oxlintrc.jsonc"; const TEMP_DIRECTORY_PATTERN = "/tmp/oxlint-plugin-solid-XXXXXX"; const PACKAGE_RUNTIME_DEPENDENCIES = [ - "@typescript-eslint", - "eslint", + "@eslint-community", "estraverse", "is-html", "kebab-case", diff --git a/packages/oxlint-plugin-solid/tests/rule_cases.mjs b/packages/oxlint-plugin-solid/tests/rule_cases.mjs index 4622bdf..3db0988 100644 --- a/packages/oxlint-plugin-solid/tests/rule_cases.mjs +++ b/packages/oxlint-plugin-solid/tests/rule_cases.mjs @@ -9,10 +9,13 @@ const expectedRuleIds = Object.freeze([ "no-array-handlers", "no-destructure", "no-innerhtml", + "no-owned-primitives-in-ref", "no-proxy-apis", "no-react-deps", "no-react-specific-props", + "no-setter-in-effect", "no-unknown-namespaces", + "no-untracked-effect-read", "prefer-arrow-components", "prefer-class-object", "prefer-for", @@ -123,6 +126,52 @@ createEffect(() => { `, ruleId: "no-react-specific-props", }), + Object.freeze({ + code: `import { createEffect, createSignal } from "solid-js"; + +const [firstName] = createSignal("Ada"); +const [, setFullName] = createSignal(""); + +createEffect( + () => firstName(), + (first) => { + setFullName(first + " Lovelace"); + }, +); +`, + ruleId: "no-setter-in-effect", + }), + Object.freeze({ + code: `import { createEffect, createSignal } from "solid-js"; + +const [query] = createSignal(""); +const [, setResults] = createSignal([]); + +createEffect( + () => query(), + async (value) => { + const found = await fetch(value).then((response) => response.json()); + setResults(found); + }, +); +`, + ruleId: "no-setter-in-effect", + }), + Object.freeze({ + code: `import { createEffect, createSignal } from "solid-js"; + +const [roomId] = createSignal("lobby"); +const [theme] = createSignal("dark"); + +createEffect( + () => roomId(), + (id) => { + connect(id, theme()); + }, +); +`, + ruleId: "no-untracked-effect-read", + }), Object.freeze({ code: `export const View = () => { return
Hello
; @@ -141,6 +190,51 @@ export function View(props: Props): JSX.Element { `, ruleId: "prefer-arrow-components", }), + Object.freeze({ + code: `export const View = (props) => { + return
Hello
; +}; +`, + ruleId: "prefer-class-object", + }), + Object.freeze({ + code: `import { + createErrorBoundary, + createLoadingBoundary, + createRevealOrder, +} from "@solidjs/web"; + +export const boundaries = [ + createErrorBoundary, + createLoadingBoundary, + createRevealOrder, +]; +`, + ruleId: "imports", + }), + Object.freeze({ + code: `import { createEffect, createSignal, onCleanup } from "solid-js"; + +export const View = () => { + const [width] = createSignal(0); + + return ( +
{ + createEffect( + () => width(), + (value) => element.setAttribute("data-width", value), + ); + onCleanup(() => element.remove()); + }} + > + Hello +
+ ); +}; +`, + ruleId: "no-owned-primitives-in-ref", + }), Object.freeze({ code: `const cn = (classes) => { return classes; diff --git a/packages/typebuddy/rules/async_rule.ts b/packages/typebuddy/rules/async_rule.ts index c074473..0ffedb4 100644 --- a/packages/typebuddy/rules/async_rule.ts +++ b/packages/typebuddy/rules/async_rule.ts @@ -1,4 +1,5 @@ import { throwsDeliberately } from "./deliberate_throw.js"; +import { getTypeBuddyImportInsertion } from "./typebuddy_import.js"; type AstNode = { type: string; @@ -29,99 +30,6 @@ type RuleFixer = { insertTextBeforeRange(range: [number, number], text: string): unknown; }; -function isNode(value: unknown): value is AstNode { - return typeof value === "object" && value !== null && "type" in value; -} - -function getProgram(node: AstNode): AstNode | null { - let current: AstNode | undefined = node; - - while (current?.parent) { - current = current.parent; - } - - return current?.type === "Program" ? current : null; -} - -function isIdentifierNamed(node: unknown, name: string): boolean { - return isNode(node) && node.type === "Identifier" && node["name"] === name; -} - -function isImportDeclaration(node: unknown): node is AstNode { - return isNode(node) && node.type === "ImportDeclaration"; -} - -function getStringLiteralValue(node: unknown): string | null { - if (!isNode(node)) { - return null; - } - - if ( - (node.type === "Literal" || node.type === "StringLiteral") && - typeof node["value"] === "string" - ) { - return node["value"]; - } - - return null; -} - -function getProgramBody(node: AstNode): AstNode[] { - if (!Array.isArray(node.body)) { - return []; - } - - return node.body.filter(isNode); -} - -function hasTypeBuddyHelperImport(program: AstNode, helperName: "err") { - return getProgramBody(program).some((statement) => { - if (!isImportDeclaration(statement)) { - return false; - } - - if ( - getStringLiteralValue(statement["source"]) !== "@murky-web/typebuddy" - ) { - return false; - } - - if (statement["importKind"] === "type") { - return false; - } - - const specifiers = Array.isArray(statement["specifiers"]) - ? statement["specifiers"] - : []; - - return specifiers.some((specifier) => { - return ( - isNode(specifier) && - specifier.type === "ImportSpecifier" && - isIdentifierNamed(specifier["local"], helperName) - ); - }); - }); -} - -function getTypeBuddyImportInsertRange( - program: AstNode, -): [number, number] | null { - const body = getProgramBody(program); - const imports = body.filter(isImportDeclaration); - const anchor = imports.at(-1) ?? body[0] ?? program; - - if (!Array.isArray(anchor.range) || anchor.range.length < 2) { - return null; - } - - if (imports.length > 0) { - return [anchor.range[1], anchor.range[1]]; - } - - return [anchor.range[0], anchor.range[0]]; -} - function hasTryCatch(nodes: AstNode[]): boolean { return nodes.some((node) => { return node.type === "TryStatement"; @@ -138,7 +46,9 @@ function isCallArgumentCallback(node: AstNode): boolean { return false; } - return Array.isArray(parent["arguments"]) && parent["arguments"].includes(node); + return ( + Array.isArray(parent["arguments"]) && parent["arguments"].includes(node) + ); } const rule = { @@ -154,81 +64,143 @@ const rule = { return []; } - const program = getProgram(node); - if (!program || hasTypeBuddyHelperImport(program, "err")) { - return []; - } - - const insertRange = getTypeBuddyImportInsertRange(program); - if (!insertRange) { + const insertion = getTypeBuddyImportInsertion(node, "err"); + if (!insertion) { return []; } scheduledErrImport = true; - const hasImports = insertRange[0] !== 0; - const importText = hasImports - ? '\nimport { err } from "@murky-web/typebuddy";' - : 'import { err } from "@murky-web/typebuddy";\n'; - - return [fixer.insertTextBeforeRange(insertRange, importText)]; + return [ + fixer.insertTextBeforeRange(insertion.range, insertion.text), + ]; } - function getIndentation(node: AstNode): string { - const lines = sourceCode.getText(node).split("\n"); - // `split` always yields at least one element, but - // `noUncheckedIndexedAccess` cannot know that. - const firstLine = lines[0] ?? ""; - const match = /^\s*/.exec(firstLine); - return match ? match[0] : ""; + // Two zero-width insertions leave the body's own text untouched, byte + // for byte. Rebuilding it statement by statement used to strip leading + // whitespace on every line — including lines inside template literals, + // which silently changed string values. + function wrapBlockFixes( + bodyNode: AstNode, + fixer: RuleFixer, + ): unknown[] | null { + const range = bodyNode.range; + if (!Array.isArray(range) || range.length < 2) { + return null; + } + + return [ + fixer.insertTextBeforeRange( + [range[0] + 1, range[0] + 1], + "\ntry {", + ), + fixer.insertTextBeforeRange( + [range[1] - 1, range[1] - 1], + "} catch {\nreturn err();\n}\n", + ), + ]; } - function wrapInTryCatch(node: AstNode): string { - const body = Array.isArray(node.body) ? node.body : []; - const indent = getIndentation(node); - const innerIndent = `${indent} `; - const bodyText = body - .map((statement) => { - const text = sourceCode.getText(statement); - return `${innerIndent}${text.replaceAll(/^\s*/gm, "")}`; - }) - .join("\n"); + /** + * Wrap an expression-bodied arrow's value in a block with try/catch. + * + * The body node's range stops inside any wrapping parentheses, so + * replacing it in `async () => ({ a: 1 })` would leave the parentheses + * around a block and produce `({ try { ... } })`. Walking left over + * those parentheses first, then inserting on either side, keeps the + * expression itself untouched — parentheses and all. + */ + function wrapExpressionBodyFixes( + node: AstNode, + bodyNode: AstNode, + fixer: RuleFixer, + ): unknown[] | null { + const arrowRange = node.range; + const bodyRange = bodyNode.range; + if ( + !Array.isArray(arrowRange) || + arrowRange.length < 2 || + !Array.isArray(bodyRange) || + bodyRange.length < 2 + ) { + return null; + } + + let prefix = sourceCode + .getText(node) + .slice(0, bodyRange[0] - arrowRange[0]); + while (prefix.trimEnd().endsWith("(")) { + prefix = prefix.trimEnd().slice(0, -1); + } + + const start = arrowRange[0] + prefix.length; + // The arrow's own end already sits past any closing parenthesis. + const end = arrowRange[1]; + if (start >= end) { + return null; + } - return `{ -${indent}try { -${bodyText} -${indent}} catch { -${innerIndent}return err(); -${indent}} -}`; + return [ + fixer.insertTextBeforeRange( + [start, start], + "{\ntry {\nreturn ", + ), + fixer.insertTextBeforeRange( + [end, end], + ";\n} catch {\nreturn err();\n}\n}", + ), + ]; } function checkFunction(node: AstNode) { if (node.async !== true) return; if (isCallArgumentCallback(node)) return; const bodyNode = node.body; - if ( - !bodyNode || - Array.isArray(bodyNode) || - bodyNode.type !== "BlockStatement" - ) { + if (!bodyNode || Array.isArray(bodyNode)) return; + + // A function that throws has already decided its failure is not a + // value. Wrapping it in try/catch would swallow that decision. + if (throwsDeliberately(bodyNode)) return; + + // An expression-bodied arrow (`async () => fetch(url)`) returns the + // one value the try/catch exists to guard, so it needs a block + // before it can get one. + if (bodyNode.type !== "BlockStatement") { + context.report({ + node, + messageId: "missingTryCatch", + fix(fixer) { + const wrapFixes = wrapExpressionBodyFixes( + node, + bodyNode, + fixer, + ); + if (!wrapFixes) { + return null; + } + + return [ + ...ensureErrImportFixes(node, fixer), + ...wrapFixes, + ]; + }, + }); return; } const body = Array.isArray(bodyNode.body) ? bodyNode.body : []; if (hasTryCatch(body)) return; - // A function that throws has already decided its failure is not a - // value. Wrapping it in try/catch would swallow that decision. - if (throwsDeliberately(bodyNode)) return; context.report({ node, messageId: "missingTryCatch", fix(fixer) { - return [ - ...ensureErrImportFixes(node, fixer), - fixer.replaceText(bodyNode, wrapInTryCatch(bodyNode)), - ]; + const wrapFixes = wrapBlockFixes(bodyNode, fixer); + if (!wrapFixes) { + return null; + } + + return [...ensureErrImportFixes(node, fixer), ...wrapFixes]; }, }); } diff --git a/packages/typebuddy/rules/deliberate_throw.ts b/packages/typebuddy/rules/deliberate_throw.ts index a9668f8..e62dbed 100644 --- a/packages/typebuddy/rules/deliberate_throw.ts +++ b/packages/typebuddy/rules/deliberate_throw.ts @@ -1,17 +1,4 @@ -type AstNode = { - type: string; - [key: string]: unknown; -}; - -const FUNCTION_TYPES = new Set([ - "ArrowFunctionExpression", - "FunctionDeclaration", - "FunctionExpression", -]); - -function isNode(value: unknown): value is AstNode { - return typeof value === "object" && value !== null && "type" in value; -} +import { walkOwnSubtree } from "./own_subtree.js"; /** * Whether a function body throws on purpose. @@ -34,40 +21,15 @@ function isNode(value: unknown): value is AstNode { * @returns {boolean} True when the body throws outside any nested function. */ function throwsDeliberately(body: unknown): boolean { - if (!isNode(body)) { - return false; - } - - const pending: AstNode[] = [body]; - while (pending.length > 0) { - // `pop` on a non-empty array always yields a node; the length check above is - // the loop condition, but `noUncheckedIndexedAccess` cannot see that. - const current = pending.pop(); - if (current === undefined) { - break; - } - - if (current.type === "ThrowStatement") { - return true; - } - - for (const [key, value] of Object.entries(current)) { - // `parent` points back up the tree; following it would never terminate. - if (key === "parent") { - continue; - } + let found = false; - const candidates = Array.isArray(value) ? value : [value]; - for (const candidate of candidates) { - if (!isNode(candidate) || FUNCTION_TYPES.has(candidate.type)) { - continue; - } - pending.push(candidate); - } + walkOwnSubtree(body, (node) => { + if (node.type === "ThrowStatement") { + found = true; } - } + }); - return false; + return found; } export { throwsDeliberately }; diff --git a/packages/typebuddy/rules/maybe_promise_rule.ts b/packages/typebuddy/rules/maybe_promise_rule.ts index 0a71774..37de4a8 100644 --- a/packages/typebuddy/rules/maybe_promise_rule.ts +++ b/packages/typebuddy/rules/maybe_promise_rule.ts @@ -1,4 +1,6 @@ import { throwsDeliberately } from "./deliberate_throw.js"; +import { getTypeBuddyImportInsertion } from "./typebuddy_import.js"; +import { walkOwnSubtree } from "./own_subtree.js"; type AstNode = { type: string; @@ -11,7 +13,7 @@ type RuleContext = { report(descriptor: { node: unknown; messageId: string; - fix(fixer: { + fix?(fixer: { replaceText(node: unknown, text: string): unknown; insertTextBeforeRange( range: [number, number], @@ -48,48 +50,34 @@ function isCallArgumentCallback(node: AstNode): boolean { return false; } - return Array.isArray(parent["arguments"]) && parent["arguments"].includes(node); + return ( + Array.isArray(parent["arguments"]) && parent["arguments"].includes(node) + ); } -function isIdentifierNamed(node: unknown, name: string): boolean { - return isNode(node) && node.type === "Identifier" && node["name"] === name; -} +/** + * Return-type names whose type argument is the value an async function settles + * on. `AsyncResult` is the preferred spelling of `MaybePromise`, so a function + * annotated with it has to be read the same way — otherwise the rule goes blind + * on exactly the code the package tells people to write. + */ +const AWAITED_TYPE_NAMES = new Set(["AsyncResult", "MaybePromise", "Promise"]); -function getProgram(node: AstNode): AstNode | null { +function isAmbient(node: AstNode): boolean { let current: AstNode | undefined = node; - while (current?.parent) { + while (current) { + if (current["declare"] === true) { + return true; + } current = current.parent; } - return current?.type === "Program" ? current : null; -} - -function isImportDeclaration(node: unknown): node is AstNode { - return isNode(node) && node.type === "ImportDeclaration"; -} - -function getStringLiteralValue(node: unknown): string | null { - if (!isNode(node)) { - return null; - } - - if ( - (node.type === "Literal" || node.type === "StringLiteral") && - typeof node["value"] === "string" - ) { - return node["value"]; - } - - return null; + return false; } -function getProgramBody(node: AstNode): AstNode[] { - if (!Array.isArray(node["body"])) { - return []; - } - - return node["body"].filter(isNode); +function isIdentifierNamed(node: unknown, name: string): boolean { + return isNode(node) && node.type === "Identifier" && node["name"] === name; } function isResultHelperCall(node: unknown, name: "ok" | "err"): boolean { @@ -148,61 +136,15 @@ function getObjectPropertyValue( } function isBooleanLiteral(node: unknown, expected: boolean): boolean { - return isNode(node) && node.type === "Literal" && node["value"] === expected; + return ( + isNode(node) && node.type === "Literal" && node["value"] === expected + ); } function isNullLiteral(node: unknown): boolean { return isNode(node) && node.type === "Literal" && node["value"] === null; } -function hasTypeBuddyHelperImport(program: AstNode, helperName: "ok" | "err") { - return getProgramBody(program).some((statement) => { - if (!isImportDeclaration(statement)) { - return false; - } - - if ( - getStringLiteralValue(statement["source"]) !== "@murky-web/typebuddy" - ) { - return false; - } - - if (statement["importKind"] === "type") { - return false; - } - - const specifiers = Array.isArray(statement["specifiers"]) - ? statement["specifiers"] - : []; - - return specifiers.some((specifier) => { - return ( - isNode(specifier) && - specifier.type === "ImportSpecifier" && - isIdentifierNamed(specifier["local"], helperName) - ); - }); - }); -} - -function getTypeBuddyImportInsertRange( - program: AstNode, -): [number, number] | null { - const body = getProgramBody(program); - const imports = body.filter(isImportDeclaration); - const anchor = imports.at(-1) ?? body[0] ?? program; - - if (!Array.isArray(anchor["range"]) || anchor["range"].length < 2) { - return null; - } - - if (imports.length > 0) { - return [anchor["range"][1], anchor["range"][1]]; - } - - return [anchor["range"][0], anchor["range"][0]]; -} - const rule = { create(context: RuleContext) { const sourceCode = context.getSourceCode(); @@ -216,7 +158,9 @@ const rule = { const typeName = node["typeName"]; if (!isNode(typeName) || typeName.type !== "Identifier") return null; - return typeof typeName["name"] === "string" ? typeName["name"] : null; + return typeof typeName["name"] === "string" + ? typeName["name"] + : null; } function getTypeArgument(node: AstNode): AstNode | null { @@ -227,8 +171,23 @@ const rule = { return firstParam; } - function getPromiseTypeArgument(node: AstNode): AstNode | null { - if (getTypeName(node) !== "Promise") return null; + /** + * The awaited type behind an async function's return annotation. + * + * Every spelling counts. Matching only `Promise` meant the rule read the + * return type of code it had not migrated yet and went blind the moment + * `MaybePromise` was there — so a bare `return;` in a + * `MaybePromise` function never became `return ok();`, while the + * identical `Promise` function was fixed. It only ever worked + * because the rename and this lookup happen in the same pass, off the + * same AST. + */ + function getAwaitedTypeArgument(node: AstNode): AstNode | null { + const typeName = getTypeName(node); + if (typeName === null || !AWAITED_TYPE_NAMES.has(typeName)) { + return null; + } + return getTypeArgument(node); } @@ -241,32 +200,19 @@ const rule = { return []; } - const program = getProgram(node); - if (!program || hasTypeBuddyHelperImport(program, helperName)) { - return []; - } - - const insertRange = getTypeBuddyImportInsertRange(program); - if (!insertRange) { + const insertion = getTypeBuddyImportInsertion(node, helperName); + if (!insertion) { return []; } scheduledHelperImports.add(helperName); - const hasImports = insertRange[0] !== 0; - const importText = hasImports - ? `\nimport { ${helperName} } from "@murky-web/typebuddy";` - : `import { ${helperName} } from "@murky-web/typebuddy";\n`; - - return [fixer.insertTextBeforeRange(insertRange, importText)]; + return [ + fixer.insertTextBeforeRange(insertion.range, insertion.text), + ]; } - function checkReturnType(node: AstNode) { - if (!isAsyncFunction(node)) return; - if (isCallArgumentCallback(node)) return; - // Same exemption as `require-try-catch`: a deliberate throw is not a - // result waiting to be wrapped. - if (throwsDeliberately(node["body"])) return; + function reportPromiseReturnType(node: AstNode) { if (!isNode(node["returnType"])) return; const typeAnnotation = node["returnType"]["typeAnnotation"]; @@ -283,6 +229,33 @@ const rule = { }); } + function checkReturnType(node: AstNode) { + if (!isAsyncFunction(node)) return; + if (isCallArgumentCallback(node)) return; + // Same exemption as `require-try-catch`: a deliberate throw is not a + // result waiting to be wrapped. + if (throwsDeliberately(node["body"])) return; + + reportPromiseReturnType(node); + } + + /** + * A type-level signature — an interface method, a function type, an + * overload — can never carry `async`, so routing it through + * `checkReturnType` meant these three visitors never fired at all. + * `Promise` in a signature is the same contract the rule rewrites + * everywhere else, so it gets reported here without the async gate. + * + * Ambient declarations are the exception: `declare` describes code the + * project does not own, and rewriting a third-party framework's + * callback shape to `MaybePromise` would be a lie about that API. + */ + function checkSignatureReturnType(node: AstNode) { + if (isAmbient(node)) return; + + reportPromiseReturnType(node); + } + function wrapReturnValue( node: AstNode, isAsync: boolean, @@ -383,6 +356,95 @@ const rule = { } } + function collectOwnReturns(root: unknown): AstNode[] { + const returns: AstNode[] = []; + + walkOwnSubtree(root, (candidate) => { + if (candidate.type === "TryStatement") { + return false; + } + + if (candidate.type === "ReturnStatement") { + returns.push(candidate); + } + + return true; + }); + + return returns; + } + + function isSettledFailure(argument: unknown): boolean { + return ( + (isNode(argument) && + argument.type === "Identifier" && + argument["name"] === "FAILED_PROMISE") || + isResultHelperCall(argument, "err") + ); + } + + /** + * Push a single `return` inside a catch block towards `err()`. + * + * Only the lossless rewrites carry a fix. Replacing + * `return ok("defaults")` with `return err()` would silently drop the + * author's fallback, and the old behaviour — appending `return err()` + * after it — produced unreachable code and a report that could never be + * satisfied. Where the fallback matters, moving it into the try block is + * a decision the author has to make, so the rule reports and stops. + */ + function reportCatchReturn(tryNode: AstNode, statement: AstNode) { + const argument = statement["argument"]; + + if (isSettledFailure(argument)) return; + + // `return;` — adding the result is pure addition, nothing is lost. + if (!isNode(argument)) { + context.report({ + node: statement, + messageId: "returnFailedPromise", + fix(fixer) { + return [ + ...ensureResultHelperImportFixes( + tryNode, + fixer, + "err", + ), + fixer.replaceText(statement, "return err()"), + ]; + }, + }); + return; + } + + if (hasIsErrorFlag(argument, true)) { + // `{ isError: true, value: null }` says the right thing the + // long way round; `err()` is the same value. + if (isNullLiteral(getObjectPropertyValue(argument, "value"))) { + context.report({ + node: argument, + messageId: "preferErrResult", + fix(fixer) { + return [ + ...ensureResultHelperImportFixes( + tryNode, + fixer, + "err", + ), + fixer.replaceText(argument, "err()"), + ]; + }, + }); + } + return; + } + + context.report({ + node: statement, + messageId: "returnFailedPromise", + }); + } + function processTryCatch(node: AstNode) { let parent = node.parent; let isAsync = false; @@ -407,109 +469,89 @@ const rule = { let returnType: AstNode | null = null; if (isNode(parentFunction["returnType"])) { - const typeAnnotation = parentFunction["returnType"]["typeAnnotation"]; + const typeAnnotation = + parentFunction["returnType"]["typeAnnotation"]; if (isTypeReference(typeAnnotation)) { - returnType = getPromiseTypeArgument(typeAnnotation); + returnType = getAwaitedTypeArgument(typeAnnotation); } } - const blockBody = - isNode(node["block"]) && Array.isArray(node["block"]["body"]) - ? node["block"]["body"] - : []; - for (const statement of blockBody) { - if (isNode(statement) && statement.type === "ReturnStatement") { - wrapReturnValue(statement, isAsync, returnType); - } + // Every return the try block owns, not just the ones sitting + // directly in it. `try { if (flag) return "early"; return "late"; }` + // used to have only `"late"` wrapped, because the walk never looked + // inside the `if`. + // + // A nested `try` is left alone: the visitor fires for it separately, + // and its own catch branch has to stay the failure path rather than + // be read as another try-block return. + for (const statement of collectOwnReturns(node["block"])) { + wrapReturnValue(statement, isAsync, returnType); } - if (!isNode(node["handler"]) || !isNode(node["handler"]["body"])) return; - const catchBody = Array.isArray(node["handler"]["body"]["body"]) - ? node["handler"]["body"]["body"].filter(isNode) - : []; - - for (const statement of catchBody) { - if ( - statement.type === "ReturnStatement" && - isNode(statement["argument"]) - ) { - wrapReturnValue(statement, isAsync, returnType); - } - } + const handler = node["handler"]; + if (!isNode(handler) || !isNode(handler["body"])) return; - const hasCorrectReturn = catchBody.some( - (statement) => - statement.type === "ReturnStatement" && - isNode(statement["argument"]) && - ((statement["argument"].type === "Identifier" && - statement["argument"]["name"] === "FAILED_PROMISE") || - isResultHelperCall(statement["argument"], "err") || - hasIsErrorFlag(statement["argument"], true)), - ); + // The catch branch is the failure path, and it settles on `err()` + // — nothing else. A default belongs in the try block, where the + // absence is read as a value instead of being recovered from a + // throw. So a catch return is never offered `ok(...)`; feeding both + // suggestions to the same statement used to produce two reports + // with opposite fixes. + const catchReturns = collectOwnReturns(handler["body"]); - if (!hasCorrectReturn) { - context.report({ - node: node["handler"], - messageId: "returnFailedPromise", - fix(fixer) { - const lastStatement = catchBody.at(-1); - if ( - lastStatement?.type === "ReturnStatement" && - isNode(lastStatement["argument"]) && - lastStatement["argument"].type === "ObjectExpression" - ) { - return [ - ...ensureResultHelperImportFixes( - node, - fixer, - "err", - ), - fixer.replaceText( - lastStatement, - "return err()", - ), - ]; - } + for (const statement of catchReturns) { + reportCatchReturn(node, statement); + } - const handler = node["handler"]; - if (!isNode(handler)) { - return null; - } + // A nested try/catch inside this catch settles the branch on its + // own, so its returns count here even though `collectOwnReturns` + // hands them to the inner `processTryCatch`. Appending another + // `return err()` after them would be unreachable. + let settlesItself = false; + walkOwnSubtree(handler["body"], (candidate) => { + if (candidate.type === "ReturnStatement") { + settlesItself = true; + } + }); - const bodyRange = handler["body"]; - if ( - !isNode(bodyRange) || - !Array.isArray(bodyRange["range"]) - ) { - return null; - } + if (settlesItself) return; - return [ - ...ensureResultHelperImportFixes( - node, - fixer, - "err", - ), - fixer.insertTextBeforeRange( - [ - bodyRange["range"][1] - 1, - bodyRange["range"][1] - 1, - ], - "return err(); ", - ), - ]; - }, - }); - } + context.report({ + node: handler, + messageId: "returnFailedPromise", + fix(fixer) { + const handlerBody = handler["body"]; + if ( + !isNode(handlerBody) || + !Array.isArray(handlerBody["range"]) + ) { + return null; + } + + return [ + ...ensureResultHelperImportFixes(node, fixer, "err"), + fixer.insertTextBeforeRange( + [ + handlerBody["range"][1] - 1, + handlerBody["range"][1] - 1, + ], + // The newline matters: `catch { log() }` would + // otherwise become `catch { log() return err(); }`, + // which does not parse. + "\nreturn err();\n", + ), + ]; + }, + }); } return { FunctionDeclaration: checkReturnType, FunctionExpression: checkReturnType, ArrowFunctionExpression: checkReturnType, - TSDeclareFunction: checkReturnType, - TSFunctionType: checkReturnType, - TSMethodSignature: checkReturnType, + TSDeclareFunction: checkSignatureReturnType, + TSFunctionType: checkSignatureReturnType, + TSMethodSignature: checkSignatureReturnType, TryStatement: processTryCatch, }; }, diff --git a/packages/typebuddy/rules/optional_rule.ts b/packages/typebuddy/rules/optional_rule.ts index 5950aef..f43dca1 100644 --- a/packages/typebuddy/rules/optional_rule.ts +++ b/packages/typebuddy/rules/optional_rule.ts @@ -17,8 +17,12 @@ const rule = { const undefinedType = node.types.find( (typeNode) => typeNode.type === "TSUndefinedKeyword", ); + // `null | undefined` has no non-nullish member to name, and rewriting + // it to `Optional` only restated it with more syntax. const otherType = node.types.find( - (typeNode) => typeNode.type !== "TSUndefinedKeyword", + (typeNode) => + typeNode.type !== "TSUndefinedKeyword" && + typeNode.type !== "TSNullKeyword", ); if (undefinedType && otherType) { diff --git a/packages/typebuddy/rules/own_subtree.ts b/packages/typebuddy/rules/own_subtree.ts new file mode 100644 index 0000000..332f6ef --- /dev/null +++ b/packages/typebuddy/rules/own_subtree.ts @@ -0,0 +1,70 @@ +type AstNode = { + type: string; + [key: string]: unknown; +}; + +const FUNCTION_TYPES = new Set([ + "ArrowFunctionExpression", + "FunctionDeclaration", + "FunctionExpression", +]); + +function isNode(value: unknown): value is AstNode { + return typeof value === "object" && value !== null && "type" in value; +} + +/** + * Visit every node that belongs to `root`, stopping at nested functions. + * + * Both async rules need the same notion of "this function's own code". A + * `throw` or a `return` inside a nested function belongs to that function, not + * to the one being linted, so the walk never descends into one — and `parent` + * points back up the tree, so following it would not terminate. + * + * `visit` may return `false` to keep the walk out of a node's children. That is + * how a rule declines a subtree that some other visitor already owns. + * + * @param {unknown} root - The node to start from. Visited itself, and never + * skipped for being a function. + * @param {(node: AstNode) => boolean | void} visit - Called once per node. + * Return `false` to skip that node's children. + * @returns {void} + */ +function walkOwnSubtree( + root: unknown, + visit: (node: AstNode) => boolean | void, +): void { + if (!isNode(root)) { + return; + } + + const pending: AstNode[] = [root]; + while (pending.length > 0) { + // `pop` on a non-empty array always yields a node; the length check above + // is the loop condition, but `noUncheckedIndexedAccess` cannot see that. + const current = pending.pop(); + if (current === undefined) { + break; + } + + if (visit(current) === false) { + continue; + } + + for (const [key, value] of Object.entries(current)) { + if (key === "parent") { + continue; + } + + const candidates = Array.isArray(value) ? value : [value]; + for (const candidate of candidates) { + if (!isNode(candidate) || FUNCTION_TYPES.has(candidate.type)) { + continue; + } + pending.push(candidate); + } + } + } +} + +export { walkOwnSubtree }; diff --git a/packages/typebuddy/rules/typebuddy_import.ts b/packages/typebuddy/rules/typebuddy_import.ts new file mode 100644 index 0000000..a8bfbaf --- /dev/null +++ b/packages/typebuddy/rules/typebuddy_import.ts @@ -0,0 +1,134 @@ +type AstNode = { + type: string; + parent?: AstNode; + [key: string]: unknown; +}; + +const PACKAGE_NAME = "@murky-web/typebuddy"; + +function isNode(value: unknown): value is AstNode { + return typeof value === "object" && value !== null && "type" in value; +} + +function isIdentifierNamed(node: unknown, name: string): boolean { + return isNode(node) && node.type === "Identifier" && node["name"] === name; +} + +function isImportDeclaration(node: unknown): node is AstNode { + return isNode(node) && node.type === "ImportDeclaration"; +} + +function getStringLiteralValue(node: unknown): string | null { + if (!isNode(node)) { + return null; + } + + if ( + (node.type === "Literal" || node.type === "StringLiteral") && + typeof node["value"] === "string" + ) { + return node["value"]; + } + + return null; +} + +function getProgram(node: unknown): AstNode | null { + if (!isNode(node)) { + return null; + } + + let current: AstNode | undefined = node; + while (current?.parent) { + current = current.parent; + } + + return current?.type === "Program" ? current : null; +} + +function getProgramBody(program: AstNode): AstNode[] { + if (!Array.isArray(program["body"])) { + return []; + } + + return program["body"].filter(isNode); +} + +function importsHelper(program: AstNode, helperName: string): boolean { + return getProgramBody(program).some((statement) => { + if (!isImportDeclaration(statement)) { + return false; + } + + if (getStringLiteralValue(statement["source"]) !== PACKAGE_NAME) { + return false; + } + + // A type-only import brings no runtime binding, so it does not satisfy a + // fix that is about to call the helper. + if (statement["importKind"] === "type") { + return false; + } + + const specifiers = Array.isArray(statement["specifiers"]) + ? statement["specifiers"] + : []; + + return specifiers.some((specifier) => { + return ( + isNode(specifier) && + specifier.type === "ImportSpecifier" && + isIdentifierNamed(specifier["local"], helperName) + ); + }); + }); +} + +/** + * Where and what to insert so `helperName` is imported from the package. + * + * Both async rules add `ok`/`err` imports, and both used to carry their own + * copy of this — including the same bug. Whether the import lands after an + * existing one was inferred from the anchor's offset (`range[0] !== 0`), which + * is only the same question in a file whose first statement starts at byte + * zero. A leading comment moved the first statement off zero, the wrong branch + * won, and the fix emitted a leading newline instead of a trailing one: + * + * import { err } from "@murky-web/typebuddy";export async function load() + * + * The list of imports answers it directly, so that is what decides now. + * + * @param {unknown} node - Any node in the file; the program is found from it. + * @param {string} helperName - The named export to import, e.g. `"err"`. + * @returns {{ range: [number, number]; text: string } | null} The zero-width + * insertion to apply, or null when the helper is already imported or no + * anchor carries a usable range. + */ +function getTypeBuddyImportInsertion( + node: unknown, + helperName: string, +): { range: [number, number]; text: string } | null { + const program = getProgram(node); + if (!program || importsHelper(program, helperName)) { + return null; + } + + const body = getProgramBody(program); + const imports = body.filter(isImportDeclaration); + const anchor = imports.at(-1) ?? body[0] ?? program; + const range = anchor["range"]; + if (!Array.isArray(range) || range.length < 2) { + return null; + } + + const statement = `import { ${helperName} } from "${PACKAGE_NAME}";`; + if (imports.length > 0) { + const end: number = range[1]; + return { range: [end, end], text: `\n${statement}` }; + } + + const start: number = range[0]; + return { range: [start, start], text: `${statement}\n` }; +} + +export { getTypeBuddyImportInsertion }; diff --git a/packages/typebuddy/smoke/oxlint/fix-expected-bare.ts b/packages/typebuddy/smoke/oxlint/fix-expected-bare.ts new file mode 100644 index 0000000..7f96219 --- /dev/null +++ b/packages/typebuddy/smoke/oxlint/fix-expected-bare.ts @@ -0,0 +1,15 @@ +// A leading comment and no imports at all. +// +// The insertion point used to be classified by the anchor's byte offset, so a +// comment that pushed the first statement off zero flipped the branch and the +// import was emitted with a leading newline instead of a trailing one — landing +// glued to the statement it was inserted before. +import { ok } from "@murky-web/typebuddy"; +import { err } from "@murky-web/typebuddy"; +export async function needsHelpers(): AsyncResult { + try { + return ok("value"); + } catch { + return err(); + } +} diff --git a/packages/typebuddy/smoke/oxlint/fix-expected.ts b/packages/typebuddy/smoke/oxlint/fix-expected.ts index a5afe84..436b3af 100644 --- a/packages/typebuddy/smoke/oxlint/fix-expected.ts +++ b/packages/typebuddy/smoke/oxlint/fix-expected.ts @@ -46,3 +46,147 @@ export async function wrapMe(): MaybePromise { return err(); } } + +declare function log(message: string): void; + +// A catch whose last statement has no semicolon: the inserted `return err()` +// used to be glued onto it, producing code that does not parse. +export async function oneLineCatch(): MaybePromise { + try { + return ok("one-line"); + } catch { + log("boom"); + return err(); + } +} + +// The wrapping fix used to strip leading whitespace on every line, which +// silently rewrote the contents of template literals. +export async function keepsTemplate(): MaybePromise { + try { + return ok(`multi + line template`); + } catch { + return err(); + } +} + +// An expression-bodied arrow has no block for the try/catch to live in. +export const arrowBody = async () => { + try { + return ok(await Promise.resolve("arrow")); + } catch { + return err(); + } +}; + +// A parenthesised expression body: the parentheses sit outside the body node. +export const parenthesisedArrowBody = async () => { + try { + return ok({ wrapped: true }); + } catch { + return err(); + } +}; + +// Type-level signatures can never be `async`, so the async gate used to make +// these three visitors unreachable. +export interface Loader { + load(): MaybePromise; +} + +export type LoadFn = () => MaybePromise; + +// Already migrated by hand. The awaited-type lookup used to match only +// `Promise`, so this bare `return` stayed unwrapped while its `Promise` +// twin two functions up was fixed. +export async function alreadyMigrated(): MaybePromise { + try { + return ok(); + } catch { + return err(); + } +} + +// `null | undefined` names no value to put in the brackets; it used to be +// rewritten to the equivalent-but-longer `Optional`. +export type NeitherCandidate = null | undefined; + +// `AsyncResult` is the preferred spelling of `MaybePromise`, so the awaited-type +// lookup has to know it too — otherwise the rule goes blind on exactly the +// annotation the package tells people to write. +export async function preferredSpelling(): AsyncResult { + try { + return ok(); + } catch { + return err(); + } +} + +// A value handed back from the failure path. The rule reports it — the default +// belongs in the try block — but must not rewrite it: replacing it with `err()` +// would drop the fallback, and appending one produced unreachable code. +export async function fallbackInCatch(): AsyncResult { + try { + return ok("real"); + } catch { + return "fallback"; + } +} + +// Same for a fallback already spelled as a result: reported, never rewritten. +export async function okInCatch(): AsyncResult { + try { + return ok("real"); + } catch { + return ok("defaults"); + } +} + +// A bare `return` in a catch loses nothing by becoming `err()`, so it is fixed. +export async function bareCatchReturn(): AsyncResult { + try { + return ok(); + } catch { + return err(); + } +} + +// Returns the try block owns, but does not hold directly. Only the last one +// used to be wrapped, because the walk never looked inside the `if`, the loop +// or the `switch`. The callback's return belongs to the callback, not here. +export async function nestedReturns( + flag: boolean, + n: number, +): AsyncResult { + try { + if (flag) return ok("early"); + for (const x of [1]) { + if (x) return ok("loop"); + } + switch (n) { + case 1: + return ok("switch"); + } + const cb = () => { + return "belongs to the callback"; + }; + return ok(cb()); + } catch { + return err(); + } +} + +// The inner try/catch settles this catch branch on its own, so no `return err()` +// may be appended after it — that would be unreachable. +export async function settledByNestedTry(): AsyncResult { + try { + return ok("real"); + } catch { + try { + return ok("retry"); + } catch { + return err(); + } + } +} diff --git a/packages/typebuddy/smoke/oxlint/fix-input-bare.ts b/packages/typebuddy/smoke/oxlint/fix-input-bare.ts new file mode 100644 index 0000000..15881dd --- /dev/null +++ b/packages/typebuddy/smoke/oxlint/fix-input-bare.ts @@ -0,0 +1,11 @@ +// A leading comment and no imports at all. +// +// The insertion point used to be classified by the anchor's byte offset, so a +// comment that pushed the first statement off zero flipped the branch and the +// import was emitted with a leading newline instead of a trailing one — landing +// glued to the statement it was inserted before. +export async function needsHelpers(): AsyncResult { + try { + return "value"; + } catch {} +} diff --git a/packages/typebuddy/smoke/oxlint/fix-input.ts b/packages/typebuddy/smoke/oxlint/fix-input.ts index 4b916c8..b7c13b6 100644 --- a/packages/typebuddy/smoke/oxlint/fix-input.ts +++ b/packages/typebuddy/smoke/oxlint/fix-input.ts @@ -33,3 +33,121 @@ export async function fromObject(): MaybePromise { export async function wrapMe(): Promise { return "wrapped"; } + +declare function log(message: string): void; + +// A catch whose last statement has no semicolon: the inserted `return err()` +// used to be glued onto it, producing code that does not parse. +export async function oneLineCatch(): Promise { + try { + return "one-line"; + } catch { log("boom") } +} + +// The wrapping fix used to strip leading whitespace on every line, which +// silently rewrote the contents of template literals. +export async function keepsTemplate(): Promise { + return `multi + line template`; +} + +// An expression-bodied arrow has no block for the try/catch to live in. +export const arrowBody = async () => await Promise.resolve("arrow"); + +// A parenthesised expression body: the parentheses sit outside the body node. +export const parenthesisedArrowBody = async () => ({ wrapped: true }); + +// Type-level signatures can never be `async`, so the async gate used to make +// these three visitors unreachable. +export interface Loader { + load(): Promise; +} + +export type LoadFn = () => Promise; + +// Already migrated by hand. The awaited-type lookup used to match only +// `Promise`, so this bare `return` stayed unwrapped while its `Promise` +// twin two functions up was fixed. +export async function alreadyMigrated(): MaybePromise { + try { + return; + } catch {} +} + +// `null | undefined` names no value to put in the brackets; it used to be +// rewritten to the equivalent-but-longer `Optional`. +export type NeitherCandidate = null | undefined; + +// `AsyncResult` is the preferred spelling of `MaybePromise`, so the awaited-type +// lookup has to know it too — otherwise the rule goes blind on exactly the +// annotation the package tells people to write. +export async function preferredSpelling(): AsyncResult { + try { + return; + } catch {} +} + +// A value handed back from the failure path. The rule reports it — the default +// belongs in the try block — but must not rewrite it: replacing it with `err()` +// would drop the fallback, and appending one produced unreachable code. +export async function fallbackInCatch(): AsyncResult { + try { + return ok("real"); + } catch { + return "fallback"; + } +} + +// Same for a fallback already spelled as a result: reported, never rewritten. +export async function okInCatch(): AsyncResult { + try { + return ok("real"); + } catch { + return ok("defaults"); + } +} + +// A bare `return` in a catch loses nothing by becoming `err()`, so it is fixed. +export async function bareCatchReturn(): AsyncResult { + try { + return ok(); + } catch { + return; + } +} + +// Returns the try block owns, but does not hold directly. Only the last one +// used to be wrapped, because the walk never looked inside the `if`, the loop +// or the `switch`. The callback's return belongs to the callback, not here. +export async function nestedReturns(flag: boolean, n: number): AsyncResult { + try { + if (flag) return "early"; + for (const x of [1]) { + if (x) return "loop"; + } + switch (n) { + case 1: + return "switch"; + } + const cb = () => { + return "belongs to the callback"; + }; + return cb(); + } catch { + return err(); + } +} + +// The inner try/catch settles this catch branch on its own, so no `return err()` +// may be appended after it — that would be unreachable. +export async function settledByNestedTry(): AsyncResult { + try { + return ok("real"); + } catch { + try { + return ok("retry"); + } catch { + return err(); + } + } +} diff --git a/packages/typebuddy/smoke/oxlint/run-fix-smoke.ts b/packages/typebuddy/smoke/oxlint/run-fix-smoke.ts index 15d3abf..bd9358d 100644 --- a/packages/typebuddy/smoke/oxlint/run-fix-smoke.ts +++ b/packages/typebuddy/smoke/oxlint/run-fix-smoke.ts @@ -3,61 +3,78 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; const smokeDir = new URL("./", import.meta.url); -const sourcePath = new URL("./fix-input.ts", smokeDir); -const expectedPath = new URL("./fix-expected.ts", smokeDir); const configPath = new URL("./.oxlintrc.jsonc", smokeDir); const formatConfigPath = new URL( "../../../config/oxc/.oxfmtrc.jsonc", smokeDir, ); -const tempDir = await mkdtemp(join(tmpdir(), "typebuddy-oxlint-fix-")); -const tempFilePath = join(tempDir, "fix-input.ts"); +// Two shapes, because the import insertion behaves differently in each: a file +// that already imports something gets the helper appended after the last +// import, one that does not gets it prepended before the first statement. +const cases = [ + { input: "./fix-input.ts", expected: "./fix-expected.ts" }, + { input: "./fix-input-bare.ts", expected: "./fix-expected-bare.ts" }, +] as const; -try { - const input = await readFile(sourcePath, "utf8"); - await writeFile(tempFilePath, input, "utf8"); +async function runCase(input: string, expected: string): Promise { + const sourcePath = new URL(input, smokeDir); + const expectedPath = new URL(expected, smokeDir); - let previous = input; + const tempDir = await mkdtemp(join(tmpdir(), "typebuddy-oxlint-fix-")); + const tempFilePath = join(tempDir, "fixture.ts"); - for (let pass = 0; pass < 6; pass += 1) { - const result = - await Bun.$`oxlint -c ${configPath.pathname} --fix ${tempFilePath}` - .quiet() - .nothrow(); + try { + const source = await readFile(sourcePath, "utf8"); + await writeFile(tempFilePath, source, "utf8"); - if (![0, 1].includes(result.exitCode)) { - console.error("Oxlint fix smoke test failed unexpectedly."); - process.exit(result.exitCode); - } + let previous = source; + + for (let pass = 0; pass < 6; pass += 1) { + const result = + await Bun.$`oxlint -c ${configPath.pathname} --fix ${tempFilePath}` + .quiet() + .nothrow(); + + if (![0, 1].includes(result.exitCode)) { + console.error( + `Oxlint fix smoke test failed unexpectedly on ${input}.`, + ); + process.exit(result.exitCode); + } - const next = await readFile(tempFilePath, "utf8"); - if (next === previous) { - break; + const next = await readFile(tempFilePath, "utf8"); + if (next === previous) { + break; + } + + previous = next; } - previous = next; - } + await Bun.$`oxfmt -c ${formatConfigPath.pathname} ${tempFilePath}`.quiet(); - await Bun.$`oxfmt -c ${formatConfigPath.pathname} ${tempFilePath}`.quiet(); - - const [actual, expected] = await Promise.all([ - readFile(tempFilePath, "utf8"), - readFile(expectedPath, "utf8"), - ]); - - if (actual !== expected) { - console.error( - "Oxlint fix smoke test did not produce the expected output.", - ); - console.error("\n--- Expected ---\n"); - console.error(expected); - console.error("\n--- Actual ---\n"); - console.error(actual); - process.exit(1); + const [actual, want] = await Promise.all([ + readFile(tempFilePath, "utf8"), + readFile(expectedPath, "utf8"), + ]); + + if (actual !== want) { + console.error( + `Oxlint fix smoke test did not produce the expected output for ${input}.`, + ); + console.error("\n--- Expected ---\n"); + console.error(want); + console.error("\n--- Actual ---\n"); + console.error(actual); + process.exit(1); + } + } finally { + await rm(tempDir, { recursive: true, force: true }); } +} - console.log("Oxlint fix smoke test passed."); -} finally { - await rm(tempDir, { recursive: true, force: true }); +for (const { input, expected } of cases) { + await runCase(input, expected); } + +console.log(`Oxlint fix smoke test passed (${cases.length} fixtures).`);