Skip to content

Commit 86c75f4

Browse files
os-muskclaude
andauthored
docs(formula): state the false-negative side of firstUndeclaredReference (#16808)
`firstUndeclaredReference` documented only which error it cannot make ("Acts ONLY on cel-js's `Unknown variable: X` fault, so it cannot false-positive"). That sentence is true; a contract that declares only that reads as making neither error. The checker returns exactly one error, so a first error of another class leaves every undeclared reference behind it unjudged and the helper answers `null` -- the same value that means "every reference is rooted". The amendment states the masking is positional rather than name-keyed, that `celEngine.compile()` is not a gate against it (it type-checks in the permissive env, where the roots this env declares `map` are `dyn`), which two error classes reach that first slot from authored input, and why widening the regex is the false positive the narrowing buys off. `flow-variable-scope` carries the same correction from the consumer side: its "known, deliberate blind spot" note bounded the under-report to a variable named after a SCOPE_ROOTS member, and measured, that bound does not hold -- such a name in an operand position terminates the discovery loop on iteration 0 and every shadow in the source is lost. Comment-only; no behaviour changes. The formula block is JSDoc on a published export, so it is emitted into dist/index.d.ts. Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg Co-authored-by: Claude <noreply@anthropic.com>
1 parent 7f96e14 commit 86c75f4

3 files changed

Lines changed: 78 additions & 0 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
"@objectstack/formula": patch
3+
"@objectstack/lint": patch
4+
---
5+
6+
`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".
7+
8+
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.
9+
10+
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.
11+
12+
What the amendment adds, all of it measured rather than reasoned:
13+
14+
- **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"`.
15+
- **`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.
16+
- **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.
17+
- **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.
18+
19+
`@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.

packages/formula/src/cel-engine.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,48 @@ let recordScopeEnv: Environment | undefined;
181181
* on arithmetic/comparison overloads — and it must NOT be applied to flow /
182182
* automation conditions, where the record's fields ARE flattened to top-level
183183
* and bare references are correct.
184+
*
185+
* ## The false-NEGATIVE side of that narrowing (#16412)
186+
*
187+
* The paragraph above states which error this helper cannot make. It does not
188+
* state that it makes neither, and it does not: cel-js's checker hands back
189+
* exactly ONE error, so when the FIRST one is of another class every undeclared
190+
* reference behind it in the same source goes unjudged and the answer is
191+
* `null` -- the same value that means "every reference is rooted". A `null`
192+
* here is "nothing was reported", never "the source is clean", and a caller
193+
* that needs the stronger reading does not get it from this helper.
194+
*
195+
* The masking is POSITIONAL, not name-keyed: the masked name is not the one
196+
* that triggered the first error, so excluding the trigger's own name does not
197+
* reach it. Measured on this env:
198+
*
199+
* data == 'x' && status == 'q' -> null first error `no such
200+
* overload: map<dyn, dyn> ==
201+
* string`; `status` unjudged
202+
* status == 'q' && data == 'x' -> "status" first error `Unknown
203+
* variable: status`
204+
*
205+
* ⚠️ {@link celEngine.compile} is not a gate against this, so a caller that
206+
* only reaches here on a clean compile is not protected by that gate. `compile`
207+
* type-checks in the PERMISSIVE env ({@link CEL_ENV_OPTIONS},
208+
* `unlistedVariablesAreDyn: true`), and the two error classes that reach the
209+
* first slot from ordinary authored input fault only HERE:
210+
*
211+
* - a {@link SCOPE_ROOTS} member -- or an object field sharing one of those
212+
* names (`data`, `config`, `result`, `item`, `event`, `input`, `user`, …) --
213+
* as the operand of an operator with no `map` overload, because this env
214+
* declares those roots `map` while the permissive one leaves them `dyn`;
215+
* - a CEL TYPE name (`type`, `string`, `int`, …) in the same position, already
216+
* pinned as a blind spot by `@objectstack/lint`'s `visibility-bare-identifier`
217+
* suite -- pinned there per NAME, while the masking it causes is source-wide.
218+
*
219+
* ⛔ Do not close this by widening the regex onto the overload message: that
220+
* false positive is precisely what the narrowing buys off (`type(record.x) ==
221+
* string` is legitimate CEL). Reporting past the first error needs a re-check
222+
* loop over a neutralised source, or a checker entry that returns more than one
223+
* error -- cel-js 8.0.0 has none, its `TypeCheckResult` carries a single
224+
* `error` -- and either one changes what every consuming rule reports. That is
225+
* a design decision, not a patch.
184226
*/
185227
export function firstUndeclaredReference(
186228
source: string,

packages/lint/src/flow-variable-scope.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,23 @@ const MAX_BARE_ROOTS = 64;
266266
* shadow goes unwarned. That is an UNDER-report, the safe direction for a new
267267
* warning, and it is the price of the pinned oracle — closing it would mean
268268
* consulting the AST, which is what re-opens the macro-variable false positive.
269+
*
270+
* ⛔ That blind spot is NOT confined to the colliding name, and reading it as
271+
* name-local understates it (#16412). Those roots are declared `map`, so using
272+
* one as the operand of an operator with no `map` overload makes the checker's
273+
* FIRST error a `no such overload` rather than an `Unknown variable` — the
274+
* oracle returns `null` on iteration 0 and this loop terminates before it has
275+
* judged anything. Every shadow in that source is then lost, whatever it is
276+
* named, and the source still compiles (the permissive env leaves those roots
277+
* `dyn`, so no sibling diagnostic fires either). Measured, `status` and
278+
* `config` both declared variables and both fields:
279+
*
280+
* config == 'x' && status == 'y' -> [] `status` LOST
281+
* status == 'y' && config == 'x' -> ['status'] same names, other order
282+
*
283+
* The masking is positional, so the loop's own upper bound is not what limits
284+
* it. See {@link firstUndeclaredReference}'s false-negative section for the
285+
* mechanism and for why widening the oracle is a design decision, not a patch.
269286
*/
270287
function bareRootsOf(source: string): string[] {
271288
const found: string[] = [];

0 commit comments

Comments
 (0)