diff --git a/.changeset/22459-hook-lowering-destructuring-alias.md b/.changeset/22459-hook-lowering-destructuring-alias.md new file mode 100644 index 00000000000..186311396a3 --- /dev/null +++ b/.changeset/22459-hook-lowering-destructuring-alias.md @@ -0,0 +1,11 @@ +--- +'@objectstack/cli': patch +--- + +fix(cli): a hook that destructures a renamed key off `ctx` is no longer refused as not-lowerable (#22459) + +Clause-②: no + +- **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. +- **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. +- **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. diff --git a/packages/cli/src/lint/hook-body-lowering.test.ts b/packages/cli/src/lint/hook-body-lowering.test.ts index f9e2fd6addf..1430deebf3d 100644 --- a/packages/cli/src/lint/hook-body-lowering.test.ts +++ b/packages/cli/src/lint/hook-body-lowering.test.ts @@ -22,6 +22,8 @@ import { lowerCallables } from '../utils/lower-callables.js'; // Module scope — exactly what a lowered body cannot reach. const SLA_MATRIX: Record = { high: 4, low: 48 }; +// Module scope, spelled like a `ctx` key — a reference to THIS is free. +const previous = { rank: 1 }; const freeIdentifierHook = { name: 'case_sla', @@ -139,6 +141,56 @@ describe('checkHookBodyLowering', () => { }); }); + describe('a hook that destructures a renamed key off `ctx` reaches its value through `ctx`', () => { + // `previous` is a property READ OFF ctx — exactly what this rule's own + // remedy prescribes — and `prev` is the only name the pattern binds. The + // scan used to count the key as a free identifier, so lint refused the + // hook (advising what it already did) and the build bundled it. + const renamedKeyHook = { + name: 'carry_rank', + object: 'task', + events: ['beforeInsert', 'beforeUpdate'], + handler: (ctx: any) => { + const { event, input, previous: prev } = ctx; + if (event === 'beforeUpdate' && prev) input.rank = prev.rank; + }, + }; + // The control: the same spelling, but read from module scope — not `ctx`. + const freePreviousHook = { + name: 'carry_rank_free', + object: 'task', + events: ['beforeUpdate'], + handler: (ctx: any) => { + const prev = previous; + ctx.input.rank = prev.rank; + }, + }; + + it('lint reports nothing, and the build lowers it to a body', () => { + expect(checkHookBodyLowering({ hooks: [{ ...renamedKeyHook }] })).toEqual([]); + + const lowering = lowerCallables({ hooks: [{ ...renamedKeyHook }] }); + expect(lowering.bodyExtractionWarnings).toEqual([]); + expect(lowering.bodyExtracted).toBe(1); + const [hook] = lowering.lowered.hooks as Array<{ body?: { source: string } }>; + expect(hook.body?.source).toContain('previous: prev'); + }); + + it('a genuinely free `previous` is still refused, by both', () => { + const issues = checkHookBodyLowering({ hooks: [{ ...freePreviousHook }] }); + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('error'); + expect(issues[0].rule).toBe(NOT_LOWERABLE_RULE); + expect(issues[0].path).toBe('hooks[0].handler'); + + const lowering = lowerCallables({ hooks: [{ ...freePreviousHook }] }); + expect(lowering.bodyExtracted).toBe(0); + expect(lowering.bodyExtractionWarnings.map((w) => [w.kind, w.freeIdentifiers])).toEqual([ + ['free-identifiers', ['previous']], + ]); + }); + }); + it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => { const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] }); diff --git a/packages/cli/src/utils/detect-free-identifiers.test.ts b/packages/cli/src/utils/detect-free-identifiers.test.ts index e0b68c67b04..c987581b3fe 100644 --- a/packages/cli/src/utils/detect-free-identifiers.test.ts +++ b/packages/cli/src/utils/detect-free-identifiers.test.ts @@ -75,6 +75,65 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => { } }); + // A renamed destructuring key — `const { previous: prev } = ctx` — reads + // `previous` OFF `ctx`; the only name it introduces is `prev`. The key is a + // property name exactly like the `key` of `{ key: value }`, so counting it as + // a free identifier refused a self-contained handler (and bundled it), with + // advice — "reach them through `ctx`" — describing what it already did. + describe('a destructuring KEY is not a reference; its alias is the binding', () => { + const lowerable: Array<[string, string]> = [ + [ + 'a renamed key off ctx, beside shorthand keys', + "(ctx) => { const { event, input, previous: prev } = ctx; if (event === 'beforeUpdate' && prev) input.n = prev.n; }", + ], + ['a nested renamed pattern', '(ctx) => { const { a: { b: c } } = ctx; return c; }'], + [ + 'a defaulted alias whose default is in scope', + '(ctx) => { const d = ctx.fallback; const { key: alias = d } = ctx; return alias; }', + ], + [ + 'a computed key and a default, both in scope', + '(ctx) => { const k = ctx.k; const d = 0; const { [k]: v = d } = ctx; return v; }', + ], + // The same element shape reached two other ways. + ['an object pattern inside an array pattern', '(ctx) => { const [{ previous: prev }] = ctx.items; return prev; }'], + ['a nested callback parameter', '(ctx) => ctx.items.map(({ previous: prev }) => prev.n)'], + ['a catch-clause pattern', '(ctx) => { try { ctx.run(); } catch ({ message: msg }) { ctx.log = msg; } }'], + // No key at all: elements are bound by position, so nothing to mistake. + ['an array pattern', '(ctx) => { const [first, second] = ctx.items; return first + second; }'], + ]; + for (const [label, source] of lowerable) { + it(label, () => { + const r = detectFreeIdentifiers(source); + expect(r.unparsed).toBe(false); + expect(r.free).toEqual([]); + }); + } + + // The controls: what the element still READS is still judged. Only the key + // stopped counting — a default or a computed key that names something out + // of scope is as free as it ever was. + it('a defaulted alias whose default is free is still reported', () => { + const r = detectFreeIdentifiers('(ctx) => { const { key: alias = FALLBACK } = ctx; return alias; }'); + expect(r.free).toEqual(['FALLBACK']); + }); + + it('a shorthand element whose default is free is still reported', () => { + const r = detectFreeIdentifiers('(ctx) => { const { previous = FALLBACK } = ctx; return previous; }'); + expect(r.free).toEqual(['FALLBACK']); + }); + + it('a computed key that is free is still reported', () => { + const r = detectFreeIdentifiers('(ctx) => { const { [KEY]: v } = ctx; return v; }'); + expect(r.free).toEqual(['KEY']); + }); + + it('a genuinely free `previous` (no ctx source) is still reported', () => { + const r = detectFreeIdentifiers('(ctx) => { const prev = previous; ctx.input.n = prev.n; }'); + expect(r.free).toEqual(['previous']); + }); + }); + describe('real compiled `.toString()` shapes', () => { it('does not flag a self-contained closure', () => { const handler = (ctx: any) => { @@ -83,6 +142,15 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => { }; expect(detectFreeIdentifiers(src(handler)).free).toEqual([]); }); + + it('does not flag a renamed destructuring key after the compiler has had it', () => { + const handler = (ctx: any) => { + const { event, input, previous: prev } = ctx; + if (event === 'beforeUpdate' && prev) input.n = prev.n; + }; + expect(src(handler)).toContain('previous: prev'); + expect(detectFreeIdentifiers(src(handler)).free).toEqual([]); + }); }); it('never invents free vars for non-handler junk (conservative — caller won\'t block)', () => { diff --git a/packages/cli/src/utils/detect-free-identifiers.ts b/packages/cli/src/utils/detect-free-identifiers.ts index e8c163f85d2..b768875ce12 100644 --- a/packages/cli/src/utils/detect-free-identifiers.ts +++ b/packages/cli/src/utils/detect-free-identifiers.ts @@ -227,8 +227,9 @@ function collectBindings(fn: ts.FunctionLikeDeclarationBase): Set { /** * Collect identifiers used in VALUE position (potential references). Excludes * the false-positive sources: property-access member names, non-shorthand - * object/class member keys, and statement labels. Binding names that slip - * through are harmless — they are subtracted via `bindings` downstream. + * object/class member keys, non-computed destructuring keys, and statement + * labels. Binding names that slip through are harmless — they are subtracted + * via `bindings` downstream. */ function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set { const refs = new Set(); @@ -253,6 +254,19 @@ function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set { walk(node.initializer); return; } + if (ts.isBindingElement(node)) { + // `{ key: alias = d } = ctx` — the same rule as `{ key: value }` above: + // `key` names a property READ OFF the source, so it is not a ref (unless + // computed). Visit the binding name (an alias is subtracted via + // `bindings`; a nested pattern recurses here), a computed key's + // expression, and the default — `{ [k]: v = d }` reads `k` and `d`. + if (node.propertyName && ts.isComputedPropertyName(node.propertyName)) { + walk(node.propertyName.expression); + } + walk(node.name); + if (node.initializer) walk(node.initializer); + return; + } if (ts.isMethodDeclaration(node) || ts.isPropertyDeclaration(node) || ts.isGetAccessor(node) || ts.isSetAccessor(node)) { if (node.name && ts.isComputedPropertyName(node.name)) walk(node.name.expression); ts.forEachChild(node, (c) => { if (c !== node.name) walk(c); });