Skip to content

Commit 277379c

Browse files
claude[bot]claude
andauthored
fix(lint): visibility-bare-identifier reports the unwrapped occurrence beside a has() guard (#16413)
* test(lint): pin the three `has()`-guard rows on `visibility-bare-identifier` Red-first pins for the reachability gap: an identifier written bare beside a `has()` guard in the same visibility predicate publishes clean, while the same identifier alone gates. Two of the eight rows fail on this commit — the guarded row and the different-name row that tells positional masking apart from a name-keyed exclusion — and the `has(status)`-alone row is the negative control that must stay silent after the fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * fix(lint): key the `has()` exclusion in `visibility-bare-identifier` to the occurrence `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, so a first error of a different class masked every undeclared reference behind it in the same predicate — the same name and any other one alike. Mask each `has(…)` call out of the source before the checker sees it, using the canonical AST's own spans and a same-width `true` literal. The argument occurrence is excluded, which is the exclusion it earns as a select target, while every other occurrence is judged exactly as it would be with no guard written beside it. The rewrite only deletes source, so it can remove a finding but never invent one, and the rest of the predicate reaches the checker byte-identical. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 * chore(changeset): patch `@objectstack/lint` for the has()-guard bare-identifier fix Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vbw3RPgdtqesx4azk9SbW8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 43e6661 commit 277379c

3 files changed

Lines changed: 165 additions & 2 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
`visibility-bare-identifier` now reports an identifier written bare beside a `has()` guard in the same visibility predicate.
6+
7+
`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.
8+
9+
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.
10+
11+
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.

packages/lint/src/validate-visibility-predicates.test.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,74 @@ describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => {
770770
expect(findings[0].message).toContain('`status`');
771771
});
772772
});
773+
774+
describe('a `has()` guard does not silence an unwrapped occurrence beside it', () => {
775+
// Three rows measured on one byte-identical authoring site: a guarded
776+
// spelling that forgot the `record.` prefix published clean while the
777+
// unguarded spelling of the SAME predicate gated. The exclusion a `has()`
778+
// argument earns is per OCCURRENCE — the argument itself is a legitimate
779+
// select target — and never spreads to the rest of the predicate.
780+
//
781+
// The cause was not an exclusion at all: `firstUndeclaredReference` reads
782+
// the FIRST error cel-js's checker reports and acts only on
783+
// `Unknown variable: X`. A bare `has(x)` — `has()` applied to something
784+
// that is not a select — fails the check with `has() invalid argument`
785+
// instead, and that different-class first error masked every undeclared
786+
// reference behind it in the same predicate.
787+
788+
it('row 1 — the unguarded spelling gates (the control that must keep firing)', () => {
789+
const findings = bareFindings(formStack('status == "qualified"'));
790+
expect(findings).toHaveLength(1);
791+
expect(findings[0].severity).toBe('error');
792+
expect(findings[0].message).toContain('`status`');
793+
});
794+
795+
it('row 2 — the SAME identifier unwrapped beside `has(status)` is reported', () => {
796+
const findings = bareFindings(formStack('has(status) && status == "qualified"'));
797+
expect(findings).toHaveLength(1);
798+
expect(findings[0].severity).toBe('error');
799+
expect(findings[0].message).toContain('`status`');
800+
expect(findings[0].hint).toContain('`record.status`');
801+
});
802+
803+
it('row 3 — `has(status)` ALONE stays silent (the negative control)', () => {
804+
// The exclusion stays: a `has()` argument is a select target, so the one
805+
// occurrence that IS the argument earns no bare-identifier verdict. Pinned
806+
// so a later refactor cannot delete the exclusion wholesale and still read
807+
// green off row 2.
808+
expect(validateVisibilityPredicates(formStack('has(status)'))).toEqual([]);
809+
});
810+
811+
it('a prefixed guard beside a bare use still reports the bare use', () => {
812+
const findings = bareFindings(formStack('has(record.status) && status == "qualified"'));
813+
expect(findings).toHaveLength(1);
814+
expect(findings[0].message).toContain('`status`');
815+
});
816+
817+
it('a bare guard beside a PREFIXED use is silent — only the argument occurred bare', () => {
818+
expect(bareFindings(formStack('has(status) && record.status == "qualified"'))).toEqual([]);
819+
});
820+
821+
it('a bare `has()` does not mask a DIFFERENT name behind it either', () => {
822+
// The masking was positional, not name-keyed: everything after the first
823+
// non-`Unknown variable` checker error went unjudged, whatever it was
824+
// called. This row is what tells the two mechanisms apart.
825+
const findings = bareFindings(formStack('has(status) && overdue == true'));
826+
expect(findings).toHaveLength(1);
827+
expect(findings[0].message).toContain('`overdue`');
828+
});
829+
830+
it('several bare `has()` arguments and nothing else stay silent', () => {
831+
expect(validateVisibilityPredicates(formStack('has(status) && has(owner)'))).toEqual([]);
832+
});
833+
834+
it('a comprehension variable behind a `has()` guard is still not a bare identifier', () => {
835+
// The checker remains the oracle for macro-bound names: the body is handed
836+
// to it unchanged, so `t` keeps resolving the way it does without a guard.
837+
expect(validateVisibilityPredicates(formStack("has(record.tags) && record.tags.all(t, t != '')"))).toEqual([]);
838+
expect(validateVisibilityPredicates(formStack("has(tags) && record.tags.all(t, t != '')"))).toEqual([]);
839+
});
840+
});
773841
});
774842

775843
// ─────────────────────────────────────────────────────────────────────

packages/lint/src/validate-visibility-predicates.ts

Lines changed: 86 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -323,7 +323,10 @@
323323
* bound root, only whether the identifier has a root at all, and a rootless
324324
* identifier resolves under neither binding. `has(record.x)`,
325325
* `record.x != null` and every other guard idiom stay green here, whichever
326-
* way #4953 is eventually settled.
326+
* way #4953 is eventually settled. The exclusion a `has()` argument earns is
327+
* keyed to that ARGUMENT's occurrence, never to its name: the same identifier
328+
* written bare elsewhere in the predicate is reported exactly as it would be
329+
* with no guard beside it (#16118).
327330
* - **A predicate the canonical front end will not parse.** `parseCelToAst`
328331
* returns `null` there, so the declaredness check has no AST to reason about
329332
* 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<string>): void {
752755
namespaceRoots(args, out);
753756
}
754757

758+
/**
759+
* The literal that stands in for a masked `has(…)` call. Padded to the call's
760+
* own width so the rewrite is length-preserving — the source handed to the
761+
* checker keeps every other token at its original offset, and the spans stay
762+
* valid whatever order they are applied in.
763+
*/
764+
const HAS_MASK = 'true';
765+
766+
/**
767+
* Half-open `[start, end)` source spans of every `has(…)` call in `ast`.
768+
* Returns `false` — meaning "do not rewrite anything" — if any span is missing
769+
* or does not line up with the source, so an AST shape this has not measured
770+
* costs coverage rather than producing a wrong span.
771+
*
772+
* A `has()` call is not descended into: the whole call, its ARGUMENT included,
773+
* is what gets masked, which is precisely the exclusion a `has()` argument
774+
* earns (it is a select target, not a bare value).
775+
*/
776+
function hasCallSpans(node: unknown, source: string, out: Array<[number, number]>): boolean {
777+
if (Array.isArray(node)) {
778+
for (const child of node) if (!hasCallSpans(child, source, out)) return false;
779+
return true;
780+
}
781+
if (!isNode(node)) return true;
782+
const args = node.args;
783+
if (node.op === 'call' && Array.isArray(args) && args[0] === 'has') {
784+
const { start, end } = node as { start?: unknown; end?: unknown };
785+
if (!Number.isInteger(start) || !Number.isInteger(end)) return false;
786+
const from = start as number;
787+
const to = end as number;
788+
if (from < 0 || to > source.length || to - from < HAS_MASK.length) return false;
789+
if (!source.slice(from, to).startsWith('has')) return false;
790+
out.push([from, to]);
791+
return true;
792+
}
793+
return hasCallSpans(args, source, out);
794+
}
795+
796+
/**
797+
* `source` with every `has(…)` call replaced by a `true` literal of the same
798+
* width, or `source` unchanged when it carries none (or carries one this cannot
799+
* locate exactly).
800+
*
801+
* This is what keeps the `has()` exclusion keyed to the OCCURRENCE rather than
802+
* to the NAME (#16118). Two facts make the rewrite necessary:
803+
*
804+
* - A `has()` argument is a legitimate select target, so the occurrence that
805+
* IS the argument must earn no bare-identifier verdict — including the
806+
* `has(x)` spelling, where the argument is not a select at all.
807+
* - `firstUndeclaredReference` reads the FIRST error cel-js's checker reports
808+
* and acts only on `Unknown variable: X`. A bare `has(x)` fails that check
809+
* with `has() invalid argument` instead, and a first error of a different
810+
* class masked every undeclared reference BEHIND it in the same predicate —
811+
* the same name and any other. `has(x) && x == 'q'` and
812+
* `has(x) && y == 'q'` both published clean.
813+
*
814+
* Masking the call before the checker sees it removes both at once: the
815+
* argument occurrence is gone, and every other occurrence is judged exactly as
816+
* it would be with no guard written at all.
817+
*
818+
* The direction is safe by construction: the rewrite only DELETES source, so
819+
* every name the checker can now report is an identifier written outside a
820+
* `has()` call in the original. It cannot invent one. The rest of the predicate
821+
* reaches the checker byte-identical, so the shapes the checker owns —
822+
* comprehension-macro variables above all — keep the verdict they have without
823+
* a guard.
824+
*/
825+
function maskHasCalls(source: string, ast: unknown): string {
826+
const spans: Array<[number, number]> = [];
827+
if (!hasCallSpans(ast, source, spans) || spans.length === 0) return source;
828+
let masked = source;
829+
for (const [from, to] of spans) {
830+
masked = masked.slice(0, from) + HAS_MASK.padEnd(to - from, ' ') + masked.slice(to);
831+
}
832+
return masked;
833+
}
834+
755835
/**
756836
* The first identifier in `source` that no binding root can resolve, or `null`
757837
* when every reference is rooted. See the module note for why this is two
758838
* oracles (the canonical AST for namespace roots, the shared strict-environment
759839
* checker for the verdict) and for the shapes it deliberately leaves alone.
760840
*
841+
* The source reaching the checker is {@link maskHasCalls}'s rewrite, not the
842+
* author's bytes: a `has(…)` argument is excluded per OCCURRENCE, and every
843+
* other occurrence of the same name is judged as if no guard were written.
844+
*
761845
* `literalRhs` is set for a METADATA-EDITING form, where the console's
762846
* evaluator hands the right of `==` / `!=` to its literal parser and never
763847
* 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
777861
const rooted = new Set<string>();
778862
namespaceRoots(ast, rooted);
779863
const literalSlot = literalRhs ? bareRhsOnlyIdentifiers(ast) : [];
780-
return firstUndeclaredReference(source, [
864+
return firstUndeclaredReference(maskHasCalls(source, ast), [
781865
...VIEW_PAGE_EXTRA_ROOTS,
782866
...rooted,
783867
...literalSlot,

0 commit comments

Comments
 (0)