Fix: Prevent map transformation when inside derive and exclude untransformed map parameters (commontoolsinc#1960)

seefeldb · claude · web-flow · commit 36d60c5ed5d6 · 2025-10-24T13:59:44.000-07:00
* Fix: Skip map transformation when map will be inside derive When a map is nested in an expression with opaque refs (e.g., `list.length > 0 && list.map(...)`), the OpaqueRefJSXTransformer wraps the entire expression in `derive(list, list => ...)`, which unwraps the array to a plain array. If ClosureTransformer had already transformed the map to `mapWithPattern`, this causes runtime errors since plain arrays don't have `mapWithPattern`. This fix adds `shouldTransformMap()` which uses dataflow analysis to detect when a map will be inside a derive and skips the transformation, keeping it as plain `.map()` which works correctly on unwrapped arrays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix: Exclude untransformed map callback parameters from outer derive capture When a plain .map() call is inside an expression that gets wrapped in derive(), the map callback parameters should not be treated as opaque parameters for the purposes of the outer derive. This is because: 1. The map is NOT being transformed to mapWithPattern (stays as plain .map) 2. The callback runs on unwrapped array elements inside the derive 3. The callback parameters are local to that scope, not outer scope variables This fix modifies getOpaqueCallKindForParameter() to check if a callback was actually transformed (using context.isMapCallback) before treating its parameters as opaque. Untransformed callbacks have regular parameters that should not be captured in outer derives. Example that's now fixed: ```tsx {items.map((item) => <div>{item.name && <span>{item.name}</span>}</div>)} ``` Before: derive({ items, item_name: item.name }, ...) ❌ After: derive({ items }, ...) ✅ 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/packages/ts-transformers/src/closures/transformer.ts b/packages/ts-transformers/src/closures/transformer.ts
@@ -1,7 +1,7 @@
 import ts from "typescript";
 import { TransformationContext, Transformer } from "../core/mod.ts";
 import { isOpaqueRefType } from "../transformers/opaque-ref/opaque-ref.ts";
-import { visitEachChildWithJsx } from "../ast/mod.ts";
+import { createDataFlowAnalyzer, visitEachChildWithJsx } from "../ast/mod.ts";
 
 export class ClosureTransformer extends Transformer {
   override filter(context: TransformationContext): boolean {
@@ -715,6 +715,72 @@ function expressionsMatch(a: ts.Expression, b: ts.Expression): boolean {
   return false;
 }
 
+/**
+ * Check if a map call should be transformed to mapWithPattern.
+ * Returns false if the map will end up inside a derive (where the array is unwrapped).
+ *
+ * This happens when the map is nested inside a larger expression with opaque refs,
+ * e.g., `list.length > 0 && list.map(...)` becomes `derive(list, list => ...)`
+ */
+function shouldTransformMap(
+  mapCall: ts.CallExpression,
+  context: TransformationContext,
+): boolean {
+  // Find the closest containing JSX expression
+  let node: ts.Node = mapCall;
+  let closestJsxExpression: ts.JsxExpression | undefined;
+
+  while (node.parent) {
+    if (ts.isJsxExpression(node.parent)) {
+      closestJsxExpression = node.parent;
+      break;
+    }
+    node = node.parent;
+  }
+
+  // If we didn't find a JSX expression, default to transforming
+  // (this handles maps in regular statements like `const x = items.map(...)`)
+  if (!closestJsxExpression || !closestJsxExpression.expression) {
+    return true;
+  }
+
+  const analyze = createDataFlowAnalyzer(context.checker);
+
+  //Case 1: Map is nested in a larger expression within the same JSX expression
+  // Example: {list.length > 0 && list.map(...)}
+  // Only check THIS expression for derive wrapping
+  if (closestJsxExpression.expression !== mapCall) {
+    const analysis = analyze(closestJsxExpression.expression);
+    // Check if this will be wrapped in a derive (not just transformed in some other way)
+    // Array-map calls have skip-call-rewrite hint, so they won't be wrapped in derive
+    const willBeWrappedInDerive = analysis.requiresRewrite &&
+      !(analysis.rewriteHint?.kind === "skip-call-rewrite" &&
+        analysis.rewriteHint.reason === "array-map");
+    return !willBeWrappedInDerive;
+  }
+
+  // Case 2: Map IS the direct content of the JSX expression
+  // Example: <div>{list.map(...)}</div>
+  // Check if an ANCESTOR JSX expression will wrap this in a derive
+  node = closestJsxExpression.parent;
+  while (node) {
+    if (ts.isJsxExpression(node) && node.expression) {
+      const analysis = analyze(node.expression);
+      const willBeWrappedInDerive = analysis.requiresRewrite &&
+        !(analysis.rewriteHint?.kind === "skip-call-rewrite" &&
+          analysis.rewriteHint.reason === "array-map");
+      if (willBeWrappedInDerive) {
+        // An ancestor JSX expression will wrap this in a derive
+        return false;
+      }
+    }
+    node = node.parent;
+  }
+
+  // No ancestor will wrap in derive, transform normally
+  return true;
+}
+
 /**
  * Create a visitor function that transforms OpaqueRef map calls.
  * This visitor can be reused for both top-level and nested transformations.
@@ -734,7 +800,11 @@ function createMapTransformVisitor(
         callback &&
         (ts.isArrowFunction(callback) || ts.isFunctionExpression(callback))
       ) {
-        return transformMapCallback(node, callback, context, visit);
+        // Check if this map will end up inside a derive (where array is unwrapped)
+        // If so, skip transformation and keep it as plain .map
+        if (shouldTransformMap(node, context)) {
+          return transformMapCallback(node, callback, context, visit);
+        }
       }
     }
 
diff --git a/packages/ts-transformers/src/transformers/opaque-ref/helpers.ts b/packages/ts-transformers/src/transformers/opaque-ref/helpers.ts
@@ -15,6 +15,7 @@ function originatesFromIgnoredParameter(
   scopeId: number,
   analysis: DataFlowAnalysis,
   checker: ts.TypeChecker,
+  context?: TransformationContext,
 ): boolean {
   const scope = analysis.graph.scopes.find((candidate) =>
     candidate.id === scopeId
@@ -28,7 +29,7 @@ function originatesFromIgnoredParameter(
       if (parameter.symbol === symbol || parameter.name === symbolName) {
         if (
           parameter.declaration &&
-          getOpaqueCallKindForParameter(parameter.declaration, checker)
+          getOpaqueCallKindForParameter(parameter.declaration, checker, context)
         ) {
           return false;
         }
@@ -70,6 +71,7 @@ function originatesFromIgnoredParameter(
 function getOpaqueCallKindForParameter(
   declaration: ts.ParameterDeclaration,
   checker: ts.TypeChecker,
+  context?: TransformationContext,
 ): "builder" | "array-map" | undefined {
   let functionNode: ts.Node | undefined = declaration.parent;
   while (functionNode && !ts.isFunctionLike(functionNode)) {
@@ -84,8 +86,18 @@ function getOpaqueCallKindForParameter(
   if (!candidate) return undefined;
 
   const callKind = detectCallKind(candidate, checker);
-  if (callKind?.kind === "builder" || callKind?.kind === "array-map") {
-    return callKind.kind;
+  if (callKind?.kind === "builder") {
+    return "builder";
+  }
+  if (callKind?.kind === "array-map") {
+    // For array-map calls, only treat parameters as opaque if the callback
+    // was actually transformed (marked in mapCallbackRegistry)
+    // Untransformed maps (plain .map inside derives) should have regular parameters
+    if (context && !context.isMapCallback(functionNode)) {
+      // Callback was not transformed, parameters are not opaque
+      return undefined;
+    }
+    return "array-map";
   }
   return undefined;
 }
@@ -156,6 +168,7 @@ export function filterRelevantDataFlows(
               dataFlow.scopeId,
               analysis,
               context.checker,
+              context,
             )
           ) {
             return false;
@@ -195,6 +208,7 @@ export function filterRelevantDataFlows(
               dataFlow.scopeId,
               analysis,
               context.checker,
+              context,
             )
           ) {
             return false;
@@ -216,6 +230,7 @@ export function filterRelevantDataFlows(
         dataFlow.scopeId,
         analysis,
         context.checker,
+        context,
       )
     ) {
       return false;
diff --git a/packages/ts-transformers/test/fixtures/jsx-expressions/map-array-length-conditional.expected.tsx b/packages/ts-transformers/test/fixtures/jsx-expressions/map-array-length-conditional.expected.tsx
@@ -0,0 +1,16 @@
+import * as __ctHelpers from "commontools";
+import { cell, recipe, UI } from "commontools";
+export default recipe("MapArrayLengthConditional", (_state) => {
+    const list = cell(["apple", "banana", "cherry"]);
+    return {
+        [UI]: (<div>
+        {__ctHelpers.derive(list, list => list.length > 0 && (<div>
+            {list.map((name) => (<span>{name}</span>))}
+          </div>))}
+      </div>),
+    };
+});
+// @ts-ignore: Internals
+function h(...args: any[]) { return __ctHelpers.h.apply(null, args); }
+// @ts-ignore: Internals
+h.fragment = __ctHelpers.h.fragment;
diff --git a/packages/ts-transformers/test/fixtures/jsx-expressions/map-array-length-conditional.input.tsx b/packages/ts-transformers/test/fixtures/jsx-expressions/map-array-length-conditional.input.tsx
@@ -0,0 +1,20 @@
+/// <cts-enable />
+import { cell, recipe, UI } from "commontools";
+
+export default recipe("MapArrayLengthConditional", (_state) => {
+  const list = cell(["apple", "banana", "cherry"]);
+
+  return {
+    [UI]: (
+      <div>
+        {list.length > 0 && (
+          <div>
+            {list.map((name) => (
+              <span>{name}</span>
+            ))}
+          </div>
+        )}
+      </div>
+    ),
+  };
+});
diff --git a/packages/ts-transformers/test/fixtures/jsx-expressions/map-nested-conditional.expected.tsx b/packages/ts-transformers/test/fixtures/jsx-expressions/map-nested-conditional.expected.tsx
@@ -6,33 +6,9 @@ export default recipe("MapNestedConditional", (_state) => {
     return {
         [UI]: (<div>
         {__ctHelpers.derive({ showList, items }, ({ showList: showList, items: items }) => showList && (<div>
-            {items.mapWithPattern(__ctHelpers.recipe({
-                $schema: "https://json-schema.org/draft/2020-12/schema",
-                type: "object",
-                properties: {
-                    element: {
-                        $ref: "#/$defs/__object"
-                    },
-                    params: {
-                        type: "object",
-                        properties: {}
-                    }
-                },
-                required: ["element", "params"],
-                $defs: {
-                    __object: {
-                        type: "object",
-                        properties: {
-                            name: {
-                                type: "string"
-                            }
-                        },
-                        required: ["name"]
-                    }
-                }
-            } as const satisfies __ctHelpers.JSONSchema, ({ element, params: {} }) => (<div>
-                {__ctHelpers.derive(element.name, _v1 => _v1 && <span>{_v1}</span>)}
-              </div>)), {})}
+            {items.map((item) => (<div>
+                {__ctHelpers.derive(item.name, _v1 => _v1 && <span>{_v1}</span>)}
+              </div>))}
           </div>))}
       </div>),
     };
diff --git a/packages/ts-transformers/test/fixtures/jsx-expressions/map-single-capture.expected.tsx b/packages/ts-transformers/test/fixtures/jsx-expressions/map-single-capture.expected.tsx
@@ -8,34 +8,7 @@ export default recipe("MapSingleCapture", (_state) => {
     return {
         [UI]: (<div>
         {__ctHelpers.derive(people, people => people.length > 0 && (<ul>
-            {people.mapWithPattern(__ctHelpers.recipe({
-                $schema: "https://json-schema.org/draft/2020-12/schema",
-                type: "object",
-                properties: {
-                    element: {
-                        $ref: "#/$defs/__object"
-                    },
-                    params: {
-                        type: "object",
-                        properties: {}
-                    }
-                },
-                required: ["element", "params"],
-                $defs: {
-                    __object: {
-                        type: "object",
-                        properties: {
-                            id: {
-                                type: "string"
-                            },
-                            name: {
-                                type: "string"
-                            }
-                        },
-                        required: ["id", "name"]
-                    }
-                }
-            } as const satisfies __ctHelpers.JSONSchema, ({ element, params: {} }) => (<li key={element.id}>{element.name}</li>)), {})}
+            {people.map((person) => (<li key={person.id}>{person.name}</li>))}
           </ul>))}
       </div>),
     };
diff --git a/packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx b/packages/ts-transformers/test/fixtures/jsx-expressions/opaque-ref-cell-map.expected.tsx
@@ -101,26 +101,12 @@ export default recipe("Charms Launcher", () => {
         [UI]: (<div>
         <h3>Stored Charms:</h3>
         {ifElse(__ctHelpers.derive(typedCellRef, typedCellRef => !typedCellRef?.length), <div>No charms created yet</div>, <ul>
-            {typedCellRef.mapWithPattern(__ctHelpers.recipe({
-                $schema: "https://json-schema.org/draft/2020-12/schema",
-                type: "object",
-                properties: {
-                    element: true,
-                    index: {
-                        type: "number"
-                    },
-                    params: {
-                        type: "object",
-                        properties: {}
-                    }
-                },
-                required: ["element", "params"]
-            } as const satisfies __ctHelpers.JSONSchema, ({ element, index, params: {} }) => (<li>
-                <ct-button onClick={goToCharm({ charm: element })}>
+            {typedCellRef.map((charm: any, index: number) => (<li>
+                <ct-button onClick={goToCharm({ charm })}>
                   Go to Charm {__ctHelpers.derive(index, index => index + 1)}
                 </ct-button>
-                <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(element, element => element[NAME] || "Unnamed")}</span>
-              </li>)), {})}
+                <span>Charm {__ctHelpers.derive(index, index => index + 1)}: {__ctHelpers.derive(charm, charm => charm[NAME] || "Unnamed")}</span>
+              </li>))}
           </ul>)}
 
         <ct-button onClick={createSimpleRecipe({ cellRef })}>
@@ -134,4 +120,3 @@ export default recipe("Charms Launcher", () => {
 function h(...args: any[]) { return __ctHelpers.h.apply(null, args); }
 // @ts-ignore: Internals
 h.fragment = __ctHelpers.h.fragment;
-
