diff --git a/.changeset/strict-env-scope-roots-dyn.md b/.changeset/strict-env-scope-roots-dyn.md new file mode 100644 index 0000000000..a0ca4a33de --- /dev/null +++ b/.changeset/strict-env-scope-roots-dyn.md @@ -0,0 +1,88 @@ +--- +"@objectstack/formula": minor +"@objectstack/lint": minor +--- + +fix(formula): the strict declaredness env declares `SCOPE_ROOTS` as `dyn`, so a bare reference behind a root name is no longer masked (#16412) + + + +**BREAKING** in the accept-set sense — an accept-set narrowing on published +CHECKERS, in the same sense as a route that starts refusing a request it should +always have refused — landing in the launch window as `minor` on both packages (during the window the bump level is +not the carrier of breaking-ness; this paragraph and the disposition above +are). Nothing that was already reported stops being reported, and no source +that is correct starts being reported. + +`firstUndeclaredReference` asks cel-js's checker for the first undeclared +identifier in a source. That checker returns exactly ONE error, and the helper +acts only on `Unknown variable: X`, so whenever the first error is of another +class every undeclared reference behind it in the same source went unjudged and +the helper answered `null` — which is also the value that means "every +reference is rooted". Four published call sites read that answer, and none of +them can tell the two readings apart. + +The widest way to reach that state was a disagreement between two environments +in this package about the same names. The strict env declared every +`SCOPE_ROOTS` member (`data`, `config`, `record`, `result`, `item`, `event`, +`input`, `user`, …) as `map`, while the permissive env that `celEngine.compile` +type-checks in leaves them `dyn`. `map` has no `==`, `<` or `+` overload, so an +ordinary comparison on one of those names compiled clean and then faulted `no +such overload` in the strict env only — taking the single error slot and +silencing everything behind it. An author reaches it by naming an object field +or a flow variable after a namespace root and reading it bare, which on a +metadata-editing form is not even a coincidence: that layer binds the row under +edit as `data`. + +The strict env now declares those roots `dyn`, which is what the list's own +doc-comment already claimed it was for — member access, arithmetic and +comparison on a root all deferring to runtime — and which `map` delivered only +the first of. The two environments agree about these names, so the class cannot +arise rather than being compensated for downstream. + +What starts reporting, measured on each published surface: + +- `@objectstack/formula` `validateExpression` with `scope: 'record'` — a bare + reference behind a root name is the hard error it always was for the same + identifier written first (`ok` was `true` with zero errors; it is now `false`). +- `@objectstack/formula` `validateExpression` with `scope: 'flattened'` — the + did-you-mean warning reaches a misspelled field behind a root name. +- `@objectstack/lint` `visibility-bare-identifier` — a bare identifier behind a + root name in a `visibleWhen` predicate is a finding. Per that rule's own + message the console otherwise falls open and the element renders + unconditionally. +- `@objectstack/lint` flow-variable shadowing — a shadowed field read behind a + root name is warned. That rule's documented blind spot is now name-local, as + its wording always claimed: the colliding name itself is still not reported. + +⚠️ One published answer also WIDENS, and it is not a reporting surface. +`inferExpressionType` (`@objectstack/formula`, re-exported from the package +root; read by `@objectstack/mcp` as `validate_expression.inferredType`) infers a +formula's coarse value type through `inferCelType`, which shares this same +strict environment. While the roots were `map` there was no `==`, `<` or `+` +overload for them, so an expression using a namespace root as a DIRECT OPERAND +did not type-check at all and the answer was `'unknown'`. With the roots `dyn` +those expressions type-check and the answer is the truthful CEL type: +`result + 1` and `record ? 1 : 2` → `'number'`, `record == "x"` → `'boolean'`, +`data == "x" ? "a" : "b"` → `'text'`, uniformly for every name on the list. No +answer changes from one concrete type to another and nothing narrows to +`'unknown'` — `size(record)` and `"a" in record` still answer, and a root that +is only the base of a member access (`record.amount > 100`) never consulted this +declaration. A consumer that keys off a concrete type therefore sees strictly +more expressions classified, never a different classification; for the +motivating consumer that means a formula written as `data == "x" ? "a" : "b"` is +now correctly seen as text rather than as unprovable. Pinned on both sides in +`validate.test.ts`. + +⛔ Two first-error classes are NOT closed by this, and both stay pinned. A CEL +TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, so no +declaration this package makes can reach it; measured on the strict env, the +message for `type == 'grid'` is byte-identical under a `map` and a `dyn` root +declaration. And `has()` handed a non-select argument still faults its own +class, which `@objectstack/lint`'s visibility rule masks at its own call site +(#16118) and which nothing else masks. + +The narrowing this helper is built on is unchanged: it still acts only on +`Unknown variable`, so `type(record.x) == string`, comprehension macros, guard +idioms, optional chaining and stdlib calls report nothing, and a widening of +that regex onto the overload message remains refused. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index 88f74c9c99..f943fd1b57 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -65,11 +65,20 @@ export function buildEnv(now: () => Date, timezone = 'UTC'): Environment { /** * Namespace roots that a `record`-scoped CEL site may legitimately reference. - * Declared as `map` (dyn values) so member access (`record.foo`) and any - * arithmetic/comparison on it defers to runtime — the strict env faults ONLY on - * an *undeclared* top-level identifier, i.e. a bare field reference. Generous on - * purpose: an unknown root is a missed catch, a missing root is a false positive - * that would break the build, so we err toward declaring more. + * Declared so that member access (`record.foo`) and any arithmetic/comparison on + * a root both defer to runtime — the strict env faults ONLY on an *undeclared* + * top-level identifier, i.e. a bare field reference. Generous on purpose: an + * unknown root is a missed catch, a missing root is a false positive that would + * break the build, so we err toward declaring more. + * + * ⚠️ The CEL type each env declares them AS is not uniform, and the difference is + * load-bearing rather than incidental. {@link buildScopedEnv} — the declaredness + * oracle — declares them `dyn`, because only `dyn` delivers BOTH halves of the + * sentence above; `map` delivered member access and faulted the comparison, and + * #16412 measured what that cost (see {@link firstUndeclaredReference}). The + * type-soundness envs keep them `map`: there a root is a container the check + * deliberately declines to reason through, and the typed struct on + * `record`/`previous`/`input` is what carries the field types. * * ## Why this list is PUBLISHED (#6713) * @@ -152,8 +161,28 @@ function buildScopedEnv(knownFields: readonly string[]): Environment { limits: DEFAULT_LIMITS, }); registerStdLib(env, () => new Date(0)); + // Roots are `dyn`, NOT `map`, for exactly the reason `knownFields` are (below) + // and the doc-comment on {@link SCOPE_ROOTS} already claims: member access, + // arithmetic and comparison on a root must all defer to runtime, so that the + // ONLY thing this env faults on is an undeclared top-level identifier. + // + // `map` delivered the member half and not the other two (#16412): `map` has no + // `==` / `<` / `+` overload, so `data == 'x'` — a root, or an object field + // sharing a root's name, in an ordinary comparison — faulted `no such + // overload` HERE while compiling clean in the permissive env, where the same + // names are `dyn`. Because cel-js's checker reports exactly ONE error, that + // fault took the first slot and every undeclared reference behind it in the + // same source went unjudged: `firstUndeclaredReference` answered `null`, the + // value that also means "every reference is rooted", and four consuming rules + // published the source clean. The two environments now agree about these + // names, so the class cannot arise rather than being compensated for + // downstream. + // + // ⛔ This does NOT weaken the check: `dyn` is undeclared-identifier-neutral — + // it changes what is legal ON a declared root, never whether an UNdeclared + // name faults. The `knownFields` loop below has always relied on that. for (const root of SCOPE_ROOTS) { - try { env.registerVariable(root, 'map'); } catch { /* duplicate — ignore */ } + try { env.registerVariable(root, 'dyn'); } catch { /* duplicate — ignore */ } } // `knownFields` are declared as `dyn` so they (and member/arith/compare on // them) never fault — only a genuinely-undeclared top-level identifier does. @@ -194,35 +223,48 @@ let recordScopeEnv: Environment | undefined; * * The masking is POSITIONAL, not name-keyed: the masked name is not the one * that triggered the first error, so excluding the trigger's own name does not - * reach it. Measured on this env: - * - * data == 'x' && status == 'q' -> null first error `no such - * overload: map == - * string`; `status` unjudged - * status == 'q' && data == 'x' -> "status" first error `Unknown - * variable: status` + * reach it. * * ⚠️ {@link celEngine.compile} is not a gate against this, so a caller that * only reaches here on a clean compile is not protected by that gate. `compile` * type-checks in the PERMISSIVE env ({@link CEL_ENV_OPTIONS}, - * `unlistedVariablesAreDyn: true`), and the two error classes that reach the - * first slot from ordinary authored input fault only HERE: - * - * - a {@link SCOPE_ROOTS} member -- or an object field sharing one of those - * names (`data`, `config`, `result`, `item`, `event`, `input`, `user`, …) -- - * as the operand of an operator with no `map` overload, because this env - * declares those roots `map` while the permissive one leaves them `dyn`; - * - a CEL TYPE name (`type`, `string`, `int`, …) in the same position, already - * pinned as a blind spot by `@objectstack/lint`'s `visibility-bare-identifier` - * suite -- pinned there per NAME, while the masking it causes is source-wide. - * - * ⛔ Do not close this by widening the regex onto the overload message: that - * false positive is precisely what the narrowing buys off (`type(record.x) == - * string` is legitimate CEL). Reporting past the first error needs a re-check - * loop over a neutralised source, or a checker entry that returns more than one - * error -- cel-js 8.0.0 has none, its `TypeCheckResult` carries a single - * `error` -- and either one changes what every consuming rule reports. That is - * a design decision, not a patch. + * `unlistedVariablesAreDyn: true`), so a source can compile clean and still + * fault HERE. + * + * ## What is CLOSED, and what is still open (#16412) + * + * ⭐ CLOSED — the {@link SCOPE_ROOTS} class. It was by far the wider of the two + * reachable ones: a root, or an object field sharing a root's name (`data`, + * `config`, `result`, `item`, `event`, `input`, `user`, …), used as the operand + * of an operator with no `map` overload. {@link buildScopedEnv} declared those + * roots `map` while the permissive env left them `dyn`, and that DISAGREEMENT + * was the whole mechanism; the roots are now `dyn` in both, so the class cannot + * arise. Measured on this env, before → after: + * + * data == 'x' && status == 'q' null -> "status" the class, closed + * status == 'q' && data == 'x' "status" unchanged, the control + * + * ⛔ STILL OPEN — every OTHER first-error class, and the mechanism above is + * untouched for them. Two are reachable from authored input: + * + * - a CEL TYPE name (`type`, `string`, `int`, …) in that same position. CEL + * itself declares those names, so no declaration this package controls can + * move them; `type == 'grid' && status == 'q'` still answers `null`. Pinned + * per NAME by `@objectstack/lint`'s `visibility-bare-identifier` suite, while + * the masking it causes is source-wide. + * - `has()` handed a non-select argument (`has(status) && other == 'x'`), which + * faults `has() invalid argument`. `@objectstack/lint`'s + * `validate-visibility-predicates` masks `has(…)` spans at its own call site + * (#16118) and that mask stays load-bearing; no other consumer has one. + * + * ⇒ A `null` from this helper is still "nothing was reported", never "the + * source is clean". ⛔ Do not close the remaining classes by widening the regex + * onto the overload message: that false positive is precisely what the + * narrowing buys off (`type(record.x) == string` is legitimate CEL). Reporting + * past the first error needs a re-check loop over a neutralised source, or a + * checker entry that returns more than one error -- cel-js 8.0.0 has none, its + * `TypeCheckResult` carries a single `error` -- and either one changes what + * every consuming rule reports. That is a design decision, not a patch. */ export function firstUndeclaredReference( source: string, @@ -598,10 +640,10 @@ export function parseCelToAstWithReason( * expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`, * `'google.protobuf.Timestamp'`, `'dyn'`, …) — or `null` when the expression does * not type-check. Reuses the SAME record-scoped, stdlib-registered env as - * {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) are - * declared `map` and `knownFields` are declared `dyn`, so both `record.` - * and bare `` references resolve while every stdlib call carries its - * declared return type. + * {@link firstUndeclaredReference}: namespace roots (`record`, `previous`, …) and + * `knownFields` are both declared `dyn`, so both `record.` and bare + * `` references resolve while every stdlib call carries its declared + * return type. * * Deliberately conservative. A member access (`record.amount`) or a bare field is * `dyn`, and an operator over two `dyn` operands stays `dyn` (cel-js cannot prove diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index c651659b57..3ce820f10b 100644 --- a/packages/formula/src/validate.test.ts +++ b/packages/formula/src/validate.test.ts @@ -7,7 +7,7 @@ import { nearestName, CEL_STDLIB_FUNCTIONS, } from './validate'; -import { buildEnv, firstUndeclaredReference } from './cel-engine'; +import { buildEnv, firstUndeclaredReference, SCOPE_ROOTS } from './cel-engine'; describe('validateExpression (ADR-0032)', () => { describe('predicates (CEL)', () => { @@ -538,6 +538,141 @@ describe('validateExpression (ADR-0032)', () => { it('does not flag a null-guard on a record-qualified field (no type false-positive)', () => { expect(validateExpression('predicate', 'record.lead_score != null && record.lead_score > 100', { scope: 'record' }).ok).toBe(true); }); + + /** + * ── #16412: a bare ref BEHIND a `SCOPE_ROOTS` name is judged ─────── + * + * `firstUndeclaredReference` reads the ONE error cel-js's checker returns, + * and acts only on `Unknown variable: X`. The strict env used to declare + * `SCOPE_ROOTS` as `map` while the permissive compile env leaves the same + * names `dyn`, so `data == 'x'` — a root, or an object field sharing a + * root's name — compiled clean and then faulted `no such overload: + * map == string` HERE. That fault took the single error slot and + * every bare reference behind it in the same source went unjudged: the + * helper answered `null`, which is also the value meaning "every reference + * is rooted", and this hard error was downgraded to silence. + * + * The roots are now `dyn` in both environments, so the class cannot arise. + * These pin the VERDICT, not the mechanism: they go red if the strict env's + * root declaration regresses to `map`, whatever the message then says. + */ + describe('a bare ref behind a `SCOPE_ROOTS` name is still an error (#16412)', () => { + it.each([ + ['data', "data == 'x' && status == 'qualified'"], + ['config', "config != null && status == 'qualified'"], + ['record', "record == 'x' && status == 'qualified'"], + ['result', "result > 1 && status == 'qualified'"], + ['item', "item == 'x' && status == 'qualified'"], + ])('%s as the FIRST operand does not mask `status`', (_root, source) => { + const r = validateExpression('predicate', source, { scope: 'record' }); + expect(r.ok).toBe(false); + expect(r.errors).toHaveLength(1); + expect(r.errors[0].message).toMatch(/bare reference `status`/); + expect(r.errors[0].message).toMatch(/record\.status/); + }); + + it('gives the same verdict in both operand orders — the masking was POSITIONAL', () => { + // The pre-fix asymmetry, and the whole reason the defect was invisible: + // one order reported, the other published clean. Both must report now, + // and the second is the control that was already correct. + const masked = validateExpression('predicate', "data == 'x' && status == 'qualified'", { scope: 'record' }); + const control = validateExpression('predicate', "status == 'qualified' && data == 'x'", { scope: 'record' }); + expect(masked.ok).toBe(false); + expect(control.ok).toBe(false); + expect(masked.errors[0].message).toBe(control.errors[0].message); + }); + + it('reports the BARE name, never the root that used to mask it', () => { + const r = validateExpression('predicate', "config != null && status == 'qualified'", { scope: 'record' }); + expect(r.errors[0].message).not.toMatch(/bare reference `config`/); + expect(r.errors[0].message).toMatch(/bare reference `status`/); + }); + + it('the flattened did-you-mean reaches a typo behind a root name too', () => { + // The same masking, one severity down: `validateExpression`'s flattened + // arm never saw the typo, so a misspelled field shipped unwarned. + const r = validateExpression('predicate', "config == 'y' && amont == 'x'", { + objectName: 'crm_opportunity', fields: ['amount', 'status'], scope: 'flattened', + }); + expect(r.ok).toBe(true); + expect(r.warnings).toHaveLength(1); + expect(r.warnings[0].message).toMatch(/`amont` is not a field/); + expect(r.warnings[0].message).toMatch(/did you mean `amount`/); + }); + + /** + * ⛔ The narrowing this helper is built on is NOT relaxed. Every source + * here has every reference rooted, or is legitimate CEL the overload + * message cannot be told apart from — none may start reporting. This is + * the property a widening of the regex onto the overload message would + * destroy, and it is the one most easily broken while closing a false + * negative. + */ + it.each([ + ['record.status == "x"', 'the canonical dotted spelling'], + ['type(record.x) == string', 'legitimate CEL an overload-message widening would reject'], + ['record.a + record.b > 1', 'arithmetic over two rooted members'], + ['previous.status != record.status', 'two roots in one source'], + ['parent.type == "grid" && record.status == "x"', 'a declared root, then a rooted term'], + ['current_user.id != null', 'the ADR-0068 canonical user root'], + ["record.tags.all(t, t != '')", 'a comprehension macro'], + ['record.lines.exists(status, status.ok)', 'a macro variable shadowing a field name'], + ['size(record.tags) > 0', 'a cel-js built-in'], + ['record.?name.orValue("x") == "x"', 'optional chaining'], + ])('%s stays clean (%s)', (source) => { + const r = validateExpression('predicate', source, { scope: 'record' }); + expect(r.ok).toBe(true); + expect(r.errors).toEqual([]); + }); + }); + + /** + * ⛔ #16412 closes the `SCOPE_ROOTS` class ONLY. Every other first-error + * class still masks what is behind it, and pinning that keeps the next + * reader from mistaking a narrowed fix for a general one. + * + * - a CEL TYPE name (`type`, `string`, `int`, …) is declared by CEL itself, + * so no declaration this package makes can reach it. Measured on the + * strict env: `type == 'grid' && …` faults `no such overload: type == + * string` under BOTH a `map` and a `dyn` root declaration — byte-identical + * messages — which is why this row is out of the fix's reach by + * construction rather than by omission. + * - `has()` handed a non-select argument faults `has() invalid argument`. + * `@objectstack/lint`'s visibility rule masks `has(…)` spans at its own + * call site (#16118); nothing here does. + */ + describe('the first-error classes #16412 does NOT close', () => { + it.each([ + ["type == 'grid' && status == 'qualified'", 'a CEL type name — CEL declares it, not `SCOPE_ROOTS`'], + ["string == 'x' && status == 'qualified'", 'the same class, another type name'], + ['has(status) && other == "x"', 'the `has()` class — #16118 masks this at the lint call site'], + ])('%s is still masked (%s)', (source) => { + expect(firstUndeclaredReference(source)).toBeNull(); + }); + + it('`type` is not a `SCOPE_ROOTS` member — read from the list, not copied', () => { + // Reads the published baseline so the claim above cannot go stale + // silently if the list ever gains the name. + expect(SCOPE_ROOTS as readonly string[]).not.toContain('type'); + }); + + /** + * #16412's own five-row probe table, pinned whole rather than by its one + * interesting row. Rows 1 and 5 are what the helper already got right and + * must keep; rows 2 and 3 are the `has()` class, unchanged by this fix; + * row 4 is the positional control that made the defect legible (rows 3 + * and 4 are the same two sub-expressions in the other order). + */ + it.each([ + ['status == "qualified"', 'status'], + ['has(status) && status == "qualified"', null], + ['has(status) && other == "x"', null], + ['other == "x" && has(status)', 'other'], + ['has(record.status) && status == "qualified"', 'status'], + ])('%s -> %s', (source, expected) => { + expect(firstUndeclaredReference(source)).toBe(expected); + }); + }); }); // #1928 tier 3 — flattened flow conditions reference fields bare, so a bare @@ -852,4 +987,49 @@ describe('inferExpressionType — coarse value-type of a formula', () => { expect(inferExpressionType('no_such_fn(amount)', { fields })).toBe('unknown'); // no overload expect(inferExpressionType('undeclared_field + 1')).toBe('unknown'); // bare ref, no fields given }); + + // ## The #16412 widening, pinned on BOTH sides + // + // `inferCelType` shares `buildScopedEnv` with `firstUndeclaredReference`, so + // #16412's `SCOPE_ROOTS` `map` -> `dyn` move lands on THIS surface too — and + // this one is published: `inferExpressionType` is re-exported from the package + // root and read by `@objectstack/mcp` as `validate_expression.inferredType`. + // `map` had no `==` / `<` / `+` overload, so a root used as a DIRECT OPERAND + // failed to type-check at all and every such expression answered `'unknown'`; + // `dyn` propagates, so the answer is now the truthful CEL type. That is a + // WIDENING of a published answer — more expressions get a concrete type, none + // changes from one concrete type to another — and it is pinned here so it + // cannot drift back or drift further unnoticed. + it('types a namespace root used as a direct operand (#16412 widening)', () => { + // Every one of these answered `'unknown'` while the roots were `map`. + expect(inferExpressionType('result + 1')).toBe('number'); + expect(inferExpressionType('record == "x"')).toBe('boolean'); + expect(inferExpressionType('data == "x" ? "a" : "b"')).toBe('text'); + expect(inferExpressionType('record > 1')).toBe('boolean'); + expect(inferExpressionType('record ? 1 : 2')).toBe('number'); + // It is a property of the DECLARATION, not of any one name, so it is uniform + // across the published list — sampled across its distinct provenance groups. + for (const root of ['config', 'item', 'event', 'input', 'user', 'parent', 'current_user']) { + expect(inferExpressionType(`${root} + 1`)).toBe('number'); + } + }); + + it('moved nothing the root declaration does not govern (#16412 controls)', () => { + // A root that is only the BASE of a member access never consults the changed + // declaration: `record.amount` is `dyn` under `map` and under `dyn` alike. + expect(inferExpressionType('record.amount > 100')).toBe('boolean'); + expect(inferExpressionType('record.amount + 1')).toBe('number'); + // The other direction, which a widening-only reading would miss: the + // overloads `map` DID carry must not have narrowed to `unknown`. + expect(inferExpressionType('size(record)')).toBe('number'); + expect(inferExpressionType('"a" in record')).toBe('boolean'); + expect(inferExpressionType('has(record.a)')).toBe('boolean'); + // `dyn` is undeclared-identifier-neutral — it changes what is legal ON a + // declared root, never whether an UNdeclared name faults. This is the + // property the whole #16412 change rests on, asserted on this surface. + expect(inferExpressionType('undeclared_field + 1')).toBe('unknown'); + expect(inferExpressionType('undeclared_field == "x"')).toBe('unknown'); + // And a root alone is still not a provable type: `dyn` maps to `'unknown'`. + expect(inferExpressionType('record')).toBe('unknown'); + }); }); diff --git a/packages/lint/src/flow-variable-scope.test.ts b/packages/lint/src/flow-variable-scope.test.ts index 7a027f9083..896fcf6b5d 100644 --- a/packages/lint/src/flow-variable-scope.test.ts +++ b/packages/lint/src/flow-variable-scope.test.ts @@ -183,6 +183,64 @@ describe('shadowedFieldReads (#14089)', () => { expect(SCOPE_ROOTS.length).toBeGreaterThan(0); expect(shadowedFieldReads(`${root} == "x"`, vars(root), [root])).toEqual([]); }); + + /** + * ── #16412: the blind spot above is name-local again, as written ──── + * + * It used to be far wider than its own wording, and that is what #16412 + * measured. `SCOPE_ROOTS` were declared `map` in the strict env, so a root + * name — or a field/variable sharing one — used with an ordinary operator + * faulted `no such overload` rather than `Unknown variable`. cel-js returns + * ONE error, so that fault took the slot, `bareRootsOf`'s loop got `null` on + * iteration 0 and terminated before judging anything, and EVERY shadow in + * that source was lost whatever it was named. The source still compiled (the + * permissive env leaves those roots `dyn`), so no sibling diagnostic fired + * either. + * + * The roots are `dyn` in both environments now. What survives is exactly what + * the pin above says: the colliding NAME itself is still not reported. + */ + it.each([ + ['config', "config == 'x' && status == 'y'"], + ['data', "data == 'x' && status == 'y'"], + ['result', "result > 1 && status == 'y'"], + ['item', "item == 'x' && status == 'y'"], + ])('a %s-named first operand no longer swallows every other shadow in the source', (root, source) => { + expect(SCOPE_ROOTS as readonly string[]).toContain(root); + expect(shadowedFieldReads(source, vars('status', root), ['status', root])).toEqual(['status']); + }); + + it('the loop reaches shadows in either order — the masking was POSITIONAL', () => { + // Same two sub-expressions, both orders. The second was already correct + // before #16412 and is the control: it is what made the first legible. + const masked = shadowedFieldReads("data == 'x' && status == 'y'", vars('status', 'data'), ['status', 'data']); + const control = shadowedFieldReads("status == 'y' && data == 'x'", vars('status', 'data'), ['status', 'data']); + expect(masked).toEqual(control); + expect(masked).toEqual(['status']); + }); + + it('still collects EVERY shadow behind a root name, not just the first', () => { + const found = shadowedFieldReads( + "config == 'x' && status == 'y' && amount > 1", + vars('status', 'amount', 'config'), + ['status', 'amount', 'config'], + ); + expect(found.sort()).toEqual(['amount', 'status']); + }); + + /** + * ⛔ The under-report direction is preserved: closing the `SCOPE_ROOTS` class + * must not start reporting anything a rooted or macro-bound source contains. + * These are the false positives the pinned oracle exists to avoid. + */ + it.each([ + ['record.status == "x"', 'the dotted spelling this rule prescribes'], + ['record.lines.exists(status, status.ok)', 'a comprehension-macro variable sharing a field name'], + ['size(record.lines) > 0', 'a function name sharing a field name'], + ['data.status == "x" && record.status == "y"', 'a root used as a NAMESPACE, which is correct'], + ])('%s reports nothing (%s)', (source) => { + expect(shadowedFieldReads(source, vars('status', 'size', 'data'), ['status', 'size', 'data'])).toEqual([]); + }); }); describe('shadowedFieldMessage (#14089)', () => { diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts index 948eaee28f..431b2463f0 100644 --- a/packages/lint/src/flow-variable-scope.ts +++ b/packages/lint/src/flow-variable-scope.ts @@ -275,17 +275,30 @@ const MAX_BARE_ROOTS = 64; * consulting the AST, which is what re-opens the macro-variable false positive. * * ⛔ That blind spot is NOT confined to the colliding name, and reading it as - * name-local understates it (#16412). Those roots are declared `map`, so using - * one as the operand of an operator with no `map` overload makes the checker's - * FIRST error a `no such overload` rather than an `Unknown variable` — the - * oracle returns `null` on iteration 0 and this loop terminates before it has - * judged anything. Every shadow in that source is then lost, whatever it is - * named, and the source still compiles (the permissive env leaves those roots - * `dyn`, so no sibling diagnostic fires either). Measured, `status` and - * `config` both declared variables and both fields: + * name-local understates it — but the WIDEST way it escaped the name is closed + * (#16412). Those roots used to be declared `map` in the oracle's strict env + * while the permissive env left them `dyn`, so using one as the operand of an + * operator with no `map` overload made the checker's FIRST error a `no such + * overload` rather than an `Unknown variable`: the oracle returned `null` on + * iteration 0 and this loop terminated before it had judged anything, losing + * every shadow in that source whatever it was named. The strict env now + * declares those roots `dyn` as well, so the two envs agree and that class + * cannot arise. Measured, `status` and `config` both declared variables and + * both fields: * - * config == 'x' && status == 'y' -> [] `status` LOST - * status == 'y' && config == 'x' -> ['status'] same names, other order + * config == 'x' && status == 'y' -> ['status'] the class, closed + * status == 'y' && config == 'x' -> ['status'] unchanged, the control + * + * ⚠️ The MECHANISM is untouched for every OTHER first-error class, so a `null` + * from the oracle is still "nothing was reported" and never "the source is + * clean", and this loop still terminates on it. Two such classes are reachable + * from authored input, and each still loses every shadow in the source whatever + * it is named — a CEL TYPE name in that same operand position, which CEL + * declares itself so no declaration this package makes can move it, and `has()` + * handed a non-select argument: + * + * type == 'grid' && status == 'y' -> [] `status` still LOST + * has(status) && other == 'x' -> [] `other` still LOST * * The masking is positional, so the loop's own upper bound is not what limits * it. See {@link firstUndeclaredReference}'s false-negative section for the diff --git a/packages/lint/src/validate-visibility-predicates.test.ts b/packages/lint/src/validate-visibility-predicates.test.ts index b5c5681c30..bd6e224603 100644 --- a/packages/lint/src/validate-visibility-predicates.test.ts +++ b/packages/lint/src/validate-visibility-predicates.test.ts @@ -674,6 +674,86 @@ describe('visibility-bare-identifier (#6128 / #5149 requirement 3)', () => { expect(bareFindings(formStack("record.type == 'grid'"))).toEqual([]); }); + /** + * ── #16412: the CEL-type blind spot is NAME-local, and now really is ── + * + * The pin above describes a missed catch on the name it pins. It used to + * cost far more than that, and the same was true of every `SCOPE_ROOTS` + * name: those roots were declared `map` in the strict env while the + * permissive compile env leaves them `dyn`, so an ordinary comparison on one + * faulted `no such overload` HERE and compiled clean THERE. cel-js returns + * exactly ONE error, so that fault took the slot and every bare identifier + * behind it in the same predicate went unjudged — the rule published the + * predicate clean, and per its own message the console then falls OPEN. + * + * The roots are `dyn` in both environments now, which closes the + * `SCOPE_ROOTS` half. ⛔ The CEL-type half above is NOT closed and cannot be + * from here: `type` is not a `SCOPE_ROOTS` member, so no declaration + * `@objectstack/formula` makes reaches it (measured: the strict env's message + * for `type == 'grid'` is byte-identical under a `map` and a `dyn` root + * declaration). The row below pins that remaining cost so it stays visible. + */ + describe('a bare identifier behind a namespace-root name is reported (#16412)', () => { + it.each([ + ['data', "data == 'x' && status == 'active'"], + ['record', "record == 'x' && status == 'active'"], + ['config', "config != null && status == 'active'"], + ['result', "result > 1 && status == 'active'"], + ])('%s as the first operand no longer masks `status`', (_root, predicate) => { + const findings = bareFindings(formStack(predicate)); + expect(findings).toHaveLength(1); + expect(findings[0].message).toContain('`status`'); + expect(findings[0].hint).toContain('`record.status`'); + }); + + it('names the BARE identifier, never the root that used to mask it', () => { + const findings = bareFindings(formStack("config != null && status == 'active'")); + expect(findings[0].message).not.toContain('`config`'); + }); + + it('emits exactly ONE finding — no double report on a predicate that compiles', () => { + // The predicate type-checks in the permissive env, so no sibling rule + // fires beside this one; the author gets a single verdict, not two. + expect(validateVisibilityPredicates(formStack("data == 'x' && status == 'active'")).map((f) => f.rule)) + .toEqual([VISIBILITY_BARE_IDENTIFIER]); + }); + + it('⛔ the CEL-type half of the masking stays open — pinned, not fixed', () => { + // `type` is CEL's own declaration, out of reach of `SCOPE_ROOTS`. This is + // the one row of #16412's twelve that its fix does not flip, and it is + // recorded here rather than left to be rediscovered. + expect(bareFindings(formStack("type == 'grid' && status == 'active'"))).toEqual([]); + // The control that proves the sentence above is about `type` and not + // about the shape: same shape, a namespace root in the first operand. + expect(bareFindings(formStack("data == 'grid' && status == 'active'"))).toHaveLength(1); + }); + + it('⛔ the `has()` half stays closed by #16118 s call-site mask, not by this', () => { + // The mask is what makes these report; the oracle alone still answers + // `null` for a `has()` first error. If the mask is ever removed these go + // silent again, which is the whole reason it is load-bearing. + expect(bareFindings(formStack("has(status) && other == 'x'"))).toHaveLength(1); + expect(bareFindings(formStack('has(record.status)'))).toEqual([]); + }); + + /** + * ⛔ The narrowing is not relaxed anywhere: every predicate here has each + * reference rooted, or is legitimate CEL, and none may start reporting. + */ + it.each([ + ['record.status == "active"', 'the canonical dotted spelling'], + ['data.status == "x"', 'a root used as a NAMESPACE, which is what roots are for'], + ['type(record.x) == string', 'the legitimate CEL the blind-spot pin protects'], + ['previous.status != record.status', 'two roots in one predicate'], + ['parent.type == "grid" && record.status == "x"', 'a declared root, then a rooted term'], + ["record.tags.all(t, t != '')", 'a comprehension macro'], + ['size(record.tags) > 0', 'a cel-js built-in'], + ['record.?name.orValue("x") == "x"', 'optional chaining'], + ])('%s produces no bare-identifier finding (%s)', (predicate) => { + expect(bareFindings(formStack(predicate))).toEqual([]); + }); + }); + // ── The #4953 boundary, pinned rather than described ──────────────── // // #4953 measured the SAME evaluator giving opposite verdicts on a total vs a