diff --git a/.changeset/visibility-bare-identifier-has-occurrence.md b/.changeset/visibility-bare-identifier-has-occurrence.md new file mode 100644 index 0000000000..0b1affa86a --- /dev/null +++ b/.changeset/visibility-bare-identifier-has-occurrence.md @@ -0,0 +1,11 @@ +--- +"@objectstack/lint": patch +--- + +`visibility-bare-identifier` now reports an identifier written bare beside a `has()` guard in the same visibility predicate. + +`has(status) && status == "qualified"` published clean while `status == "qualified"` — the same defect, without the guard — gated at `error`. The guarded spelling is the one the totality discipline pushes authors toward, so an author who correctly adds `has()` and forgets the `record.` prefix on both halves landed in the silent row. That predicate never evaluates for any record, and an unevaluable `visibleWhen` on a form surface fails OPEN: the field renders and carries its `required: true` into the console's submit check. + +The cause was not the exclusion a `has()` argument earns — that is correct and stays. `firstUndeclaredReference` reads the first error the CEL checker reports and acts only on `Unknown variable: X`; a bare `has(x)` fails that check with `has() invalid argument` instead, and a first error of a different class masked every undeclared reference behind it in the same predicate, whatever it was called. Each `has(…)` call is now masked out of the source before the checker sees it, using the canonical AST's own spans, so the argument occurrence is excluded and every other occurrence is judged exactly as it would be with no guard written beside it. + +Expect new `error` findings on predicates that used to publish clean: a guarded-but-unprefixed `visibleWhen` on a view, page component or form section is now refused at build, validate and lint alike. That is the fail-open shape the rule exists to catch. A `has()` argument that is the only bare occurrence — `has(status)` on its own — stays silent, as it did before. diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index 618b6f4b8a..b5c5681c30 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -770,6 +770,74 @@ describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => { expect(findings[0].message).toContain('`status`'); }); }); + + describe('a `has()` guard does not silence an unwrapped occurrence beside it', () => { + // Three rows measured on one byte-identical authoring site: a guarded + // spelling that forgot the `record.` prefix published clean while the + // unguarded spelling of the SAME predicate gated. The exclusion a `has()` + // argument earns is per OCCURRENCE — the argument itself is a legitimate + // select target — and never spreads to the rest of the predicate. + // + // The cause was not an exclusion at all: `firstUndeclaredReference` reads + // the FIRST error cel-js's checker reports and acts only on + // `Unknown variable: X`. A bare `has(x)` — `has()` applied to something + // that is not a select — fails the check with `has() invalid argument` + // instead, and that different-class first error masked every undeclared + // reference behind it in the same predicate. + + it('row 1 — the unguarded spelling gates (the control that must keep firing)', () => { + const findings = bareFindings(formStack('status == "qualified"')); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].message).toContain('`status`'); + }); + + it('row 2 — the SAME identifier unwrapped beside `has(status)` is reported', () => { + const findings = bareFindings(formStack('has(status) && status == "qualified"')); + expect(findings).toHaveLength(1); + expect(findings[0].severity).toBe('error'); + expect(findings[0].message).toContain('`status`'); + expect(findings[0].hint).toContain('`record.status`'); + }); + + it('row 3 — `has(status)` ALONE stays silent (the negative control)', () => { + // The exclusion stays: a `has()` argument is a select target, so the one + // occurrence that IS the argument earns no bare-identifier verdict. Pinned + // so a later refactor cannot delete the exclusion wholesale and still read + // green off row 2. + expect(validateVisibilityPredicates(formStack('has(status)'))).toEqual([]); + }); + + it('a prefixed guard beside a bare use still reports the bare use', () => { + const findings = bareFindings(formStack('has(record.status) && status == "qualified"')); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('`status`'); + }); + + it('a bare guard beside a PREFIXED use is silent — only the argument occurred bare', () => { + expect(bareFindings(formStack('has(status) && record.status == "qualified"'))).toEqual([]); + }); + + it('a bare `has()` does not mask a DIFFERENT name behind it either', () => { + // The masking was positional, not name-keyed: everything after the first + // non-`Unknown variable` checker error went unjudged, whatever it was + // called. This row is what tells the two mechanisms apart. + const findings = bareFindings(formStack('has(status) && overdue == true')); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('`overdue`'); + }); + + it('several bare `has()` arguments and nothing else stay silent', () => { + expect(validateVisibilityPredicates(formStack('has(status) && has(owner)'))).toEqual([]); + }); + + it('a comprehension variable behind a `has()` guard is still not a bare identifier', () => { + // The checker remains the oracle for macro-bound names: the body is handed + // to it unchanged, so `t` keeps resolving the way it does without a guard. + expect(validateVisibilityPredicates(formStack("has(record.tags) && record.tags.all(t, t != '')"))).toEqual([]); + expect(validateVisibilityPredicates(formStack("has(tags) && record.tags.all(t, t != '')"))).toEqual([]); + }); + }); }); // ───────────────────────────────────────────────────────────────────── diff --git a/packages/lint/src/validate-visibility-predicates.ts b/packages/lint/src/validate-visibility-predicates.ts index 608a3b36a7..5fdac4f820 100644 --- a/packages/lint/src/validate-visibility-predicates.ts +++ b/packages/lint/src/validate-visibility-predicates.ts @@ -323,7 +323,10 @@ * bound root, only whether the identifier has a root at all, and a rootless * identifier resolves under neither binding. `has(record.x)`, * `record.x != null` and every other guard idiom stay green here, whichever - * way #4953 is eventually settled. + * way #4953 is eventually settled. The exclusion a `has()` argument earns is + * keyed to that ARGUMENT's occurrence, never to its name: the same identifier + * written bare elsewhere in the predicate is reported exactly as it would be + * with no guard beside it (#16118). * - **A predicate the canonical front end will not parse.** `parseCelToAst` * returns `null` there, so the declaredness check has no AST to reason about * and this rule gives no BARE-IDENTIFIER verdict on it. That is a division of @@ -752,12 +755,93 @@ function namespaceRoots(node: unknown, out: Set): void { namespaceRoots(args, out); } +/** + * The literal that stands in for a masked `has(…)` call. Padded to the call's + * own width so the rewrite is length-preserving — the source handed to the + * checker keeps every other token at its original offset, and the spans stay + * valid whatever order they are applied in. + */ +const HAS_MASK = 'true'; + +/** + * Half-open `[start, end)` source spans of every `has(…)` call in `ast`. + * Returns `false` — meaning "do not rewrite anything" — if any span is missing + * or does not line up with the source, so an AST shape this has not measured + * costs coverage rather than producing a wrong span. + * + * A `has()` call is not descended into: the whole call, its ARGUMENT included, + * is what gets masked, which is precisely the exclusion a `has()` argument + * earns (it is a select target, not a bare value). + */ +function hasCallSpans(node: unknown, source: string, out: Array<[number, number]>): boolean { + if (Array.isArray(node)) { + for (const child of node) if (!hasCallSpans(child, source, out)) return false; + return true; + } + if (!isNode(node)) return true; + const args = node.args; + if (node.op === 'call' && Array.isArray(args) && args[0] === 'has') { + const { start, end } = node as { start?: unknown; end?: unknown }; + if (!Number.isInteger(start) || !Number.isInteger(end)) return false; + const from = start as number; + const to = end as number; + if (from < 0 || to > source.length || to - from < HAS_MASK.length) return false; + if (!source.slice(from, to).startsWith('has')) return false; + out.push([from, to]); + return true; + } + return hasCallSpans(args, source, out); +} + +/** + * `source` with every `has(…)` call replaced by a `true` literal of the same + * width, or `source` unchanged when it carries none (or carries one this cannot + * locate exactly). + * + * This is what keeps the `has()` exclusion keyed to the OCCURRENCE rather than + * to the NAME (#16118). Two facts make the rewrite necessary: + * + * - A `has()` argument is a legitimate select target, so the occurrence that + * IS the argument must earn no bare-identifier verdict — including the + * `has(x)` spelling, where the argument is not a select at all. + * - `firstUndeclaredReference` reads the FIRST error cel-js's checker reports + * and acts only on `Unknown variable: X`. A bare `has(x)` fails that check + * with `has() invalid argument` instead, and a first error of a different + * class masked every undeclared reference BEHIND it in the same predicate — + * the same name and any other. `has(x) && x == 'q'` and + * `has(x) && y == 'q'` both published clean. + * + * Masking the call before the checker sees it removes both at once: the + * argument occurrence is gone, and every other occurrence is judged exactly as + * it would be with no guard written at all. + * + * The direction is safe by construction: the rewrite only DELETES source, so + * every name the checker can now report is an identifier written outside a + * `has()` call in the original. It cannot invent one. The rest of the predicate + * reaches the checker byte-identical, so the shapes the checker owns — + * comprehension-macro variables above all — keep the verdict they have without + * a guard. + */ +function maskHasCalls(source: string, ast: unknown): string { + const spans: Array<[number, number]> = []; + if (!hasCallSpans(ast, source, spans) || spans.length === 0) return source; + let masked = source; + for (const [from, to] of spans) { + masked = masked.slice(0, from) + HAS_MASK.padEnd(to - from, ' ') + masked.slice(to); + } + return masked; +} + /** * The first identifier in `source` that no binding root can resolve, or `null` * when every reference is rooted. See the module note for why this is two * oracles (the canonical AST for namespace roots, the shared strict-environment * checker for the verdict) and for the shapes it deliberately leaves alone. * + * The source reaching the checker is {@link maskHasCalls}'s rewrite, not the + * author's bytes: a `has(…)` argument is excluded per OCCURRENCE, and every + * other occurrence of the same name is judged as if no guard were written. + * * `literalRhs` is set for a METADATA-EDITING form, where the console's * evaluator hands the right of `==` / `!=` to its literal parser and never * resolves it (objectui#4049). A bare word there is not a dropped root — it is @@ -777,7 +861,7 @@ function firstBareIdentifier(source: string, literalRhs: boolean): string | null const rooted = new Set(); namespaceRoots(ast, rooted); const literalSlot = literalRhs ? bareRhsOnlyIdentifiers(ast) : []; - return firstUndeclaredReference(source, [ + return firstUndeclaredReference(maskHasCalls(source, ast), [ ...VIEW_PAGE_EXTRA_ROOTS, ...rooted, ...literalSlot,