diff --git a/.changeset/lint-checked-parse-findings.md b/.changeset/lint-checked-parse-findings.md new file mode 100644 index 0000000000..f3d47733a7 --- /dev/null +++ b/.changeset/lint-checked-parse-findings.md @@ -0,0 +1,35 @@ +--- +"@objectstack/lint": minor +--- + +Report an unparseable source instead of scoring it CLEAN (#10653). + +Four validators parsed authored source with `ts.createSourceFile` and never read +`parseDiagnostics`. That call **cannot throw**, so a source with syntax errors +came back as a tree built by error recovery, got walked like any other, and +produced no findings — a source the validator could not read, reported as a +source with nothing to report. Two of the sites carried a `try/catch` around the +parse that never once ran. + +Each now reports what it could not read, as a finding the author receives rather +than as an exit — a publish-time validator is handed metadata by someone else, +so ending the process on their input is not its call. Four new advisory +(`warning`) rule ids, all additive: every finding these rules produce today they +still produce, including from a partially recovered tree. + +- `react-page-source-unparseable` — `kind:'react'` page source + (`validateReactPageProps`) +- `startup-source-unparseable` — plugin source (`findStartupRegistryVerdicts`) +- `hook-body-source-unparseable` — L2 hook body (`validateHookBodyWrites`) +- `action-body-source-unparseable` — L2 action body (`validateActionBodyWrites`) + +New exports: the four rule-id constants, plus `describeParseFailure`, +`PARSE_FAILURE_HINT` and the `SourceParseFailure` / `CheckedParse` / +`CheckedParseOptions` types. `ExtractedHookBodyWriteSet` gains an optional +`parseFailure`, so a consumer of the extractor can tell "wrote nothing" from +"could not be read" — the distinction that was missing. + +Nothing is removed or renamed, and no source that parses gains a finding. A +stack whose authored sources all parse lints exactly as before; one carrying a +source with a syntax error gains a warning that names the file, line and column +instead of silently skipping the checks. diff --git a/packages/lint/src/checked-parse.test.ts b/packages/lint/src/checked-parse.test.ts new file mode 100644 index 0000000000..6c30a84c82 --- /dev/null +++ b/packages/lint/src/checked-parse.test.ts @@ -0,0 +1,132 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #10653 — the premise, pinned. Every claim in `checked-parse.ts`'s header is a +// measurement, and a measurement that lives only in a comment is one nobody +// re-runs. The two that matter: +// +// 1. `ts.createSourceFile` does not throw on a wreck. That is what makes the +// `try/catch` this module replaced dead code, and what makes the unread +// `parseDiagnostics` the LIVE path. If a future TypeScript ever starts +// throwing, `cannot throw` below fails and the callers' catch-free parses +// need revisiting — loudly, rather than by a caller crashing in the field. +// 2. The diagnostics are REACHED. `parseDiagnostics` is internal to the +// compiler and typed optional here, so a rename would make every checked +// parse silently return "no failure" — the exact green-line-that-lies this +// module exists to remove, wearing its own badge. `the property is really +// there` is the non-vacuity proof (#4690: a check that has only ever been +// green must be tellable apart from a dead one). +// +// The test imports `typescript` directly, which the RULES may not do — they take +// the compiler as a parameter and load it lazily (`lazy-deps.test.ts` guards +// that). A test file is not on the kernel boot path. +import ts from 'typescript'; +import { describe, it, expect } from 'vitest'; + +import { createSourceFileChecked, describeParseFailure } from './checked-parse.js'; + +/** Wrecks, and the ScriptKind each is measured under. */ +const WRECKS: ReadonlyArray<{ label: string; source: string; kind: ts.ScriptKind }> = [ + { + label: 'merge-conflict markers', + source: '<<<<<<< HEAD\nconst a = 1;\n=======\nconst a = 2;\n>>>>>>> other\n', + kind: ts.ScriptKind.TS, + }, + { label: 'truncated function body', source: 'export function f() {\n const x = 1;\n', kind: ts.ScriptKind.TS }, + { label: 'unterminated string', source: "const s = 'oops;\n", kind: ts.ScriptKind.TS }, + { label: 'unterminated block comment', source: 'function f() {\n /* TODO\n}\n', kind: ts.ScriptKind.TS }, + { label: 'unterminated template literal', source: 'const s = `oops;\n', kind: ts.ScriptKind.TS }, + { label: 'a JSX element read as TS', source: 'const a =
hi
;\n', kind: ts.ScriptKind.TS }, +]; + +const parse = (source: string, kind: ts.ScriptKind, synthesizedLinesBefore?: number) => + createSourceFileChecked(ts, 'probe.tsx', source, { + target: ts.ScriptTarget.Latest, + setParentNodes: true, + scriptKind: kind, + ...(synthesizedLinesBefore === undefined ? {} : { synthesizedLinesBefore }), + }); + +describe('createSourceFileChecked — the premise (#10653)', () => { + it.each(WRECKS)('cannot throw: $label', ({ source, kind }) => { + // The claim is about `createSourceFile` itself, so it is called RAW here — + // going through the helper would prove only that the helper does not throw. + expect(() => ts.createSourceFile('probe.tsx', source, ts.ScriptTarget.Latest, true, kind)).not.toThrow(); + }); + + it.each(WRECKS)('reports a failure instead of a clean tree: $label', ({ source, kind }) => { + const { failure } = parse(source, kind); + expect(failure, 'a source that does not parse must come back with a failure').toBeDefined(); + expect(failure!.count).toBeGreaterThan(0); + expect(failure!.message.length).toBeGreaterThan(0); + expect(failure!.line).toBeGreaterThanOrEqual(1); + expect(failure!.column).toBeGreaterThanOrEqual(1); + }); + + it('the property is really there — non-vacuity of the whole module (#4690)', () => { + // If `parseDiagnostics` is ever renamed or dropped, the optional read in + // checked-parse.ts yields undefined and EVERY checked parse in this package + // goes quietly back to scoring wreckage clean. Nothing else would fail. So + // the property is asserted directly, on the compiler, once. + const sf = ts.createSourceFile('probe.ts', 'const a = ;\n', ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const diagnostics = (sf as unknown as { parseDiagnostics?: readonly ts.Diagnostic[] }).parseDiagnostics; + expect(Array.isArray(diagnostics), '`SourceFile.parseDiagnostics` is gone — checked-parse.ts is now inert').toBe( + true, + ); + expect(diagnostics!.length).toBeGreaterThan(0); + }); +}); + +describe('createSourceFileChecked — a source that DOES parse', () => { + it.each([ + ['plain TS', 'export const a: number = 1;\n', ts.ScriptKind.TS], + ['TSX with JSX', 'export default function P() {\n return ;\n}\n', ts.ScriptKind.TSX], + ['empty', '', ts.ScriptKind.TS], + ] as const)('reports no failure: %s', (_label, source, kind) => { + expect(parse(source, kind).failure).toBeUndefined(); + }); + + it('the SAME source under the wrong ScriptKind is a failure, not an opinion', () => { + // The ScriptKind hat: `
` is a JSX element in TSX and a wreck in + // TS. Both verdicts are correct for their kind, and neither is silent. + const jsx = 'const a =
hi
;\n'; + expect(parse(jsx, ts.ScriptKind.TSX).failure).toBeUndefined(); + expect(parse(jsx, ts.ScriptKind.TS).failure).toBeDefined(); + }); +}); + +describe('createSourceFileChecked — position reporting', () => { + it('reports the line the error is on', () => { + const { failure } = parse('const a = 1;\nconst b = 2;\nconst c = ;\n', ts.ScriptKind.TS); + expect(failure!.line).toBe(3); + }); + + it('subtracts the caller-synthesised lines, so a wrapper never shifts the blame', () => { + // What validate-hook-body-writes.ts does: the author wrote `const c = ;` on + // line 2 of their BODY, and the wrapper puts it on line 3 of what is parsed. + const body = 'const a = 1;\nconst c = ;\n'; + const wrapped = `async function __body(ctx) {\n${body}\n}`; + expect(parse(wrapped, ts.ScriptKind.TS).failure!.line, 'raw: the wrapper line counts').toBe(3); + expect(parse(wrapped, ts.ScriptKind.TS, 1).failure!.line, "remapped: the author's own line").toBe(2); + }); + + it('clamps to line 1 — a diagnostic on a synthesised line never points above the source', () => { + // A diagnostic that lands ON the wrapper's own first line would remap to 0, + // which is not a line anybody wrote. + const failure = parse('const a = ;\n', ts.ScriptKind.TS, 5).failure!; + expect(failure.line).toBe(1); + }); + + it('counts every diagnostic, and names the first', () => { + const failure = parse('<<<<<<< HEAD\nconst a = 1;\n=======\nconst a = 2;\n>>>>>>> other\n', ts.ScriptKind.TS) + .failure!; + expect(failure.count).toBeGreaterThan(1); + expect(describeParseFailure(failure)).toContain(`${failure.count} syntax errors in total`); + expect(describeParseFailure(failure)).toContain(failure.message); + }); + + it('a single diagnostic is described without a count', () => { + const failure = parse('const a = ;\n', ts.ScriptKind.TS).failure!; + expect(failure.count).toBe(1); + expect(describeParseFailure(failure)).not.toContain('in total'); + }); +}); diff --git a/packages/lint/src/checked-parse.ts b/packages/lint/src/checked-parse.ts new file mode 100644 index 0000000000..43c2fbd03b --- /dev/null +++ b/packages/lint/src/checked-parse.ts @@ -0,0 +1,173 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// `@objectstack/lint`'s ONE answer to "did this source actually parse?" (#10653). +// +// ## The defect this closes — and it is NOT the `try/catch` it replaced +// +// **`ts.createSourceFile` never throws.** Hand it merge-conflict markers, an +// unterminated comment or a truncated body and it returns a `SourceFile` built +// by error recovery, indistinguishable from any other: the errors are parked on +// `parseDiagnostics`, which nothing in this package read. A validator then walks +// that wreckage, finds none of the shapes it is looking for, and returns an +// empty finding list — **a source the validator could not read, reported as a +// source with nothing to report.** +// +// The three call sites this module was extracted from each carried a +// `try/catch` around the parse, and two of them read as if the catch were the +// guard. It never ran. Measured here on 2026-08-21 against TypeScript 6.0.3 — +// every one of these returns a tree, throws nothing, and is walked as if clean: +// +// source ScriptKind.TS ScriptKind.TSX threw? +// --------------------------- ------------- -------------- ------ +// merge-conflict markers 3 diagnostics 3 diagnostics no +// truncated function body 1 diagnostic 1 diagnostic no +// unterminated string 1 diagnostic 1 diagnostic no +// unterminated block comment 1 diagnostic 1 diagnostic no +// a JSX element 1 diagnostic 0 no +// +// The last row is the same defect wearing the `ScriptKind` hat, and it is why +// the answer lives in one module rather than three: the question "did this +// parse?" has one right answer and three places that were getting it wrong in +// slightly different ways. `checked-parse.test.ts` pins the measurement so the +// premise above cannot rot into a comment nobody re-ran — including the row +// that matters most, that the diagnostics are REACHED (see the non-vacuity +// case there, and `parseDiagnostics` below). +// +// ## Why a returned FINDING rather than a refusal +// +// `scripts/ts-parse.mjs` answers the same question for repo tooling and answers +// it by REFUSING — correct there, because a `scripts/**` gate audits a tree its +// own author controls. A publish-time validator is in the opposite position: it +// is handed metadata by someone else, and ending the process on their input is +// not its call. So this module reports, and every caller turns the report into +// a finding the author receives at authoring time. Nothing here throws, and +// nothing here decides severity — that belongs to the rule. +// +// ## Why lint-local, and not the two neighbours it could have been +// +// • NOT `scripts/ts-parse.mjs`: `@objectstack/lint` is PUBLISHED and packs +// `dist, README.md, CHANGELOG.md`. A published package that asks repo +// tooling whether something parsed trades this bug for a worse one — the +// objection `invoked-as.mjs` already argued for its own `packages/cli` +// sibling, and the reason #10606 existed at all. +// • NOT a new shared `@objectstack/…` package: a published dependency built +// for four call sites, argued down in the same place. +// +// The TypeScript module is a PARAMETER, never an import: `@objectstack/lint` +// sits on the kernel boot path and `typescript` is ~9 MB of CJS, so every +// caller here loads it lazily and only when a source actually needs parsing. +// This module must therefore stay type-only in its imports (`lazy-deps.test.ts` +// is the guard). +import type ts from 'typescript'; + +/** + * `parseDiagnostics` is where the parser parks syntax errors, and it is not on + * the public `SourceFile` type — the property is internal to the compiler and + * reached here by a narrowing cast rather than by `as any`. + * + * Declaring it OPTIONAL is deliberate: if a future TypeScript renames or drops + * it, this reads `undefined` and every checked parse silently returns to the + * behaviour this module exists to remove — a green line about a source nobody + * could read. That failure mode is invisible by construction, so it is pinned + * in `checked-parse.test.ts` instead of being trusted here. + */ +interface SourceFileWithParseDiagnostics { + readonly parseDiagnostics?: readonly ts.Diagnostic[]; +} + +/** What a caller needs to tell an author WHICH source went unread, and where. */ +export interface SourceParseFailure { + /** The first parse diagnostic, in the compiler's own wording, flattened to one line. */ + message: string; + /** 1-based line, in the AUTHORED source's coordinates (see `synthesizedLinesBefore`). */ + line: number; + /** 1-based column. */ + column: number; + /** How many parse diagnostics in total — `message` is the first of `count`. */ + count: number; +} + +/** A parse plus the verdict on whether it succeeded. `failure` absent ⇒ it parsed. */ +export interface CheckedParse { + /** + * The tree, ALWAYS returned — including when `failure` is set. Error recovery + * produces a partial tree, and a caller that already reports findings from it + * keeps doing so: the fix here is the missing SIGNAL, not the removal of + * whatever the recovered tree could still be read for. + */ + sourceFile: ts.SourceFile; + /** Set when the parser reported at least one syntax diagnostic. */ + failure?: SourceParseFailure; +} + +export interface CheckedParseOptions { + target: ts.ScriptTarget; + setParentNodes: boolean; + scriptKind: ts.ScriptKind; + /** + * Lines the CALLER synthesised ahead of the authored source, subtracted from + * the reported position so it lands in the author's coordinates. + * + * `validate-hook-body-writes.ts` parses an L2 hook body wrapped in + * `async function __body(ctx) {\n…\n}` — the shape the runtime compiles it + * into — so its diagnostics are one line low. The reported line is clamped to + * at least 1, so a diagnostic that lands on a synthesised line is attributed + * to the nearest AUTHORED line and never to a line the author did not write. + */ + synthesizedLinesBefore?: number; +} + +/** + * `ts.createSourceFile`, with the diagnostics READ. + * + * Never throws and never exits: the verdict comes back as data. + */ +export function createSourceFileChecked( + tsc: typeof ts, + fileName: string, + source: string, + options: CheckedParseOptions, +): CheckedParse { + const sourceFile = tsc.createSourceFile( + fileName, + source, + options.target, + options.setParentNodes, + options.scriptKind, + ); + const diagnostics = (sourceFile as unknown as SourceFileWithParseDiagnostics).parseDiagnostics; + if (!diagnostics || diagnostics.length === 0) return { sourceFile }; + + const first = diagnostics[0]!; + const { line, character } = sourceFile.getLineAndCharacterOfPosition(first.start ?? 0); + const offset = options.synthesizedLinesBefore ?? 0; + return { + sourceFile, + failure: { + message: tsc.flattenDiagnosticMessageText(first.messageText, ' '), + line: Math.max(1, line + 1 - offset), + column: character + 1, + count: diagnostics.length, + }, + }; +} + +/** + * The one wording every caller's message embeds, so three findings about the + * same defect do not describe it three ways. + */ +export function describeParseFailure(failure: SourceParseFailure): string { + const more = failure.count > 1 ? `; ${failure.count} syntax errors in total` : ''; + return `line ${failure.line}, column ${failure.column}: ${failure.message}${more}`; +} + +/** + * The hint every caller's finding carries. It says what the finding IS — a + * statement about what the checker could read, not a second syntax verdict — + * because a source that does not parse is not scored, and an author who reads + * "no problems found" about it would be reading a green line that lied. + */ +export const PARSE_FAILURE_HINT = + 'The source is parsed (never executed) so it can be checked. A source with syntax errors is only ' + + 'partially recovered, so the checks that follow may have skipped real problems — fix the syntax ' + + 'error and re-run to get a verdict that covers the whole source.'; diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 10587612ef..6a8bccf7fb 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -58,6 +58,7 @@ export { STARTUP_VERDICT_HINT, STARTUP_OPEN_VOCABULARY_VERDICT, STARTUP_VERDICT_ASSERTIVE_WORDING, + STARTUP_SOURCE_UNPARSEABLE, } from './lint-startup-registry-verdict.js'; export type { StartupRegistryVerdictFinding, @@ -147,8 +148,16 @@ export { REACT_CHART_AXIS_UNKNOWN, REACT_CHART_DRILLDOWN_INVALID, REACT_BLOCK_NEEDS_RECORD_CONTEXT, + REACT_PAGE_SOURCE_UNPARSEABLE, } from './validate-react-page-props.js'; export type { ReactPropFinding, ReactPropSeverity } from './validate-react-page-props.js'; + +// [#10653] The package-local checked parse behind the three `*-source-unparseable` +// rules above and below. Exported because a consumer that receives one of those +// findings may want the failure's shape (line/column/count) rather than +// re-parsing the message string — the same reason the rule ids are exported. +export { describeParseFailure, PARSE_FAILURE_HINT } from './checked-parse.js'; +export type { SourceParseFailure, CheckedParse, CheckedParseOptions } from './checked-parse.js'; export { validatePageSourceStyling, PAGE_SOURCE_CLASSNAME } from './validate-page-source-styling.js'; export type { SourceStyleFinding, SourceStyleSeverity } from './validate-page-source-styling.js'; @@ -506,6 +515,7 @@ export { HOOK_BODY_WRITE_EXCLUSIONS, HOOK_BODY_WRITE_UNKNOWN_FIELD, HOOK_BODY_WRITE_UNPROVISIONED_ANCHOR, + HOOK_BODY_SOURCE_UNPARSEABLE, } from './validate-hook-body-writes.js'; export type { HookBodyWriteFinding, @@ -530,6 +540,7 @@ export { ACTION_BODY_WRITE_UNKNOWN_FIELD, ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR, ACTION_RECORD_WRITE_DISCARDED, + ACTION_BODY_SOURCE_UNPARSEABLE, } from './validate-action-body-writes.js'; export type { ActionBodyWriteFinding, diff --git a/packages/lint/src/lint-startup-registry-verdict.test.ts b/packages/lint/src/lint-startup-registry-verdict.test.ts index ed82f371b7..060f50169c 100644 --- a/packages/lint/src/lint-startup-registry-verdict.test.ts +++ b/packages/lint/src/lint-startup-registry-verdict.test.ts @@ -17,6 +17,7 @@ import { OPEN_VOCABULARY_PROBES, PRE_SEAL_PHASES, STARTUP_OPEN_VOCABULARY_VERDICT, + STARTUP_SOURCE_UNPARSEABLE, STARTUP_VERDICT_ASSERTIVE_WORDING, STARTUP_VERDICT_HINT, } from './lint-startup-registry-verdict.js'; @@ -402,9 +403,96 @@ describe('the vocabulary is data, and every entry earns its place', () => { expect(PRE_SEAL_PHASES.has('start')).toBe(true); }); - it('an empty or unparseable source is not a verdict about anyone', () => { + it('an empty source is not a verdict about anyone', () => { + // No parse is attempted, so nothing is claimed — including nothing about + // parseability. Unchanged by #10653. expect(findStartupRegistryVerdicts('')).toEqual([]); expect(findStartupRegistryVerdicts(' \n ')).toEqual([]); - expect(findStartupRegistryVerdicts('class { { { getRegisteredNodeTypes(')).toEqual([]); + }); + + it('a source that mentions nothing from the vocabulary is skipped WITHOUT a parse claim', () => { + // The pre-filter is a raw-text scan, so it is sound whether or not the + // source parses: a source with no probe name in it cannot contain a probe + // READ, however it parses. Skipping the parse therefore hides nothing, and + // must not produce a parse finding either. + expect(findStartupRegistryVerdicts('class { { { totally unparseable ((( ')).toEqual([]); + }); + + // [#10653] This case used to assert `toEqual([])` for an unparseable source, + // under the heading "not a verdict about anyone". Its two halves came apart on + // inspection: declining to draw a STARTUP verdict from wreckage is right and + // still holds below, but returning nothing at all said the source was clean — + // and the assertion kept passing precisely BECAUSE nothing was produced. The + // `try/catch` it was read as covering never ran; the live path was the unread + // `parseDiagnostics`. + /** + * [#10653] Sources that DO parse, for the false-positive control below. The + * three sanctioned cures plus the ordinary shapes, deliberately including the + * TS-only syntax a JS-flavoured parser would trip on — `ScriptKind.TS` is what + * the rule parses under, and a rule that started reporting "did not parse" on + * a type annotation would be worse than the silence it replaced. + * + * The wider control is the rest of this file: every case above asserts an + * exact rule-id list for a parseable source, so a spurious parse finding + * reddens them too. + */ + const PARSEABLE_CORPUS: readonly string[] = [ + 'export class P { async start(ctx) { ctx.hook("kernel:ready", () => this.engine.getRegisteredNodeTypes()); } }', + 'export class P { async init() {} }', + 'const known: string[] = this.engine.getRegisteredNodeTypes();\nexport type T = { a: number };', + 'export class P {\n private engine?: Engine;\n async start(ctx: Ctx): Promise {\n const t = this.engine!.getRegisteredNodeTypes() as string[];\n }\n}', + 'enum Phase { Init, Start }\nexport class P { async start() { const k = this.e.knownNodeTypes; } }', + 'export const listExecutors = () => [] as const;', + ]; + + describe('an unparseable source is REPORTED, not scored clean (#10653)', () => { + const wreck = 'class P {\n async start(ctx) {\n /* TODO getRegisteredNodeTypes\n }\n}\n'; + + it('returns the parse finding', () => { + const findings = findStartupRegistryVerdicts(wreck, { file: 'plugin.ts' }); + const parseFindings = findings.filter((f) => f.rule === STARTUP_SOURCE_UNPARSEABLE); + expect(parseFindings).toHaveLength(1); + expect(parseFindings[0].severity).toBe('warning'); + expect(parseFindings[0].message).toContain('did not parse'); + expect(parseFindings[0].path).toMatch(/^plugin\.ts:\d+$/); + }); + + it('still declines to draw a startup verdict from the wreckage', () => { + // The half of the original case that was right: no open-vocabulary verdict + // is invented out of a tree the parser had to guess at. + const rules = findStartupRegistryVerdicts(wreck, { file: 'plugin.ts' }).map((f) => f.rule); + expect(rules).not.toContain(STARTUP_OPEN_VOCABULARY_VERDICT); + expect(rules).not.toContain(STARTUP_VERDICT_ASSERTIVE_WORDING); + }); + + it('POSITIVE CONTROL — the same source, repaired, is the one that was being missed', () => { + // The wreck above is the repaired source with `/*` in front of the verdict. + // Repaired, the rule has plenty to say; wrecked, it used to say nothing at + // all. That gap is what the finding closes. + const repaired = + 'class P {\n' + + ' async start(ctx) {\n' + + ' const types = this.engine.getRegisteredNodeTypes();\n' + + " if (!types.includes('email')) {\n" + + " ctx.logger.warn('email will fail at execution time');\n" + + ' }\n' + + ' }\n' + + '}\n'; + const findings = findStartupRegistryVerdicts(repaired, { file: 'plugin.ts' }); + expect(findings.map((f) => f.rule)).toContain(STARTUP_OPEN_VOCABULARY_VERDICT); + expect(findings.map((f) => f.rule)).not.toContain(STARTUP_SOURCE_UNPARSEABLE); + }); + + it('FALSE-POSITIVE CONTROL — no source that parses gains a parse finding', () => { + // Every source in this file's own corpus of parseable cases, swept for the + // new rule id. A rule that fires on readable source is worse than the + // silence it replaced. + for (const source of PARSEABLE_CORPUS) { + const rules = findStartupRegistryVerdicts(source, { file: 'plugin.ts' }).map((f) => f.rule); + expect(rules, `parseable source gained a parse finding:\n${source}`).not.toContain( + STARTUP_SOURCE_UNPARSEABLE, + ); + } + }); }); }); diff --git a/packages/lint/src/lint-startup-registry-verdict.ts b/packages/lint/src/lint-startup-registry-verdict.ts index b05823c00d..4d574bf3c6 100644 --- a/packages/lint/src/lint-startup-registry-verdict.ts +++ b/packages/lint/src/lint-startup-registry-verdict.ts @@ -117,6 +117,8 @@ import { createRequire } from 'node:module'; import type ts from 'typescript'; +import { createSourceFileChecked, describeParseFailure, PARSE_FAILURE_HINT } from './checked-parse.js'; + // The TypeScript compiler must NOT be imported at module top level: it is ~9 MB // of CJS and @objectstack/lint sits on the kernel boot path, while this rule // only parses when a caller actually hands it source. Same lazy-load contract as @@ -164,6 +166,17 @@ export const STARTUP_OPEN_VOCABULARY_VERDICT = 'startup-open-vocabulary-verdict' */ export const STARTUP_VERDICT_ASSERTIVE_WORDING = 'startup-verdict-assertive-wording'; +/** + * The source could not be parsed, so this rule's verdict about it covers only + * what error recovery left standing (#10653). + * + * This rule's own subject matter, turned on the rule: a terminal conclusion + * drawn about a world that had not finished forming. "No findings" about a + * source the parser could not read is exactly that, and it is what this module + * used to return — silently, with a `catch` above it that never ran. + */ +export const STARTUP_SOURCE_UNPARSEABLE = 'startup-source-unparseable'; + // ── The vocabulary (词表) ──────────────────────────────────────────────────── /** @@ -590,8 +603,10 @@ interface VerdictRecord { * Find startup open-vocabulary verdicts in one TypeScript/JavaScript source. * * Pure: parses, never executes, never type-checks, touches no filesystem. An - * unparseable source yields no findings rather than throwing — this advises on - * source someone else owns, and refusing to parse is not a verdict about them. + * unparseable source is REPORTED ({@link STARTUP_SOURCE_UNPARSEABLE}) rather + * than thrown on — this advises on source someone else owns, so refusing to + * parse is not a verdict about them, but going silent about it was a verdict + * too, and the wrong one (#10653). */ export function findStartupRegistryVerdicts( source: string, @@ -612,14 +627,31 @@ export function findStartupRegistryVerdicts( const t = loadTypeScript(); const fileLabel = options.file ?? 'source'; - let sf: ts.SourceFile; - try { - sf = t.createSourceFile(fileLabel, source, t.ScriptTarget.Latest, true, t.ScriptKind.TS); - } catch { - return []; - } + // [#10653] The parse used to sit in a `try/catch` returning `[]`. The catch + // never ran — `createSourceFile` cannot throw (measured; see checked-parse.ts) + // — so the live path was the unread `parseDiagnostics`: a recovered partial + // tree walked and reported clean. The tree is still walked, so every finding + // this rule produces today it still produces; what is added is the signal that + // the reading was partial. + const { sourceFile: sf, failure } = createSourceFileChecked(t, fileLabel, source, { + target: t.ScriptTarget.Latest, + setParentNodes: true, + scriptKind: t.ScriptKind.TS, + }); const findings: StartupRegistryVerdictFinding[] = []; + if (failure) { + findings.push({ + severity: 'warning', + rule: STARTUP_SOURCE_UNPARSEABLE, + where: fileLabel, + path: `${fileLabel}:${failure.line}`, + message: + `source did not parse (${describeParseFailure(failure)}), so this rule read a partially recovered ` + + `tree — a startup verdict in the unread part is not reported.`, + hint: PARSE_FAILURE_HINT, + }); + } const moduleBindings = moduleLevelMutableBindings(t, sf); const functionBodies = indexFunctionBodies(t, sf); const lineOf = (node: ts.Node) => sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1; diff --git a/packages/lint/src/validate-action-body-writes.test.ts b/packages/lint/src/validate-action-body-writes.test.ts index c24079acfa..b72dd906ed 100644 --- a/packages/lint/src/validate-action-body-writes.test.ts +++ b/packages/lint/src/validate-action-body-writes.test.ts @@ -11,6 +11,7 @@ import { ACTION_BODY_WRITE_UNKNOWN_FIELD, ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR, ACTION_RECORD_WRITE_DISCARDED, + ACTION_BODY_SOURCE_UNPARSEABLE, } from './validate-action-body-writes.js'; import { extractHookBodyWrites, @@ -501,3 +502,46 @@ describe('[#8663] validateActionBodyWrites — unprovisioned anchor writes', () ).toEqual([]); }); }); + +// ── [#10653] The body that could not be READ, on the action surface ────────── +// +// Same extractor, same synthesised wrapper, same parse as the hook rule — so +// the body that came back silently unread there came back silently unread here. +// The hook rule's tests carry the attribution proof for the wrapper; what is +// pinned here is that this surface reports it too, rather than inheriting only +// the half of the fix that fits in one file. +describe('an unparseable action body is reported, not scored clean (#10653)', () => { + const wrecked = "const x = 1;\n/* TODO\nawait ctx.api.object('wh_order').updateById(ctx.recordId, { owner_id: 1 });\n"; + + it('POSITIVE CONTROL — the repaired body is flagged, so the wreck had something to lose', () => { + const repaired = "const x = 1;\nawait ctx.api.object('wh_order').updateById(ctx.recordId, { owner_id: 1 });\n"; + const findings = validateActionBodyWrites(actionStackOver(federatedObject, repaired)); + expect(findings.map((f) => f.rule)).toContain(ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR); + expect(findings.map((f) => f.rule)).not.toContain(ACTION_BODY_SOURCE_UNPARSEABLE); + }); + + it('the wrecked body loses that finding, and gains the parse one', () => { + const findings = validateActionBodyWrites(actionStackOver(federatedObject, wrecked)); + expect(findings.map((f) => f.rule)).not.toContain(ACTION_BODY_WRITE_UNPROVISIONED_ANCHOR); + const parseFindings = findings.filter((f) => f.rule === ACTION_BODY_SOURCE_UNPARSEABLE); + expect(parseFindings).toHaveLength(1); + expect(parseFindings[0].severity).toBe('warning'); + expect(parseFindings[0].where).toBe('action "stamp_owner" › body'); + expect(parseFindings[0].path).toBe('actions[0].body.source'); + expect(parseFindings[0].message).toContain('did not parse'); + }); + + it('FALSE-POSITIVE CONTROL — no body that parses gains a parse finding', () => { + const parseable = [ + "await ctx.api.object('wh_order').updateById(ctx.recordId, { owner_id: ctx.user.id });", + "ctx.record.stage = 'won'; await ctx.api.object('wh_order').update(ctx.record);", + "const r = ctx.record;\nif (r) { await ctx.api.object('wh_order').insert({ owner_id: 1 }); }", + ...ACTION_BODY_WRITE_PATTERNS.map((p) => p.example.source), + ...ACTION_RECORD_WRITE_PATTERNS.map((p) => p.example.source), + ]; + for (const source of parseable) { + const rules = validateActionBodyWrites(actionStackOver(federatedObject, source)).map((f) => f.rule); + expect(rules, `parseable body gained a parse finding:\n${source}`).not.toContain(ACTION_BODY_SOURCE_UNPARSEABLE); + } + }); +}); diff --git a/packages/lint/src/validate-action-body-writes.ts b/packages/lint/src/validate-action-body-writes.ts index fb6d743b1f..9fc4bc685d 100644 --- a/packages/lint/src/validate-action-body-writes.ts +++ b/packages/lint/src/validate-action-body-writes.ts @@ -77,6 +77,8 @@ // match either shape and never pays the TypeScript load. import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; + +import { describeParseFailure, PARSE_FAILURE_HINT } from './checked-parse.js'; import { indexUnprovisionedAnchors, unprovisionedAnchorCause, @@ -111,6 +113,18 @@ export interface ActionBodyWriteFinding { export const ACTION_BODY_WRITE_UNKNOWN_FIELD = 'action-body-write-unknown-field'; export const ACTION_RECORD_WRITE_DISCARDED = 'action-record-write-discarded'; +/** + * [#10653] The action-surface twin of `hook-body-source-unparseable`. Same + * extractor, same synthesised wrapper, same parse — so the body that came back + * silently unread on the hook surface came back silently unread here too. + * + * Wiring only the hook rule would have left the blind half standing at the call + * site next door, which this rule's own division of labour forbids: an action + * body runs through the same `HookBodySchema` and the same sandbox, so it gets + * the same treatment. + */ +export const ACTION_BODY_SOURCE_UNPARSEABLE = 'action-body-source-unparseable'; + /** * [#8663] The action-surface twin of `hook-body-write-unprovisioned-anchor`. * Same question, same wording, same `warning` severity — this rule and the hook @@ -304,13 +318,30 @@ export function validateActionBodyWrites(stack: AnyRec): ActionBodyWriteFinding[ if (!/\bapi\b/.test(site.source) && !/\brecord\b/.test(site.source)) continue; // ONE parse per body, both checks read from it. - const { writes: allWrites, ctxRecordEscapes } = extractHookBodyWriteSet(site.source); + const { writes: allWrites, ctxRecordEscapes, parseFailure } = extractHookBodyWriteSet(site.source); const writes = allWrites.filter((w) => APPLICABLE_IDS.has(w.patternId)); const recordWrites = allWrites.filter((w) => RECORD_WRITE_IDS.has(w.patternId)); - if (writes.length === 0 && recordWrites.length === 0) continue; const where = `action "${site.name}" › body`; + // [#10653] Read BEFORE the empty-write-set return below: an unparseable body + // is the one case where an empty write set means "not read" rather than + // "nothing written". + if (parseFailure) { + findings.push({ + severity: 'warning', + rule: ACTION_BODY_SOURCE_UNPARSEABLE, + where, + path: site.path, + message: + `L2 body did not parse (${describeParseFailure(parseFailure)}), so its write set was read from a ` + + `partially recovered tree — an undeclared field write in the unread part is not reported.`, + hint: PARSE_FAILURE_HINT, + }); + } + + if (writes.length === 0 && recordWrites.length === 0) continue; + // ── Discarded record writes (#4345) ────────────────────────────────── // Reported only when the write is PROVABLY dead: `ctx.record` never // leaves the body as a value, so nothing can persist the mutation. When diff --git a/packages/lint/src/validate-hook-body-writes.test.ts b/packages/lint/src/validate-hook-body-writes.test.ts index 16f91815d1..6cea10bd03 100644 --- a/packages/lint/src/validate-hook-body-writes.test.ts +++ b/packages/lint/src/validate-hook-body-writes.test.ts @@ -487,3 +487,112 @@ describe('[#8663] validateHookBodyWrites — unprovisioned anchor writes', () => expect(validateHookBodyWrites(stackOver(federatedObject, "ctx.input._id = 'x'; ctx.input.record_type = 'y';"))).toEqual([]); }); }); + +// ── [#10653] The body that could not be READ ───────────────────────────────── +// +// This extractor's contract said "error-tolerant (a body with syntax errors +// simply yields fewer matches)". Fewer matches is indistinguishable, at the call +// site, from a body that writes nothing — so an unparseable body came back as a +// hook with nothing to report. It is the same silence the undeclared write has +// at run time, this time wearing the checker's badge. +// +// This site is the delicate one of the three, because it parses a SYNTHESISED +// wrapper: `async function __body(ctx) { … }`, the shape the runtime compiles a +// hook body into. So "unparseable" here could in principle mean the wrapper is +// wrong rather than the author's source, and blaming an author for the checker's +// own bug would be a worse defect than the one being fixed. `the wrapper is not +// what fails` below is the mechanical proof that it cannot be. +describe('an unparseable hook body is reported, not scored clean (#10653)', () => { + // Same write in both halves; the only difference is the `/*` that eats it. + const wrecked = "const x = 1;\n/* TODO\nctx.input.amout = 0;\n"; + const repaired = "const x = 1;\nctx.input.amout = 0;\n"; + + it('POSITIVE CONTROL — the repaired body yields the write, so the wreck had something to lose', () => { + expect(extractHookBodyWriteSet(repaired).writes).toEqual([{ patternId: 'input-property-assign', field: 'amout' }]); + expect(extractHookBodyWriteSet(repaired).parseFailure).toBeUndefined(); + }); + + it('the wrecked body loses the write — the harm, reproduced', () => { + expect(extractHookBodyWriteSet(wrecked).writes).toEqual([]); + }); + + it('…and the set now carries the parse verdict instead of returning an empty write list', () => { + const failure = extractHookBodyWriteSet(wrecked).parseFailure; + expect(failure).toBeDefined(); + expect(failure!.count).toBeGreaterThan(0); + expect(failure!.message.length).toBeGreaterThan(0); + }); + + it('reports the position in the BODY’s coordinates, never the wrapper’s', () => { + // The author wrote the wreck on line 2 of their own body. The wrapper puts + // it on line 3 of what is actually parsed. + // + // The `ctx` on line 1 is load-bearing, not scenery: without it the raw-text + // pre-filter returns before any parse happens and there is no position to + // report at all. (This fixture was first written without it, and this test + // failed — the pre-filter contract holding, not a bug.) + const failure = extractHookBodyWriteSet('ctx.input.a = 1;\nconst y = ;\n').parseFailure!; + expect(failure.line).toBe(2); + }); + + it('the wrapper is not what fails — a parse failure is always the BODY’s (attribution proof)', () => { + // The wrapper is a constant. If it were ill-formed, it would fail around a + // trivially valid body too — and around every example the pattern ledger + // declares. It does not, so a diagnostic can only come from the body. + expect(extractHookBodyWriteSet('ctx.input.stage = 1;').parseFailure).toBeUndefined(); + expect(extractHookBodyWriteSet('Object.assign(ctx.input, {});').parseFailure).toBeUndefined(); + for (const pattern of HOOK_BODY_WRITE_PATTERNS) { + expect( + extractHookBodyWriteSet(pattern.example.source).parseFailure, + `the wrapper fails around a declared ledger example (${pattern.id}) — the synthesis is what is broken, ` + + `not the body, and this rule would be blaming the author for it`, + ).toBeUndefined(); + } + }); + + it('a body the runtime itself could not compile is the author’s, and is reported', () => { + // A hook body runs as `new AsyncFunction('ctx', source)`. `return`/`await` + // are legal there and must stay legal here (they parse inside the wrapper), + // while a body that is not a function body at all is a real author error. + expect(extractHookBodyWriteSet('await ctx.api.object("a").insert({});\nreturn 1;').parseFailure).toBeUndefined(); + expect(extractHookBodyWriteSet('ctx.input.a = 1;\n}\n').parseFailure, 'a stray brace closes the wrapper early') + .toBeDefined(); + }); + + it('the rule surfaces it as a finding on the hook', () => { + const findings = validateHookBodyWrites(stackOver(federatedObject, wrecked)); + const parseFindings = findings.filter((f) => f.rule === 'hook-body-source-unparseable'); + expect(parseFindings).toHaveLength(1); + expect(parseFindings[0].severity).toBe('warning'); + expect(parseFindings[0].where).toBe('hook "stamp" › body'); + expect(parseFindings[0].path).toBe('hooks[0].body.source'); + expect(parseFindings[0].message).toContain('did not parse'); + }); + + it('a body with no `ctx` and no `Object` is skipped WITHOUT a parse claim', () => { + // The pre-filter is a raw-text scan and is sound whether or not the body + // parses: no `ctx`/`Object` identifier means no pattern can match, however + // it parses. So the skipped parse hides nothing and must claim nothing. + expect(extractHookBodyWriteSet('const a = ;\n').parseFailure).toBeUndefined(); + expect(extractHookBodyWriteSet('const a = ;\n').writes).toEqual([]); + }); + + it('FALSE-POSITIVE CONTROL — no body that parses gains a parse finding', () => { + const parseable = [ + "ctx.input.stage = 'won';", + "await ctx.api.object('crm_deal').update({ stage: 'won' });", + "const r = ctx.record; r.stage = 'won';", + "if (ctx.input.amount > 0) { ctx.input.stage = 'won'; } else { ctx.input.stage = 'lost'; }", + "for (const k of Object.keys(ctx.input)) { ctx.input[k] = ctx.input[k]; }", + "try { await ctx.api.object('crm_deal').insert({ stage: 'x' }); } catch (e) { ctx.logger.warn(e); }", + "ctx.input.stage = `won-${ctx.user.id}`;", + ...HOOK_BODY_WRITE_PATTERNS.map((p) => p.example.source), + ]; + for (const source of parseable) { + expect( + extractHookBodyWriteSet(source).parseFailure, + `parseable body gained a parse failure:\n${source}`, + ).toBeUndefined(); + } + }); +}); diff --git a/packages/lint/src/validate-hook-body-writes.ts b/packages/lint/src/validate-hook-body-writes.ts index cc4e188420..9547a548b4 100644 --- a/packages/lint/src/validate-hook-body-writes.ts +++ b/packages/lint/src/validate-hook-body-writes.ts @@ -65,6 +65,13 @@ import { createRequire } from 'node:module'; import type ts from 'typescript'; import { findClosestMatches, formatSuggestion } from '@objectstack/spec/shared'; +import { + createSourceFileChecked, + describeParseFailure, + PARSE_FAILURE_HINT, + type SourceParseFailure, +} from './checked-parse.js'; + import { SYSTEM_FIELDS, indexUnprovisionedAnchors, @@ -122,6 +129,19 @@ export interface HookBodyWriteFinding { // Rule id (registry entry). export const HOOK_BODY_WRITE_UNKNOWN_FIELD = 'hook-body-write-unknown-field'; +/** + * [#10653] The body did not parse, so its write set is whatever error recovery + * left readable. + * + * Reported rather than skipped for the reason this rule exists at all: a + * mistake must be visible where it is MADE. An unparseable body reached the + * extractor, produced fewer matches, and came back as a hook with nothing to + * report — the same silence the undeclared write itself has at run time, this + * time wearing the checker's badge. `warning` because the whole rule is + * advisory and never gates (the severity type admits nothing else). + */ +export const HOOK_BODY_SOURCE_UNPARSEABLE = 'hook-body-source-unparseable'; + /** * [#8663] The write-axis twin of `flow-template-field-unprovisioned` (#8340): * the body writes a field {@link IMPLICIT_FIELDS} exempts, but on THIS target @@ -438,6 +458,31 @@ export interface ExtractedHookBodyWriteSet { * escape too, which is the safe direction: it suppresses findings.) */ ctxRecordEscapes: boolean; + /** + * [#10653] Set when the body did not parse, so `writes` is whatever error + * recovery left readable rather than the body's actual write set. + * + * Absent means one of two things, and they are not the same: the body parsed, + * or the cheap pre-filter above rejected it before any parse. The filter is a + * raw-text scan for `ctx` / `Object`, and a body containing neither cannot + * match any pattern however it parses — so a skipped parse claims nothing and + * hides nothing. + * + * ## Whose fault an unparseable body is — asked, not assumed + * + * The body is parsed inside a synthesised wrapper (`async function __body(ctx) + * { … }`) because that is the shape the runtime compiles it into + * (`new AsyncFunction('ctx', source)`). So a parse failure here could in + * principle be the WRAPPER's fault rather than the author's, and blaming the + * author for the checker's own bug is the failure this whole change is about. + * It cannot be: the wrapper is a constant, and `validate-hook-body-writes. + * test.ts` pins that it parses clean around an empty body and around every + * example in the pattern ledger. Any diagnostic therefore comes from the + * body — and its position is reported in the BODY's own coordinates (the + * wrapper's line is subtracted, and the result is clamped so it can never + * point at a line the author did not write). + */ + parseFailure?: SourceParseFailure; } /** @@ -465,12 +510,23 @@ export function extractHookBodyWriteSet(source: string): ExtractedHookBodyWriteS // The runtime wraps a hook body as `new AsyncFunction('ctx', source)` — a // FUNCTION BODY, not a module. Parse it in the same context so bare // `return` / `await` mean what they mean at run time. - const sf = tsc.createSourceFile( + // + // [#10653] Checked: this call cannot throw, so an unparseable body used to + // reach the walk below as a partially recovered tree and come back as "fewer + // matches" — indistinguishable from a body that genuinely writes nothing. The + // walk is unchanged; the verdict on the parse now rides out with the set. + // The wrapper adds exactly one line ahead of the author's source, which + // `synthesizedLinesBefore` takes back off the reported position. + const { sourceFile: sf, failure: parseFailure } = createSourceFileChecked( + tsc, 'hook-body.ts', `async function __body(ctx) {\n${source}\n}`, - tsc.ScriptTarget.Latest, - /* setParentNodes */ false, - tsc.ScriptKind.TS, + { + target: tsc.ScriptTarget.Latest, + setParentNodes: false, + scriptKind: tsc.ScriptKind.TS, + synthesizedLinesBefore: 1, + }, ); const writes: ExtractedHookBodyWrite[] = []; @@ -642,6 +698,7 @@ export function extractHookBodyWriteSet(source: string): ExtractedHookBodyWriteS return { writes, ctxRecordEscapes: recordRefs.some((ref) => !consumedRecordRefs.has(ref)), + ...(parseFailure ? { parseFailure } : {}), }; } @@ -667,12 +724,30 @@ export function validateHookBodyWrites(stack: AnyRec): HookBodyWriteFinding[] { const source = body.source; if (typeof source !== 'string' || source.trim() === '') return; - const writes = extractHookBodyWrites(source).filter((w) => HOOK_APPLICABLE_IDS.has(w.patternId)); + // [#10653] The SET, not the thin projection: the parse verdict rides with it, + // and it must be read BEFORE the `writes.length === 0` return below — an + // unparseable body is the one case where an empty write set means "not + // read" rather than "nothing written". + const extracted = extractHookBodyWriteSet(source); + const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`; + if (extracted.parseFailure) { + findings.push({ + severity: 'warning', + rule: HOOK_BODY_SOURCE_UNPARSEABLE, + where: `hook "${hookName}" › body`, + path: `hooks[${hookIndex}].body.source`, + message: + `L2 body did not parse (${describeParseFailure(extracted.parseFailure)}), so its write set was ` + + `read from a partially recovered tree — an undeclared field write in the unread part is not reported.`, + hint: PARSE_FAILURE_HINT, + }); + } + + const writes = extracted.writes.filter((w) => HOOK_APPLICABLE_IDS.has(w.patternId)); if (writes.length === 0) return; objectFields ??= indexObjectFields(stack); anchors ??= indexUnprovisionedAnchors(stack); - const hookName = typeof hook.name === 'string' && hook.name ? hook.name : `#${hookIndex}`; // The hook's own target set, for `ctx.input` writes. A wildcard target has // no single object to check against; a target whose fields cannot be judged diff --git a/packages/lint/src/validate-react-page-props.test.ts b/packages/lint/src/validate-react-page-props.test.ts index 498a983755..db928eac33 100644 --- a/packages/lint/src/validate-react-page-props.test.ts +++ b/packages/lint/src/validate-react-page-props.test.ts @@ -8,8 +8,12 @@ import { REACT_CHART_AXIS_UNKNOWN, REACT_CHART_DRILLDOWN_INVALID, REACT_BLOCK_NEEDS_RECORD_CONTEXT, + REACT_PAGE_SOURCE_UNPARSEABLE, type ReactPropFinding as PropFinding, } from './validate-react-page-props.js'; +// [#10653] The syntax gate whose cover the deleted `catch` comment credited, +// imported so the divergence between the two parsers is asserted, not described. +import { validateReactPages } from './validate-react-pages.js'; import { SEARCHABLE_FIELD_UNKNOWN, SEARCHABLE_FIELD_UNSEARCHABLE, @@ -1340,3 +1344,98 @@ describe('validateReactPageProps — unprovisioned injected anchors (#8340)', () expect(warned[0].message).toContain('external object (ADR-0015)'); }); }); + +// ── [#10653] The source that could not be READ ─────────────────────────────── +// +// The parse here used to sit in `try { … } catch { continue; }`, commented "the +// syntax gate reports unparseable sources". Two separate things were wrong with +// that, and the first one hid the second: +// +// 1. `ts.createSourceFile` cannot throw, so the catch never ran. The live path +// was the unread `parseDiagnostics` — error recovery hands back a partial +// tree, this gate walks it, finds no `` in the part that was +// lost, and returns clean. +// 2. The syntax gate parses with a DIFFERENT parser. `validate-jsx-pages.ts` +// does not lint `kind:'react'` at all; the cover is `validate-react- +// pages.ts`, which uses Sucrase. Measured on 2026-08-21, the two acceptance +// sets differ in both directions (see `the syntax gate is a different +// parser` below), so the syntax gate's silence was never evidence that THIS +// gate could read the source. +describe('an unparseable react source is reported, not scored clean (#10653)', () => { + // The defect (`` with no objectName) is real in both halves; the + // only difference is the `/*` that swallows it. + const wrecked = 'function Page(){\n /* TODO\n return ;\n}\n'; + const repaired = 'function Page(){\n return ;\n}\n'; + + it('POSITIVE CONTROL — the repaired source is flagged, so the wreck had something to lose', () => { + const f = validateReactPageProps(page(repaired)); + expect(f.some((x) => x.rule === 'react-prop-missing-required')).toBe(true); + expect(f.some((x) => x.rule === REACT_PAGE_SOURCE_UNPARSEABLE)).toBe(false); + }); + + it('the wrecked source loses that finding — the harm, reproduced', () => { + // This is the measurement the card turns on: not "it throws and we skip", + // but "it parses, the finding vanishes, and the verdict reads clean". + const f = validateReactPageProps(page(wrecked)); + expect(f.some((x) => x.rule === 'react-prop-missing-required')).toBe(false); + }); + + it('…and says so instead of returning clean', () => { + const f = validateReactPageProps(page(wrecked)); + const parseFindings = f.filter((x) => x.rule === REACT_PAGE_SOURCE_UNPARSEABLE); + expect(parseFindings).toHaveLength(1); + expect(parseFindings[0].severity).toBe('warning'); + expect(parseFindings[0].where).toBe('page "p"'); + expect(parseFindings[0].path).toBe('pages[0].source'); + expect(parseFindings[0].message).toContain('did not parse'); + expect(parseFindings[0].message).toMatch(/line \d+, column \d+/); + }); + + it('is ADDITIVE — a recovered tree keeps every finding it yields today', () => { + // `0755` is a parse diagnostic whose recovery is local: the JSX below it + // still reads. The parse finding is added; the prop finding is NOT traded + // away for it. (A fix that skipped the walk on failure would silently drop + // this one.) + const f = validateReactPageProps(page('const n = 0755;\nfunction Page(){ return ; }')); + expect(f.some((x) => x.rule === REACT_PAGE_SOURCE_UNPARSEABLE)).toBe(true); + expect(f.some((x) => x.rule === 'react-prop-missing-required')).toBe(true); + }); + + it('the syntax gate is a DIFFERENT parser — measured, not assumed', () => { + // The card recorded this divergence as unmeasured. Measured here: Sucrase + // accepts `0755`, TypeScript reports a parse diagnostic on it. So a react + // page can pass the syntax gate and still be unreadable to this one — which + // is the case where the silence this rule replaced was total. + const octal = 'const n = 0755;\nfunction Page(){ return ; }'; + expect(validateReactPages(page(octal)), 'sucrase accepts it').toEqual([]); + expect( + validateReactPageProps(page(octal)).some((x) => x.rule === REACT_PAGE_SOURCE_UNPARSEABLE), + 'typescript does not', + ).toBe(true); + }); + + it('FALSE-POSITIVE CONTROL — sources that parse never gain the finding', () => { + // Including the TS-only spellings a react page may legitimately carry: the + // parse is ScriptKind.TSX, and a generic arrow or a type annotation must not + // read as a wreck. + const parseable = [ + 'function Page(){ return ; }', + 'const Page = (): JSX.Element => ;\nexport default Page;', + 'const id = (x: T): T => x;\nfunction Page(){ return ; }', + 'interface Props { a: number }\nexport default function Page(p: Props){ return ; }', + 'function Page(){ return <>; }', + 'function Page(){ return ; }', + ]; + for (const source of parseable) { + const rules = validateReactPageProps(page(source)).map((x) => x.rule); + expect(rules, `parseable source gained a parse finding:\n${source}`).not.toContain( + REACT_PAGE_SOURCE_UNPARSEABLE, + ); + } + }); + + it('an empty source is skipped without a parse claim', () => { + expect(validateReactPageProps(page(''))).toEqual([]); + expect(validateReactPageProps(page(' \n '))).toEqual([]); + }); +}); diff --git a/packages/lint/src/validate-react-page-props.ts b/packages/lint/src/validate-react-page-props.ts index a560d360b9..fa986d97a8 100644 --- a/packages/lint/src/validate-react-page-props.ts +++ b/packages/lint/src/validate-react-page-props.ts @@ -62,6 +62,7 @@ import { // #5020's zod-rejection renderer, shared with the SDUI component-props gate // since #5068 — see `zod-issue-format.ts` for why one copy matters here. import { describeIssue } from './zod-issue-format.js'; +import { createSourceFileChecked, describeParseFailure, PARSE_FAILURE_HINT } from './checked-parse.js'; import { SYSTEM_FIELDS, @@ -261,6 +262,24 @@ function filterAttrValue(tsc: typeof ts, sf: ts.SourceFile, attr: ts.JsxAttribut // own chart-view wiring emit them, and they remain a valid (if unpublished) // way to write the same binding. +/** + * The source could not be parsed, so the prop checks below read a partially + * recovered tree (#10653). + * + * Severity is `warning`, not `error`, and the reason is measured rather than + * cautious. The syntax VERDICT on a react page belongs to `validate-react- + * pages.ts`, which transpiles the same source through **Sucrase** — the parser + * family that actually compiles a react page — and errors when it refuses. This + * rule speaks for a different parser, and the two acceptance sets are not the + * same set: measured on 2026-08-21 (TypeScript 6.0.3 / sucrase 3.35.x), a react + * source containing `0755`, `'\012'`, `0b2` or `1__0` parses CLEAN through + * Sucrase while TypeScript reports a parse diagnostic, and `with (o) {}` goes + * the other way. Erroring here would newly fail builds the platform's own + * transpiler accepts; going silent is the defect this rule closes. A warning + * says the true thing: these checks did not get to run. + */ +export const REACT_PAGE_SOURCE_UNPARSEABLE = 'react-page-source-unparseable'; + export const REACT_CHART_FIELD_UNKNOWN = 'react-chart-field-unknown'; export const REACT_CHART_FIELD_UNPROVISIONED = 'react-chart-field-unprovisioned'; export const REACT_CHART_AGGREGATE_INVALID = 'react-chart-aggregate-invalid'; @@ -978,15 +997,36 @@ export function validateReactPageProps(stack: AnyRec): ReactPropFinding[] { if (typeof source !== 'string' || source.trim() === '') continue; const name = String(page.name ?? `#${p}`); - // Outside the try below on purpose: a missing compiler must surface as an - // error, not be swallowed as "unparseable source". + // A missing compiler must surface as an error, not as "unparseable source": + // one is this deployment's problem, the other is the author's. const tsc = loadTypeScript(); - let sf: ts.SourceFile; - try { - sf = tsc.createSourceFile('page.tsx', source, tsc.ScriptTarget.Latest, true, tsc.ScriptKind.TSX); - } catch { - continue; // the syntax gate reports unparseable sources + // [#10653] The parse used to sit in a `try/catch` that continued to the next + // page, on the reading that an unparseable source is the syntax gate's to + // report. Both halves were wrong. `createSourceFile` cannot throw (measured; + // see checked-parse.ts), so the catch never ran and the LIVE path was the + // unread `parseDiagnostics`: a recovered partial tree walked and scored + // clean. And the syntax gate parses with a different parser (Sucrase), so + // its silence is not evidence that this one could read the source. + // + // The recovered tree is still walked below — whatever findings it yields + // today it keeps yielding. What is added is the missing signal. + const { sourceFile: sf, failure } = createSourceFileChecked(tsc, 'page.tsx', source, { + target: tsc.ScriptTarget.Latest, + setParentNodes: true, + scriptKind: tsc.ScriptKind.TSX, + }); + if (failure) { + findings.push({ + severity: 'warning', + rule: REACT_PAGE_SOURCE_UNPARSEABLE, + where: `page "${name}"`, + path: `pages[${p}].source`, + message: + `kind:'react' source did not parse (${describeParseFailure(failure)}), so the component-contract ` + + `checks read a partially recovered tree and may have missed real problems.`, + hint: PARSE_FAILURE_HINT, + }); } const locals = localComponentNames(tsc, sf);