-
Notifications
You must be signed in to change notification settings - Fork 9
Fix map capture collisions #1975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a297631
5dda474
5f8f83b
013947b
839a59e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -897,17 +897,114 @@ function transformDestructuredProperties( | |
| ): ts.ConciseBody { | ||
| const elemName = elemParam?.name; | ||
|
|
||
| // Collect destructured property names if the param is an object destructuring pattern | ||
| const destructuredProps = new Set<string>(); | ||
| let transformedBody: ts.ConciseBody = body; | ||
|
|
||
| const prependStatements = ( | ||
| statements: readonly ts.Statement[], | ||
| currentBody: ts.ConciseBody, | ||
| ): ts.ConciseBody => { | ||
| if (statements.length === 0) return currentBody; | ||
|
|
||
| if (ts.isBlock(currentBody)) { | ||
| return factory.updateBlock( | ||
| currentBody, | ||
| factory.createNodeArray([ | ||
| ...statements, | ||
| ...currentBody.statements, | ||
| ]), | ||
| ); | ||
| } | ||
|
|
||
| return factory.createBlock( | ||
| [ | ||
| ...statements, | ||
| factory.createReturnStatement(currentBody as ts.Expression), | ||
| ], | ||
| true, | ||
| ); | ||
| }; | ||
|
|
||
| const destructuredProps = new Map<string, () => ts.Expression>(); | ||
| const computedInitializers: ts.VariableStatement[] = []; | ||
| const usedTempNames = new Set<string>(); | ||
|
|
||
| const registerTempName = (base: string): string => { | ||
| let candidate = `__ct_${base || "prop"}_key`; | ||
| let counter = 1; | ||
| while (usedTempNames.has(candidate)) { | ||
| candidate = `__ct_${base || "prop"}_key_${counter++}`; | ||
| } | ||
| usedTempNames.add(candidate); | ||
| return candidate; | ||
| }; | ||
|
|
||
| if (elemName && ts.isObjectBindingPattern(elemName)) { | ||
| for (const element of elemName.elements) { | ||
| if (ts.isBindingElement(element) && ts.isIdentifier(element.name)) { | ||
| destructuredProps.add(element.name.text); | ||
| const alias = element.name.text; | ||
| const propertyName = element.propertyName; | ||
| usedTempNames.add(alias); | ||
|
|
||
| destructuredProps.set(alias, () => { | ||
| const target = factory.createIdentifier("element"); | ||
|
|
||
| if (!propertyName) { | ||
| return factory.createPropertyAccessExpression( | ||
| target, | ||
| factory.createIdentifier(alias), | ||
| ); | ||
| } | ||
|
|
||
| if (ts.isIdentifier(propertyName)) { | ||
| return factory.createPropertyAccessExpression( | ||
| target, | ||
| factory.createIdentifier(propertyName.text), | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| ts.isStringLiteral(propertyName) || | ||
| ts.isNumericLiteral(propertyName) | ||
| ) { | ||
| return factory.createElementAccessExpression(target, propertyName); | ||
| } | ||
|
|
||
| if (ts.isComputedPropertyName(propertyName)) { | ||
| const tempName = registerTempName(alias); | ||
| const tempIdentifier = factory.createIdentifier(tempName); | ||
|
|
||
| computedInitializers.push( | ||
| factory.createVariableStatement( | ||
| undefined, | ||
| factory.createVariableDeclarationList( | ||
| [ | ||
| factory.createVariableDeclaration( | ||
| tempIdentifier, | ||
| undefined, | ||
| undefined, | ||
| propertyName.expression, | ||
| ), | ||
| ], | ||
| ts.NodeFlags.Const, | ||
| ), | ||
| ), | ||
| ); | ||
|
|
||
| return factory.createElementAccessExpression( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Transforming destructured aliases with computed property names into direct element accesses re-evaluates the property expression ( Prompt for AI agents✅ Addressed in |
||
| target, | ||
| tempIdentifier, | ||
| ); | ||
| } | ||
|
|
||
| return factory.createPropertyAccessExpression( | ||
| target, | ||
| factory.createIdentifier(alias), | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Collect array destructured identifiers: [date, pizza] -> {date: 0, pizza: 1} | ||
| const arrayDestructuredVars = new Map<string, number>(); | ||
| if (elemName && ts.isArrayBindingPattern(elemName)) { | ||
| let index = 0; | ||
|
|
@@ -919,35 +1016,28 @@ function transformDestructuredProperties( | |
| } | ||
| } | ||
|
|
||
| // If param was object-destructured, replace property references with element.prop | ||
| if (destructuredProps.size > 0) { | ||
| const visitor: ts.Visitor = (node) => { | ||
| if (ts.isIdentifier(node) && destructuredProps.has(node.text)) { | ||
| // Check if this identifier is not part of a property access already | ||
| // (e.g., don't transform the 'x' in 'something.x') | ||
| if ( | ||
| !node.parent || | ||
| !(ts.isPropertyAccessExpression(node.parent) && | ||
| node.parent.name === node) | ||
| ) { | ||
| return factory.createPropertyAccessExpression( | ||
| factory.createIdentifier("element"), | ||
| factory.createIdentifier(node.text), | ||
| ); | ||
| const accessFactory = destructuredProps.get(node.text)!; | ||
| return accessFactory(); | ||
| } | ||
| } | ||
| return visitEachChildWithJsx(node, visitor, undefined); | ||
| }; | ||
| return ts.visitNode(body, visitor) as ts.ConciseBody; | ||
| transformedBody = ts.visitNode(transformedBody, visitor) as ts.ConciseBody; | ||
| } | ||
|
|
||
| // If param was array-destructured, replace variable references with element[index] | ||
| if (arrayDestructuredVars.size > 0) { | ||
| const visitor: ts.Visitor = (node) => { | ||
| if (ts.isIdentifier(node)) { | ||
| const index = arrayDestructuredVars.get(node.text); | ||
| if (index !== undefined) { | ||
| // Check if this identifier is not part of a property access already | ||
| if ( | ||
| !node.parent || | ||
| !(ts.isPropertyAccessExpression(node.parent) && | ||
|
|
@@ -962,10 +1052,14 @@ function transformDestructuredProperties( | |
| } | ||
| return visitEachChildWithJsx(node, visitor, undefined); | ||
| }; | ||
| return ts.visitNode(body, visitor) as ts.ConciseBody; | ||
| transformedBody = ts.visitNode(transformedBody, visitor) as ts.ConciseBody; | ||
| } | ||
|
|
||
| return body; | ||
| if (computedInitializers.length > 0) { | ||
| transformedBody = prependStatements(computedInitializers, transformedBody); | ||
| } | ||
|
|
||
| return transformedBody; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -1155,12 +1249,32 @@ function transformMapCallback( | |
| const captureExpressions = collectCaptures(callback, checker); | ||
|
|
||
| // Build map of capture name -> expression | ||
| const captures = new Map<string, ts.Expression>(); | ||
| const captureEntries: Array<{ name: string; expr: ts.Expression }> = []; | ||
| const usedNames = new Set<string>(["element", "index", "array", "params"]); | ||
|
|
||
| for (const expr of captureExpressions) { | ||
| const name = getCaptureName(expr); | ||
| if (name && !captures.has(name)) { | ||
| captures.set(name, expr); | ||
| const baseName = getCaptureName(expr); | ||
| if (!baseName) continue; | ||
|
|
||
| // Skip if an existing entry captures an equivalent expression | ||
| const existing = captureEntries.find((entry) => | ||
| expressionsMatch(expr, entry.expr) | ||
| ); | ||
| if (existing) continue; | ||
|
|
||
| let candidate = baseName; | ||
| let counter = 2; | ||
| while (usedNames.has(candidate)) { | ||
| candidate = `${baseName}_${counter++}`; | ||
| } | ||
|
|
||
| usedNames.add(candidate); | ||
| captureEntries.push({ name: candidate, expr }); | ||
| } | ||
|
|
||
| const captures = new Map<string, ts.Expression>(); | ||
| for (const entry of captureEntries) { | ||
| captures.set(entry.name, entry.expr); | ||
| } | ||
|
|
||
| // Build set of captured variable names | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import * as __ctHelpers from "commontools"; | ||
| import { recipe, UI } from "commontools"; | ||
| let keyCounter = 0; | ||
| function nextKey() { | ||
| return `value-${keyCounter++}`; | ||
| } | ||
| interface State { | ||
| items: Array<Record<string, number>>; | ||
| } | ||
| export default recipe({ | ||
| type: "object", | ||
| properties: { | ||
| items: { | ||
| type: "array", | ||
| items: { | ||
| type: "object", | ||
| properties: {}, | ||
| additionalProperties: { | ||
| type: "number" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| required: ["items"] | ||
| } as const satisfies __ctHelpers.JSONSchema, (state) => { | ||
| return { | ||
| [UI]: (<div> | ||
| {state.items.mapWithPattern(__ctHelpers.recipe({ | ||
| $schema: "https://json-schema.org/draft/2020-12/schema", | ||
| type: "object", | ||
| properties: { | ||
| element: { | ||
| type: "object", | ||
| properties: {}, | ||
| additionalProperties: { | ||
| type: "number" | ||
| } | ||
| }, | ||
| params: { | ||
| type: "object", | ||
| properties: {} | ||
| } | ||
| }, | ||
| required: ["element", "params"] | ||
| } as const satisfies __ctHelpers.JSONSchema, ({ element, params: {} }) => { | ||
| const __ct_amount_key = nextKey(); | ||
| return (<span>{__ctHelpers.derive({ element, __ct_amount_key }, ({ element: element, __ct_amount_key: __ct_amount_key }) => element[__ct_amount_key])}</span>); | ||
| }), {})} | ||
| </div>), | ||
| }; | ||
| }); | ||
| // @ts-ignore: Internals | ||
| function h(...args: any[]) { return __ctHelpers.h.apply(null, args); } | ||
| // @ts-ignore: Internals | ||
| h.fragment = __ctHelpers.h.fragment; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| /// <cts-enable /> | ||
| import { recipe, UI } from "commontools"; | ||
|
|
||
| let keyCounter = 0; | ||
| function nextKey() { | ||
| return `value-${keyCounter++}`; | ||
| } | ||
|
|
||
| interface State { | ||
| items: Array<Record<string, number>>; | ||
| } | ||
|
|
||
| export default recipe<State>("ComputedAliasSideEffect", (state) => { | ||
| return { | ||
| [UI]: ( | ||
| <div> | ||
| {state.items.map(({ [nextKey()]: amount }) => ( | ||
| <span>{amount}</span> | ||
| ))} | ||
| </div> | ||
| ), | ||
| }; | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import * as __ctHelpers from "commontools"; | ||
| import { recipe, UI } from "commontools"; | ||
| interface State { | ||
| items: Array<{ | ||
| price: number; | ||
| }>; | ||
| discount: number; | ||
| } | ||
| export default recipe({ | ||
| type: "object", | ||
| properties: { | ||
| items: { | ||
| type: "array", | ||
| items: { | ||
| type: "object", | ||
| properties: { | ||
| price: { | ||
| type: "number" | ||
| } | ||
| }, | ||
| required: ["price"] | ||
| } | ||
| }, | ||
| discount: { | ||
| type: "number" | ||
| } | ||
| }, | ||
| required: ["items", "discount"] | ||
| } as const satisfies __ctHelpers.JSONSchema, (state) => { | ||
| return { | ||
| [UI]: (<div> | ||
| {state.items.mapWithPattern(__ctHelpers.recipe({ | ||
| $schema: "https://json-schema.org/draft/2020-12/schema", | ||
| type: "object", | ||
| properties: { | ||
| element: { | ||
| type: "object", | ||
| properties: { | ||
| price: { | ||
| type: "number" | ||
| } | ||
| }, | ||
| required: ["price"] | ||
| }, | ||
| params: { | ||
| type: "object", | ||
| properties: { | ||
| discount: { | ||
| type: "number", | ||
| asOpaque: true | ||
| } | ||
| }, | ||
| required: ["discount"] | ||
| } | ||
| }, | ||
| required: ["element", "params"] | ||
| } as const satisfies __ctHelpers.JSONSchema, ({ element, params: { discount } }) => (<span>{__ctHelpers.derive({ element_price: element.price, discount }, ({ element_price: _v1, discount: discount }) => _v1 * discount)}</span>)), { discount: state.discount })} | ||
| </div>), | ||
| }; | ||
| }); | ||
| // @ts-ignore: Internals | ||
| function h(...args: any[]) { return __ctHelpers.h.apply(null, args); } | ||
| // @ts-ignore: Internals | ||
| h.fragment = __ctHelpers.h.fragment; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /// <cts-enable /> | ||
| import { recipe, UI } from "commontools"; | ||
|
|
||
| interface State { | ||
| items: Array<{ price: number }>; | ||
| discount: number; | ||
| } | ||
|
|
||
| export default recipe<State>("MapDestructuredAlias", (state) => { | ||
| return { | ||
| [UI]: ( | ||
| <div> | ||
| {state.items.map(({ price: cost }) => ( | ||
| <span>{cost * state.discount}</span> | ||
| ))} | ||
| </div> | ||
| ), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if
NodeFactory#createTempVariableorNodeFactory#createUniqueNamecould be helpful in synthesizing unused var namesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for pointing those out, i'm glad to know about them. i think they might be a little overly complex for this exact usage, in that they return Identifiers and we'd need to get the text for either synthetic or non-synthetic nodes; they're also not fully deterministic so it might be a bit tricky to make fixtures for them although maybe they're deterministic-enough for a fixed input, i'm not sure
either way though i intend to deprecate this mechanism entirely in my next larger follow-up change to avoid rewriting closure bodies, so i'm going to leave this for now and come back to it later if that turns out not to be true