Fix map capture collisions #1975
Conversation
| const elemName = elemParam?.name; | ||
|
|
||
| // Collect destructured property names if the param is an object destructuring pattern | ||
| const destructuredProps = new Set<string>(); |
There was a problem hiding this comment.
I wonder if NodeFactory#createTempVariable or NodeFactory#createUniqueName could be helpful in synthesizing unused var names
There was a problem hiding this comment.
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
There was a problem hiding this comment.
1 issue found across 11 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/ts-transformers/src/closures/transformer.ts">
<violation number="1" location="packages/ts-transformers/src/closures/transformer.ts:933">
Transforming destructured aliases with computed property names into direct element accesses re-evaluates the property expression (`counter++`, function calls, etc.) on every use, so aliases no longer behave like bound values. This breaks callbacks that rely on the original single evaluation.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| if (ts.isComputedPropertyName(propertyName)) { | ||
| return factory.createElementAccessExpression( |
There was a problem hiding this comment.
Transforming destructured aliases with computed property names into direct element accesses re-evaluates the property expression (counter++, function calls, etc.) on every use, so aliases no longer behave like bound values. This breaks callbacks that rely on the original single evaluation.
Prompt for AI agents
Address the following comment on packages/ts-transformers/src/closures/transformer.ts at line 933:
<comment>Transforming destructured aliases with computed property names into direct element accesses re-evaluates the property expression (`counter++`, function calls, etc.) on every use, so aliases no longer behave like bound values. This breaks callbacks that rely on the original single evaluation.</comment>
<file context>
@@ -898,11 +898,49 @@ function transformDestructuredProperties(
+ }
+
+ if (ts.isComputedPropertyName(propertyName)) {
+ return factory.createElementAccessExpression(
+ target,
+ propertyName.expression,
</file context>
✅ Addressed in 839a59e
* Fix map closure destructured alias handling and add regression fixtures * Fix map closure capture name collisions and add regression fixture * Avoid recomputing computed destructured aliases and add fixture
Make it work when there's a name collision in map captures. This should be superseded soon by a larger refactor to avoid rewriting callback bodies.
Summary by cubic
Fixes map callback capture name collisions and correctly handles destructured aliases (including computed, string, and numeric keys). Prevents wrong property access and shadowing in transformed map closures.