Skip to content

Commit d2e7d6c

Browse files
fix(cli): a renamed destructuring key off ctx is a key, not a free identifier (#22489)
Fixes #22459 Clause-②: no The rule's published text negates this refusal: its own remedy line reads "Inline the value(s) into the handler, or reach them through `ctx`." (`packages/cli/src/lint/hook-body-lowering.ts:163`), and `const { previous: prev } = ctx` reaches the value through `ctx`. Removing a misrejection that the rule's text already denies is `no`. ## What changed The free-identifier scan behind `objectstack lint`'s `hook-body/not-lowerable` rule and `objectstack build`'s hook lowering (`packages/cli/src/utils/detect-free-identifiers.ts`) counted a renamed destructuring key as a value reference. For a hook that destructures a renamed key off `ctx`, such as `const { event, input, previous: prev } = ctx`, it reported `previous` as free. The binding walk (`collectBindings`) already bound the alias `prev`. The reference walk (`collectReferences`) then met the element's `propertyName` as a plain identifier. So lint refused a lowerable hook with advice describing what it already did, and the build bundled it instead of lowering it. `collectReferences` now handles a `BindingElement` the way it already handles a `PropertyAssignment`. A non-computed `propertyName` is a key, not a reference. The walk still visits a computed key's expression, the binding name (an alias is subtracted by `bindings`; a nested pattern recurses), and the default, because `{ [k]: v = d }` reads `k` and `d`. It is one branch in the existing walker, with no second walker. `extract-hook-body.ts`, the rule's message text and `packages/spec` are untouched. Changeset: `.changeset/22459-hook-lowering-destructuring-alias.md`, `@objectstack/cli` `patch`. The remedy is: none needed, because a hook written this way now lowers. ## Reproduction, red then green - **Red.** The new pins, run against the unmodified `collectReferences` (base `f66c440de`): `detect-free-identifiers.test.ts` has 8 failed and 37 passed (45). The failures read `expected [ 'previous' ] to deeply equal []` (the card's A shape, an object pattern inside an array pattern, a nested callback parameter, and the compiled `.toString()` shape), `[ 'a', 'b' ]` (nested), `[ 'key' ]` (defaulted alias), `[ 'message' ]` (catch clause), and `[ 'FALLBACK', 'key' ]` where the control expects `[ 'FALLBACK' ]`. - **Green** at `38fa4d085`: `detect-free-identifiers.test.ts` and `hook-body-lowering.test.ts` give 63 passed (63). - **Ablation** (one-shot, run from the committed fix). The new branch was neutralised on disk with `scripts/ablation-replace.mjs`: anchor hit 1 time and went from 1 to 0, and the blob went `b768875ce125` to `dcd006617c08`. Both suites then gave 9 failed and 54 passed (63). The ninth failure is the lint/build pin: `expected [ { severity: 'error', …(3) } ] to deeply equal []`. On restore, the blob matched HEAD (`b768875ce125`) and `git diff HEAD` was empty. Pins, as triage directed: - the card's A shape (`{ event, input, previous: prev } = ctx`) lowers; - nested (`{ a: { b: c } } = ctx`) and defaulted (`{ key: alias = d }`, `d` in scope) shapes lower, and so does a computed key with an in-scope default; - control: a defaulted alias whose default is free still reports `FALLBACK` (also the shorthand `{ previous = FALLBACK }`), and a free computed key still reports `KEY`; - control: a genuinely free `previous` (no `ctx` source) is still reported, at the helper and at lint/build level. ## H2 and H3 (measured) - **Array destructuring** (`const [first, second] = ctx.items`): no false positive before the fix. It was green on base, because an array element has no key. **Parameter destructuring**: the handler's own top-level parameters never had it, because the reference walk does not visit top-level parameter patterns (the existing `({ a: { b } }) => b + 1` pin was green on base). The same element shape inside the body DID have it: `ctx.items.map(({ previous: prev }) => prev.n)`, `const [{ previous: prev }] = ctx.items`, and `catch ({ message: msg })` were all red on base. The same rule covers them, and each is pinned. - **End to end** (`hook-body-lowering.test.ts`): for a hook that destructures a renamed key off `ctx`, `checkHookBodyLowering` returns `[]`. `lowerCallables` records no extraction warning, `bodyExtracted` is 1, and the lowered `body.source` carries `previous: prev`. The control hook reads a module-scope `previous`. It is still an `error` under `hook-body/not-lowerable` at `hooks[0].handler`, and the build records `['free-identifiers', ['previous']]` and lowers nothing. Under the ablation the positive pin went red; the control stayed green both ways. ## Verification, at `38fa4d085` - `pnpm --filter @objectstack/cli exec vitest run --project unit --maxWorkers=2`: 272 files passed. 2 files failed on a prerequisite, not a verdict ("packages/cli is not built (./dist/index.js is absent)"): `published-subpath-hook-body.pin` and `published-subpath-console.pin`. After `pnpm --filter @objectstack/cli build`, those two gave 29 passed (29). Totals: 274 of 274 files, 4023 + 29 tests passed, 29 skipped. The `integration` layer is declared to CI, because the diff touches no integration-layer file and no spawn entry. - `pnpm --filter @objectstack/cli typecheck`: exit 0. `tsc -p tsconfig.json --listFilesOnly` includes both edited test files (count 2). - Lint, as a proven narrowing rather than the full `pnpm lint`. (1) Population, read from `eslint.config.mjs` through the ESLint API: none of the 3 changed TS files is ignored. The changeset `.md` is outside every `files` glob. (2) Count, from `--format json`: 3 files, 0 errors, 0 warnings. (3) Invariance: the config enables no type-aware linting (each file's resolved `parserOptions` is `{ ecmaVersion, sourceType }` only, with no `project`), and the diff touches no ESLint config, plugin or baseline. So no untouched file's verdict can move. - Gates: `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands` (no paths) derives 64 commands; all 64 exit 0. Four of them (`check:dual-build-cjs-loads`, `check:i18n`, `check:i18n-coverage`, `check:i18n-walk-parity`) first exited 3, PREREQUISITE NOT MET, and exited 0 after the build. `--ran` reconciles: 64 derived, 64 run, 0 NOT-MEASURED, 0 UNRUN. ## Acceptance notes - **Observation, not filed (reach not measured).** Lowering drops the handler's own parameter list. The runner wraps a hook body as `(async (ctx) => { … })(ctx)`, and the scan treats the handler's top-level parameters as bound. Measured at `38fa4d085`, a pre-existing path this PR does not touch: `extractHookBody(({ input }) => { input.x = 1; })` lowers to the source `input.x=1`, and `(c) => { c.input.x = 1; }` lowers to `c.input.x=1`. `checkHookBodyLowering` reports 0 issues for both. Relatedly, the reference walk never visits a top-level parameter pattern's default: `detectFreeIdentifiers('({ x = FREE }) => x')` gives `[]`, while the in-body spelling gives `['FREE']`. NOT MEASURED: whether such a body throws when the runtime evaluates it. Carrier: none. --- _Generated by [Claude Code](https://claude.ai/code/session_01BmsuLyUeuG5CNpZFMH1jzS)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 35ef501 commit d2e7d6c

4 files changed

Lines changed: 147 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/cli': patch
3+
---
4+
5+
fix(cli): a hook that destructures a renamed key off `ctx` is no longer refused as not-lowerable (#22459)
6+
7+
Clause-②: no
8+
9+
- **What changed.** The free-identifier scan behind `objectstack lint`'s `hook-body/not-lowerable` rule and `objectstack build`'s hook lowering counted a renamed destructuring key as a free identifier. For `const { event, input, previous: prev } = ctx`, it reported `previous`, a property read off `ctx`, although the only name the pattern binds is `prev`. So `objectstack lint` failed the hook with the advice "reach them through `ctx`", which the hook already did, and `objectstack build` shipped it as a bundled closure instead of a metadata-only body. A non-computed destructuring key is now a key, as the key of `{ key: value }` already was. Nested patterns (`{ a: { b: c } } = ctx`), patterns inside array patterns, the parameters of a callback inside the handler, and `catch` clauses are covered by the same rule.
10+
- **What is still refused.** A destructuring default or a computed key that names something out of scope is still free: `const { key: alias = FALLBACK } = ctx` and `const { [KEY]: v } = ctx` report `FALLBACK` and `KEY`, as before. A genuinely free `previous` that is not read off `ctx` is still refused.
11+
- **What to do.** Nothing. A hook written this way now lowers to a body on the next `objectstack build`, and `objectstack lint` stops reporting it. The un-renamed spelling (`const prev = ctx.previous`) keeps working as before.

‎packages/cli/src/lint/hook-body-lowering.test.ts‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import { lowerCallables } from '../utils/lower-callables.js';
2222

2323
// Module scope — exactly what a lowered body cannot reach.
2424
const SLA_MATRIX: Record<string, number> = { high: 4, low: 48 };
25+
// Module scope, spelled like a `ctx` key — a reference to THIS is free.
26+
const previous = { rank: 1 };
2527

2628
const freeIdentifierHook = {
2729
name: 'case_sla',
@@ -139,6 +141,56 @@ describe('checkHookBodyLowering', () => {
139141
});
140142
});
141143

144+
describe('a hook that destructures a renamed key off `ctx` reaches its value through `ctx`', () => {
145+
// `previous` is a property READ OFF ctx — exactly what this rule's own
146+
// remedy prescribes — and `prev` is the only name the pattern binds. The
147+
// scan used to count the key as a free identifier, so lint refused the
148+
// hook (advising what it already did) and the build bundled it.
149+
const renamedKeyHook = {
150+
name: 'carry_rank',
151+
object: 'task',
152+
events: ['beforeInsert', 'beforeUpdate'],
153+
handler: (ctx: any) => {
154+
const { event, input, previous: prev } = ctx;
155+
if (event === 'beforeUpdate' && prev) input.rank = prev.rank;
156+
},
157+
};
158+
// The control: the same spelling, but read from module scope — not `ctx`.
159+
const freePreviousHook = {
160+
name: 'carry_rank_free',
161+
object: 'task',
162+
events: ['beforeUpdate'],
163+
handler: (ctx: any) => {
164+
const prev = previous;
165+
ctx.input.rank = prev.rank;
166+
},
167+
};
168+
169+
it('lint reports nothing, and the build lowers it to a body', () => {
170+
expect(checkHookBodyLowering({ hooks: [{ ...renamedKeyHook }] })).toEqual([]);
171+
172+
const lowering = lowerCallables({ hooks: [{ ...renamedKeyHook }] });
173+
expect(lowering.bodyExtractionWarnings).toEqual([]);
174+
expect(lowering.bodyExtracted).toBe(1);
175+
const [hook] = lowering.lowered.hooks as Array<{ body?: { source: string } }>;
176+
expect(hook.body?.source).toContain('previous: prev');
177+
});
178+
179+
it('a genuinely free `previous` is still refused, by both', () => {
180+
const issues = checkHookBodyLowering({ hooks: [{ ...freePreviousHook }] });
181+
expect(issues).toHaveLength(1);
182+
expect(issues[0].severity).toBe('error');
183+
expect(issues[0].rule).toBe(NOT_LOWERABLE_RULE);
184+
expect(issues[0].path).toBe('hooks[0].handler');
185+
186+
const lowering = lowerCallables({ hooks: [{ ...freePreviousHook }] });
187+
expect(lowering.bodyExtracted).toBe(0);
188+
expect(lowering.bodyExtractionWarnings.map((w) => [w.kind, w.freeIdentifiers])).toEqual([
189+
['free-identifiers', ['previous']],
190+
]);
191+
});
192+
});
193+
142194
it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => {
143195
const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] });
144196

‎packages/cli/src/utils/detect-free-identifiers.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,65 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
7575
}
7676
});
7777

78+
// A renamed destructuring key — `const { previous: prev } = ctx` — reads
79+
// `previous` OFF `ctx`; the only name it introduces is `prev`. The key is a
80+
// property name exactly like the `key` of `{ key: value }`, so counting it as
81+
// a free identifier refused a self-contained handler (and bundled it), with
82+
// advice — "reach them through `ctx`" — describing what it already did.
83+
describe('a destructuring KEY is not a reference; its alias is the binding', () => {
84+
const lowerable: Array<[string, string]> = [
85+
[
86+
'a renamed key off ctx, beside shorthand keys',
87+
"(ctx) => { const { event, input, previous: prev } = ctx; if (event === 'beforeUpdate' && prev) input.n = prev.n; }",
88+
],
89+
['a nested renamed pattern', '(ctx) => { const { a: { b: c } } = ctx; return c; }'],
90+
[
91+
'a defaulted alias whose default is in scope',
92+
'(ctx) => { const d = ctx.fallback; const { key: alias = d } = ctx; return alias; }',
93+
],
94+
[
95+
'a computed key and a default, both in scope',
96+
'(ctx) => { const k = ctx.k; const d = 0; const { [k]: v = d } = ctx; return v; }',
97+
],
98+
// The same element shape reached two other ways.
99+
['an object pattern inside an array pattern', '(ctx) => { const [{ previous: prev }] = ctx.items; return prev; }'],
100+
['a nested callback parameter', '(ctx) => ctx.items.map(({ previous: prev }) => prev.n)'],
101+
['a catch-clause pattern', '(ctx) => { try { ctx.run(); } catch ({ message: msg }) { ctx.log = msg; } }'],
102+
// No key at all: elements are bound by position, so nothing to mistake.
103+
['an array pattern', '(ctx) => { const [first, second] = ctx.items; return first + second; }'],
104+
];
105+
for (const [label, source] of lowerable) {
106+
it(label, () => {
107+
const r = detectFreeIdentifiers(source);
108+
expect(r.unparsed).toBe(false);
109+
expect(r.free).toEqual([]);
110+
});
111+
}
112+
113+
// The controls: what the element still READS is still judged. Only the key
114+
// stopped counting — a default or a computed key that names something out
115+
// of scope is as free as it ever was.
116+
it('a defaulted alias whose default is free is still reported', () => {
117+
const r = detectFreeIdentifiers('(ctx) => { const { key: alias = FALLBACK } = ctx; return alias; }');
118+
expect(r.free).toEqual(['FALLBACK']);
119+
});
120+
121+
it('a shorthand element whose default is free is still reported', () => {
122+
const r = detectFreeIdentifiers('(ctx) => { const { previous = FALLBACK } = ctx; return previous; }');
123+
expect(r.free).toEqual(['FALLBACK']);
124+
});
125+
126+
it('a computed key that is free is still reported', () => {
127+
const r = detectFreeIdentifiers('(ctx) => { const { [KEY]: v } = ctx; return v; }');
128+
expect(r.free).toEqual(['KEY']);
129+
});
130+
131+
it('a genuinely free `previous` (no ctx source) is still reported', () => {
132+
const r = detectFreeIdentifiers('(ctx) => { const prev = previous; ctx.input.n = prev.n; }');
133+
expect(r.free).toEqual(['previous']);
134+
});
135+
});
136+
78137
describe('real compiled `.toString()` shapes', () => {
79138
it('does not flag a self-contained closure', () => {
80139
const handler = (ctx: any) => {
@@ -83,6 +142,15 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => {
83142
};
84143
expect(detectFreeIdentifiers(src(handler)).free).toEqual([]);
85144
});
145+
146+
it('does not flag a renamed destructuring key after the compiler has had it', () => {
147+
const handler = (ctx: any) => {
148+
const { event, input, previous: prev } = ctx;
149+
if (event === 'beforeUpdate' && prev) input.n = prev.n;
150+
};
151+
expect(src(handler)).toContain('previous: prev');
152+
expect(detectFreeIdentifiers(src(handler)).free).toEqual([]);
153+
});
86154
});
87155

88156
it('never invents free vars for non-handler junk (conservative — caller won\'t block)', () => {

‎packages/cli/src/utils/detect-free-identifiers.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -227,8 +227,9 @@ function collectBindings(fn: ts.FunctionLikeDeclarationBase): Set<string> {
227227
/**
228228
* Collect identifiers used in VALUE position (potential references). Excludes
229229
* the false-positive sources: property-access member names, non-shorthand
230-
* object/class member keys, and statement labels. Binding names that slip
231-
* through are harmless — they are subtracted via `bindings` downstream.
230+
* object/class member keys, non-computed destructuring keys, and statement
231+
* labels. Binding names that slip through are harmless — they are subtracted
232+
* via `bindings` downstream.
232233
*/
233234
function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set<string> {
234235
const refs = new Set<string>();
@@ -253,6 +254,19 @@ function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set<string> {
253254
walk(node.initializer);
254255
return;
255256
}
257+
if (ts.isBindingElement(node)) {
258+
// `{ key: alias = d } = ctx` — the same rule as `{ key: value }` above:
259+
// `key` names a property READ OFF the source, so it is not a ref (unless
260+
// computed). Visit the binding name (an alias is subtracted via
261+
// `bindings`; a nested pattern recurses here), a computed key's
262+
// expression, and the default — `{ [k]: v = d }` reads `k` and `d`.
263+
if (node.propertyName && ts.isComputedPropertyName(node.propertyName)) {
264+
walk(node.propertyName.expression);
265+
}
266+
walk(node.name);
267+
if (node.initializer) walk(node.initializer);
268+
return;
269+
}
256270
if (ts.isMethodDeclaration(node) || ts.isPropertyDeclaration(node) || ts.isGetAccessor(node) || ts.isSetAccessor(node)) {
257271
if (node.name && ts.isComputedPropertyName(node.name)) walk(node.name.expression);
258272
ts.forEachChild(node, (c) => { if (c !== node.name) walk(c); });

0 commit comments

Comments
 (0)