Skip to content

Commit 58f3220

Browse files
baozhoutaoclaude
andauthored
refactor(formula,lint): parseCelToAst 成为唯一的 CEL 解析入口 (#4812) (#6130)
lint 的 null-guard 闸门此前自建 cel-js Environment,而该 env 不带 limits: 它会解析、并进而判定 celEngine.compile() 直接以 Exceeded maxAstNodes (256) / maxDepth (32) / maxListElements (64) 拒绝的谓词。两个解析入口对「什么能解析」 给出两个答案,而这个闸门握着更宽松的那一个。 formula 新增 parseCelToAst(source): CelAstNode | null —— 与 compile/evaluate/ collectCelRootIdentifiers 共用同一条前端链路(#3306 rewriteNullableTernary 重写 + DEFAULT_LIMITS + 注册 stdlib 的 env)。只做 parse 不做 check:解析成功 但类型检查失败的表达式仍拿到 AST,类型裁决仍归 compile()。一并 re-export CelAstNode,补上 lowerCelAst 一直接收却从未导出的类型 —— 那正是消费方越过本包 直连 cel-js 的成因。 lint 改走该入口,并从 deps 移除 @marcbachmann/cel-js(import 与 package.json 双清,pnpm 侧 symlink 随之消失)。超界表达式不再由本闸门二次判定,交还给同一批 调用点上本就在跑的 validateExpression。 注:issue 正文所设想的洞(formula 会重写、裸 cel-js 解析不了 → lint 静默逃过) 经实测不成立,且不可构造 —— rewriteNullableTernary 先 parse,失败即原样返回, 故它永远无法把「解析不了」变成「解析得了」。真实分歧方向相反,且在 bounds 上。 Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We Co-authored-by: Claude <noreply@anthropic.com>
1 parent ce9c185 commit 58f3220

9 files changed

Lines changed: 475 additions & 25 deletions
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
'@objectstack/formula': minor
3+
---
4+
5+
新增规范 parse-to-AST 入口 `parseCelToAst(source)`,并 re-export AST 节点类型 `CelAstNode`(#4812)。
6+
7+
`parseCelToAst``compile` / `evaluate` / `collectCelRootIdentifiers` 共用同一条前端链路
8+
——#3306`rewriteNullableTernary` 重写、`DEFAULT_LIMITS` 边界、以及注册了 stdlib 的
9+
`unlistedVariablesAreDyn: true` 环境 —— 因此全仓对「什么能解析」只有一个答案。此前消费方
10+
若自建 `new Environment(...)`,拿到的是一份**不带 limits** 的答案:它会解析、并进而推理
11+
`compile()` 直接拒绝的表达式。
12+
13+
`parseCelToAst` 只做 parse,不做 check(后者是 `compile()` 的职责):解析成功但类型检查失败的
14+
表达式(大量 `dyn` 操作数的谓词即是)仍然会拿到 AST。解析失败返回 `null` 而不抛错。
15+
16+
`CelAstNode` 的 re-export 补上了一个既有缺口:`lowerCelAst` 一直接收 cel-js 的 `ASTNode`,
17+
而该类型从未导出,消费方只能越过本包直接依赖 `@marcbachmann/cel-js` —— 这正是第二个解析入口
18+
的成因。
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
'@objectstack/lint': patch
3+
---
4+
5+
null-guard 闸门改走 `@objectstack/formula` 的规范解析入口,并移除对 `@marcbachmann/cel-js`
6+
的直接依赖(#4812)。
7+
8+
`validate-null-guards.ts` 此前自建了一个**不带 limits** 的 cel-js `Environment`,于是它会解析、
9+
并进而判定平台自身拒绝的谓词 —— 超过 `maxAstNodes` (256) / `maxDepth` (32) /
10+
`maxListElements` (64) 的表达式在 lint 侧照常出 finding,在 `compile()` 侧却是
11+
`Exceeded max…`。两个解析入口,两个答案,而这个闸门握着更宽松的那个。
12+
13+
改走 `parseCelToAst` 后两者合一。超界表达式不再由本闸门二次判定,而是交还给同一批调用点上
14+
本就在跑的 `validateExpression` —— 它以 blocking error 报告边界错误,措辞面向自纠;作者修好
15+
边界问题后,null-guard 判定自然回来。规则判定本身没有变化:#3306 的三元重写对本 pass 是
16+
verdict-neutral(重写仅在三元的某一支恰为 `null` 字面量时触发,而该支本就证明不出任何
17+
guard),已加测试钉住。

packages/formula/src/cel-engine.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
*/
1515

1616
import { Environment, serialize } from '@marcbachmann/cel-js';
17+
import type { ASTNode } from '@marcbachmann/cel-js';
1718
import type { Expression } from '@objectstack/spec';
1819

1920
import { buildScope, registerNumericCoercions, registerStdLib } from './stdlib';
@@ -177,6 +178,78 @@ export function collectCelRootIdentifiers(
177178
}
178179
}
179180

181+
/**
182+
* A parsed CEL AST node, re-exported so a consumer can name the type this
183+
* package already hands it without importing `@marcbachmann/cel-js` itself.
184+
*
185+
* The alias is not cosmetic. {@link lowerCelAst} has always *taken* a cel-js
186+
* `ASTNode` while the type stayed unexported, so every caller that wanted to
187+
* hold an AST had to reach past this package to the parser — which is precisely
188+
* how a second, differently-configured parse entry gets built (#4812). Prefixed
189+
* `Cel` to match the package's other CEL-domain public names
190+
* (`CelFilterCompileResult`, `collectCelRootIdentifiers`, `isPushdownableCel`);
191+
* bare `ASTNode` would be ambiguous in a package that also owns the cron and
192+
* template dialects.
193+
*/
194+
export type CelAstNode = ASTNode;
195+
196+
/**
197+
* The canonical parse env. Identical in configuration to the one
198+
* {@link celEngine.compile} builds per call — same `unlistedVariablesAreDyn`,
199+
* same `enableOptionalTypes`, same {@link DEFAULT_LIMITS}, same stdlib — and
200+
* built once because `parse` neither mutates the environment nor depends on the
201+
* `now()` it was given (the same reasoning `recordScopeEnv` already relies on).
202+
* The parity suite pins the equivalence against a freshly-built env, so this
203+
* memo cannot silently drift away from `compile`.
204+
*/
205+
let canonicalParseEnv: Environment | undefined;
206+
207+
/**
208+
* Parse a CEL source to its AST through the **canonical** front end — the one
209+
* answer in this repo to "what parses" (#4812).
210+
*
211+
* Every other entry point in this package (`compile`, `evaluate`,
212+
* {@link collectCelRootIdentifiers}) reaches the parser through the same three
213+
* things, and so does this one:
214+
*
215+
* 1. {@link rewriteNullableTernary} — the #3306 `cond ? value : null` rewrite,
216+
* so the AST a consumer analyses is the AST the runtime will execute, not
217+
* the shape the author happened to type;
218+
* 2. {@link DEFAULT_LIMITS} — the platform's bounds. A source over
219+
* `maxAstNodes` / `maxDepth` / `maxListElements` does **not** parse here,
220+
* because it does not parse anywhere else on the platform either;
221+
* 3. the registered stdlib and `unlistedVariablesAreDyn: true` env.
222+
*
223+
* A consumer that built its own `new Environment(...)` instead got a different
224+
* answer to (2) in particular — it would happily parse, and then reason about,
225+
* a predicate `compile()` rejects outright. That is not a hypothetical: it is
226+
* what `@objectstack/lint`'s null-guard pass did until #4812.
227+
*
228+
* Returns `null` — never throws — when the source is empty or does not parse,
229+
* so a caller whose job is *not* to adjudicate syntax can skip it in one line
230+
* and leave the verdict to the gate that owns it (`validateExpression`, which
231+
* reports both the syntax fault and the bounds fault with a message written for
232+
* self-correction).
233+
*
234+
* This is `parse` only, deliberately **not** `parse + check`: `compile()` is the
235+
* entry that also type-checks. A caller that wants the AST of an expression
236+
* which parses but does not type-check (a great many predicates over `dyn`
237+
* operands) must not be denied it, and a caller that wants the type verdict
238+
* should ask `compile()` for it. The parity suite pins both halves of that
239+
* asymmetry so neither side drifts.
240+
*/
241+
export function parseCelToAst(source: string): CelAstNode | null {
242+
if (typeof source !== 'string' || !source.trim()) return null;
243+
try {
244+
// A wall-clock-free `now()` — the stdlib is registered for parse-time shape
245+
// only and is never called on this path.
246+
canonicalParseEnv ??= buildEnv(() => new Date(0));
247+
return canonicalParseEnv.parse(rewriteNullableTernary(source)).ast;
248+
} catch {
249+
return null;
250+
}
251+
}
252+
180253
/**
181254
* The result type cel-js's type-checker infers for a `value`/`predicate`
182255
* expression — its raw CEL type name (`'int'`, `'double'`, `'string'`, `'bool'`,

packages/formula/src/index.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ export { celEngine, DEFAULT_LIMITS } from './cel-engine';
1515
// (approval `expression` approvers): lint and the runtime pre-check share this
1616
// one helper so what they accept can never drift.
1717
export { collectCelRootIdentifiers } from './cel-engine';
18+
// #4812 — the canonical parse-to-AST entry. Any consumer that needs the AST of
19+
// an authored CEL source takes it from here, so "what parses" has exactly ONE
20+
// answer across build, lint and runtime. Building a private `new Environment()`
21+
// instead silently opts out of the platform's rewrite AND its bounds.
22+
export { parseCelToAst } from './cel-engine';
23+
export type { CelAstNode } from './cel-engine';
1824
export { cronEngine } from './cron-engine';
1925
export { templateEngine, TEMPLATE_FORMATTERS, formatValue } from './template-engine';
2026
export { registerStdLib, buildScope } from './stdlib';
Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// #4812 — `parseCelToAst` is the canonical parse-to-AST entry: the ONE answer in
4+
// this repo to "what parses". These tests pin it against `celEngine.compile`,
5+
// which is the entry the runtime actually uses, in both directions — what both
6+
// accept, what both reject, and the one asymmetry that is deliberate.
7+
//
8+
// They also pin the two things a consumer silently opts out of by building its
9+
// own `new Environment(...)` instead: the #3306 nullable-ternary rewrite, and
10+
// `DEFAULT_LIMITS`. `@objectstack/lint`'s null-guard pass did exactly that until
11+
// #4812 — and it was the *bounds* it diverged on, so a bare-env comparison is
12+
// asserted here rather than described.
13+
14+
import { Environment } from '@marcbachmann/cel-js';
15+
import { describe, expect, it } from 'vitest';
16+
17+
import { celEngine, parseCelToAst, DEFAULT_LIMITS } from './cel-engine';
18+
19+
/**
20+
* A bare cel-js environment — byte-for-byte the one `validate-null-guards.ts`
21+
* built for itself before #4812, and the shape any consumer naturally reaches
22+
* for. Its only difference from the canonical env is what it does NOT carry:
23+
* no `limits`, no stdlib, no rewrite.
24+
*/
25+
const bareEnv = new Environment({ unlistedVariablesAreDyn: true, enableOptionalTypes: true });
26+
const bareParses = (source: string): boolean => {
27+
try {
28+
bareEnv.parse(source);
29+
return true;
30+
} catch {
31+
return false;
32+
}
33+
};
34+
35+
/** Sources the platform accepts — drawn from the shapes this package's own suites use. */
36+
const ACCEPTED = [
37+
'record.amount > 1000',
38+
'record.end_date < record.start_date',
39+
'record.start_date != null && record.end_date != null && record.end_date < record.start_date',
40+
'has(record.start_date) && record.start_date < record.end_date',
41+
'"manager" in os.user.positions',
42+
'record.rating >= 4',
43+
'record.end_date <= daysFromNow(60)',
44+
'daysBetween(record.start_date, record.end_date) + 1',
45+
'record.budget == null || record.budget > 100',
46+
'!isBlank(record.owner) && record.owner != record.creator',
47+
'record.status == "open" ? record.amount * 0.1 : null',
48+
'true ? 5 : null',
49+
'size(record.items) > 0',
50+
'[1, 2, 3].all(x, x > 0)',
51+
'record.?owner.orValue("none") == "none"',
52+
];
53+
54+
/** Sources the platform rejects at PARSE — syntax faults, in both entries. */
55+
const SYNTAX_REJECTED = [
56+
'record.budget >',
57+
'record.a $$ 1',
58+
'record.a ?? 3', // cel-js 8 has no `??`
59+
'((record.a)',
60+
];
61+
62+
/**
63+
* The comparable content of a cel-js AST: `op` plus `args`, recursively.
64+
*
65+
* A raw deep-equal cannot be used here. `compile()` runs cel-js's `check()`,
66+
* which decorates every node IN PLACE with an entire evaluation plan — measured
67+
* on `record.amount > 1000`, the root gains `left`, `right`, `candidates`,
68+
* `handle` (a bound function), `rightStaticType` and `checkedType`, and
69+
* `candidates.registry` points back at the Environment, so the decorated tree is
70+
* circular. `parseCelToAst` is parse-only and carries none of it.
71+
*
72+
* `op` + `args` is also exactly the surface every AST consumer in this repo
73+
* walks (`lowerCelAst`, `collectCelRootIdentifiers`, lint's null-guard pass), so
74+
* equality on this projection is the claim that matters: the two entries hand a
75+
* consumer the same tree.
76+
*/
77+
function shapeOf(value: unknown): unknown {
78+
if (Array.isArray(value)) return value.map(shapeOf);
79+
if (value && typeof value === 'object' && typeof (value as { op?: unknown }).op === 'string') {
80+
return { op: (value as { op: string }).op, args: shapeOf((value as { args: unknown }).args) };
81+
}
82+
// Leaves: identifier / member / function-name strings and literal values
83+
// (including the BigInts cel-js produces for `int`).
84+
return value;
85+
}
86+
87+
/** Sources that parse but do NOT type-check — the deliberate asymmetry. */
88+
const PARSES_BUT_FAILS_CHECK = [
89+
'1 + "x"',
90+
'NOPE(record.a) > 1',
91+
'record.a.NOPE()',
92+
];
93+
94+
/** Over the platform's bounds — the divergence a bare env does not see. */
95+
const OVER_BOUNDS: Record<string, string> = {
96+
maxAstNodes: Array.from({ length: 300 }, (_, i) => `record.f${i}`).join(' + '),
97+
maxDepth: '('.repeat(60) + 'record.a' + ')'.repeat(60),
98+
maxListElements: `[${Array.from({ length: 200 }, (_, i) => i).join(',')}].size() > 0`,
99+
};
100+
101+
describe('parseCelToAst — parity with celEngine.compile (#4812)', () => {
102+
it.each(ACCEPTED)('returns the SAME ast compile() returns: %s', (source) => {
103+
const compiled = celEngine.compile(source);
104+
expect(compiled.ok).toBe(true);
105+
const ast = parseCelToAst(source);
106+
expect(ast).not.toBeNull();
107+
// `compile()` builds a FRESH env per call; `parseCelToAst` memoizes one.
108+
// Deep equality here is what pins that memo as equivalent — if the two ever
109+
// drift (a rewrite applied on one side, a limit on the other), this fails.
110+
expect(shapeOf(ast)).toEqual(
111+
shapeOf(compiled.ok ? compiled.value : undefined),
112+
);
113+
});
114+
115+
it.each(SYNTAX_REJECTED)('rejects exactly what compile() rejects at parse: %s', (source) => {
116+
// The parity claim is the accept/reject verdict itself. `compile`'s error
117+
// *classification* is asserted separately below — cel-js does not phrase
118+
// every syntax fault the same way, and `classifyError` reads the phrasing.
119+
expect(parseCelToAst(source)).toBeNull();
120+
expect(celEngine.compile(source).ok).toBe(false);
121+
});
122+
123+
it('yields a plain parse tree, while compile() yields a type-ANNOTATED one', () => {
124+
// The concrete difference between "parse" and "parse + check", pinned so the
125+
// two entries are not mistaken for interchangeable. A consumer walking
126+
// `.op`/`.args` sees the same tree from either; only `compile()` has run the
127+
// type checker over it.
128+
const source = 'record.amount > 1000';
129+
const compiled = celEngine.compile(source);
130+
expect(compiled.ok).toBe(true);
131+
expect(compiled.ok && (compiled.value as { checkedType?: unknown }).checkedType).toBeDefined();
132+
expect((parseCelToAst(source) as unknown as { checkedType?: unknown }).checkedType).toBeUndefined();
133+
});
134+
135+
it('classifies the common syntax fault as `parse`', () => {
136+
const compiled = celEngine.compile('record.budget >');
137+
expect(compiled.ok).toBe(false);
138+
if (!compiled.ok) expect(compiled.error.kind).toBe('parse');
139+
// NOT asserted for `((record.a)`: cel-js phrases an unbalanced delimiter as
140+
// `Expected RPAREN, got EOF`, which `classifyError`'s
141+
// /parse|unexpected|syntax/i does not match, so a genuine syntax fault is
142+
// reported to the author as `runtime`. Pre-existing, out of scope for #4812,
143+
// filed separately — asserting it here would enshrine it.
144+
});
145+
146+
it.each(PARSES_BUT_FAILS_CHECK)(
147+
'still yields an AST for a source that parses but fails check(): %s',
148+
(source) => {
149+
// The asymmetry is deliberate and load-bearing: `parseCelToAst` is parse
150+
// ONLY, `compile()` is parse + check. A consumer analysing an AST (the
151+
// null-guard pass, the pushdown compiler) must not be denied one just
152+
// because cel-js cannot type an expression over `dyn` operands — and a
153+
// caller who wants the type verdict asks `compile()`. Asserted so that
154+
// nobody "tightens" this entry into a second compile().
155+
expect(parseCelToAst(source)).not.toBeNull();
156+
const compiled = celEngine.compile(source);
157+
expect(compiled.ok).toBe(false);
158+
if (!compiled.ok) expect(compiled.error.kind).toBe('type');
159+
},
160+
);
161+
162+
it('never throws, and answers null for an empty source', () => {
163+
expect(parseCelToAst('')).toBeNull();
164+
expect(parseCelToAst(' ')).toBeNull();
165+
expect(parseCelToAst(undefined as unknown as string)).toBeNull();
166+
expect(parseCelToAst(null as unknown as string)).toBeNull();
167+
});
168+
});
169+
170+
describe('parseCelToAst — carries the #3306 nullable-ternary rewrite', () => {
171+
// `true ? 5 : null` is the specimen: it PARSES in any env, but cel-js's
172+
// ternary unifier rejects it at check ("Ternary branches must have the same
173+
// type, got 'int' and 'null'"), so without the rewrite the blessed
174+
// `guard ? value : null` shape does not compile. The rewrite wraps the
175+
// non-null branch in `dyn(...)`, which is what makes it legal — and it is the
176+
// AST the runtime executes. An entry that skipped the rewrite would hand a
177+
// consumer a DIFFERENT tree from the one the platform runs.
178+
it('wraps the non-null branch in dyn(...), matching what the runtime executes', () => {
179+
const ast = parseCelToAst('true ? 5 : null') as unknown as {
180+
op: string;
181+
args: [unknown, { op: string; args: [string, unknown[]] }, unknown];
182+
};
183+
expect(ast).not.toBeNull();
184+
expect(ast.op).toBe('?:');
185+
expect(ast.args[1].op).toBe('call');
186+
expect(ast.args[1].args[0]).toBe('dyn');
187+
});
188+
189+
it('agrees with compile() on the rewritten shape, which only compile() could evaluate', () => {
190+
const source = 'record.status == "open" ? record.amount * 0.1 : null';
191+
const compiled = celEngine.compile(source);
192+
expect(compiled.ok).toBe(true);
193+
expect(shapeOf(parseCelToAst(source))).toEqual(
194+
shapeOf(compiled.ok ? compiled.value : undefined),
195+
);
196+
});
197+
198+
it('cannot change WHETHER a source parses — only the AST it yields', () => {
199+
// Stated as a test because it is the fact that makes #4812's originally
200+
// suspected hole ("formula rewrites something bare cel-js cannot parse, so
201+
// lint silently skips it") impossible by construction: the rewrite parses
202+
// the source FIRST and returns it unchanged on failure. Every source below
203+
// therefore gets the same accept/reject verdict from both entries.
204+
for (const source of [...ACCEPTED, ...PARSES_BUT_FAILS_CHECK]) {
205+
expect(bareParses(source)).toBe(true);
206+
expect(parseCelToAst(source)).not.toBeNull();
207+
}
208+
for (const source of SYNTAX_REJECTED) {
209+
expect(bareParses(source)).toBe(false);
210+
expect(parseCelToAst(source)).toBeNull();
211+
}
212+
});
213+
});
214+
215+
describe('parseCelToAst — carries the platform bounds (the real #4812 divergence)', () => {
216+
it.each(Object.entries(OVER_BOUNDS))(
217+
'refuses a source over %s, which a bare env happily parses',
218+
(_limit, source) => {
219+
// This is the measured divergence, in the direction it actually runs: a
220+
// consumer with its own limitless env parses — and then reasons about — a
221+
// predicate the platform rejects outright. Both halves asserted, so the
222+
// test states the divergence rather than merely benefiting from its fix.
223+
expect(bareParses(source)).toBe(true);
224+
expect(parseCelToAst(source)).toBeNull();
225+
226+
const compiled = celEngine.compile(source);
227+
expect(compiled.ok).toBe(false);
228+
if (!compiled.ok) {
229+
expect(compiled.error.kind).toBe('bounds');
230+
expect(compiled.error.message).toMatch(/Exceeded max/i);
231+
}
232+
},
233+
);
234+
235+
it('pins the bounds the entry enforces to DEFAULT_LIMITS', () => {
236+
// If DEFAULT_LIMITS moves, the fixtures above must move with it; this
237+
// assertion is the tripwire that says so out loud.
238+
expect(DEFAULT_LIMITS.maxAstNodes).toBe(256);
239+
expect(DEFAULT_LIMITS.maxDepth).toBe(32);
240+
expect(DEFAULT_LIMITS.maxListElements).toBe(64);
241+
});
242+
});

packages/lint/package.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
"check:doc-formula-expressions": "node scripts/check-doc-formula-expressions.mjs --self-test && node scripts/check-doc-formula-expressions.mjs"
2727
},
2828
"dependencies": {
29-
"@marcbachmann/cel-js": "^8.0.0",
3029
"@objectstack/formula": "workspace:*",
3130
"@objectstack/sdui-parser": "workspace:*",
3231
"@objectstack/spec": "workspace:*",

0 commit comments

Comments
 (0)