diff --git a/.changeset/first-undeclared-reference-false-negative-contract.md b/.changeset/first-undeclared-reference-false-negative-contract.md new file mode 100644 index 0000000000..f34be606c2 --- /dev/null +++ b/.changeset/first-undeclared-reference-false-negative-contract.md @@ -0,0 +1,19 @@ +--- +"@objectstack/formula": patch +"@objectstack/lint": patch +--- + +`firstUndeclaredReference` now documents the side of its contract it was silent about: it can false-NEGATIVE, and a `null` is "nothing was reported", not "every reference is rooted". + +The existing sentence — "Acts ONLY on cel-js's `Unknown variable: X` fault, so it cannot false-positive on arithmetic/comparison overloads" — is true, and stays. What it never said is what that narrowing costs. cel-js's checker returns exactly ONE error, so when the first one is of another class every undeclared reference behind it in the same source goes unjudged and the helper answers `null` — the same value that means the source is clean. A contract that declares only which error it cannot make reads as making neither. + +No behaviour changes. This is the contract text, and it ships: the amended block is JSDoc on a published export, so it is emitted into `@objectstack/formula`'s `dist/index.d.ts` and `dist/index.d.mts` (measured — the declaration file grew 53.45 KB to 55.99 KB) and is what a consumer reads on hover. + +What the amendment adds, all of it measured rather than reasoned: + +- **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. `data == 'x' && status == 'q'` answers `null`; the same two names in the other order answer `"status"`. +- **`celEngine.compile()` is not a gate against it.** `compile` type-checks in the permissive environment, where every unlisted name is `dyn`. The strict environment here declares `SCOPE_ROOTS` as `map`, so a root — or an object field sharing one of those names (`data`, `config`, `result`, `item`, `event`, `input`, `user`, …) — used as the operand of an operator with no `map` overload faults HERE and nowhere else. A caller that only reaches the helper on a clean compile is therefore not protected by its own gate. +- **The CEL type-name class is the same shape.** `type == 'grid'` is already pinned as a blind spot in `@objectstack/lint`'s `visibility-bare-identifier` suite, but pinned per NAME; the masking it causes is source-wide. +- **What closing it would take, and why that is not this change.** Widening the regex onto the overload message is the false positive 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 returning more than one error — cel-js 8.0.0 has neither; its `TypeCheckResult` carries a single `error`. Both change what every consuming rule reports, so the oracle's shape is a design decision. + +`@objectstack/lint` carries a second comment-only correction, to `flow-variable-scope`'s account of the same oracle. Its "known, deliberate blind spot" note bounded the under-report to a flow variable named after a `SCOPE_ROOTS` member; measured, the bound does not hold — such a name in an operand position terminates the discovery loop on iteration 0 and every shadow in that source is lost, whatever it is named. That block sits on an internal function, so unlike the `formula` half it reaches no published declaration file; the entry is here because the package is touched and published. diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index ee36cf434a..88f74c9c99 100644 --- a/packages/formula/src/cel-engine.ts +++ b/packages/formula/src/cel-engine.ts @@ -181,6 +181,48 @@ let recordScopeEnv: Environment | undefined; * on arithmetic/comparison overloads — and it must NOT be applied to flow / * automation conditions, where the record's fields ARE flattened to top-level * and bare references are correct. + * + * ## The false-NEGATIVE side of that narrowing (#16412) + * + * The paragraph above states which error this helper cannot make. It does not + * state that it makes neither, and it does not: cel-js's checker hands back + * exactly ONE error, so when the FIRST one is of another class every undeclared + * reference behind it in the same source goes unjudged and the answer is + * `null` -- the same value that means "every reference is rooted". A `null` + * here is "nothing was reported", never "the source is clean", and a caller + * that needs the stronger reading does not get it from this helper. + * + * 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` + * + * ⚠️ {@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. */ export function firstUndeclaredReference( source: string, diff --git a/packages/lint/src/flow-variable-scope.ts b/packages/lint/src/flow-variable-scope.ts index 2059fdbc90..2202963845 100644 --- a/packages/lint/src/flow-variable-scope.ts +++ b/packages/lint/src/flow-variable-scope.ts @@ -266,6 +266,23 @@ const MAX_BARE_ROOTS = 64; * shadow goes unwarned. That is an UNDER-report, the safe direction for a new * warning, and it is the price of the pinned oracle — closing it would mean * 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: + * + * config == 'x' && status == 'y' -> [] `status` LOST + * status == 'y' && config == 'x' -> ['status'] same names, other order + * + * 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 + * mechanism and for why widening the oracle is a design decision, not a patch. */ function bareRootsOf(source: string): string[] { const found: string[] = [];