Skip to content

Commit ecc19ab

Browse files
skovhusclaude
andcommitted
fix: skip non-computed MemberExpression properties in identifier rewrite
replaceIdentifierInAst now skips the `property` key of non-computed MemberExpressions, preventing incorrect rewrites like `Math.min` → `Math.props.min` when the param name matches a member property name. Addresses review comment on #330. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d3c2d96 commit ecc19ab

2 files changed

Lines changed: 87 additions & 0 deletions

File tree

src/internal/lower-rules/finalize-decl.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ import type { VariantDimension } from "../transform-types.js";
2929
import type { WarningLog } from "../logger.js";
3030
import { isStyleConditionKey, mergeStyleObjects } from "./utils.js";
3131

32+
export { replaceIdentifierInAst };
33+
3234
export function finalizeDeclProcessing(ctx: DeclProcessingState): void {
3335
const {
3436
state,
@@ -910,6 +912,10 @@ function replaceIdentifierInAst(
910912
if (key === "type" || key === "loc" || key === "start" || key === "end" || key === "comments") {
911913
continue;
912914
}
915+
// Skip non-computed MemberExpression.property — it's a property name, not a variable reference
916+
if (key === "property" && n.type === "MemberExpression" && !n.computed) {
917+
continue;
918+
}
913919
const child = n[key];
914920
if (Array.isArray(child)) {
915921
for (let i = 0; i < child.length; i++) {
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import { describe, it, expect } from "vitest";
2+
import jscodeshift from "jscodeshift";
3+
import { replaceIdentifierInAst } from "./finalize-decl.js";
4+
5+
const j = jscodeshift.withParser("tsx");
6+
7+
/** Parse an expression and return the AST node. */
8+
function parseExpr(code: string): unknown {
9+
const root = j(`const __x = ${code}`);
10+
const decl = root.find(j.VariableDeclarator).get();
11+
return decl.node.init;
12+
}
13+
14+
/** Serialize an AST node back to source via recast. */
15+
function print(node: unknown): string {
16+
const root = j(`const __x = 0`);
17+
root.find(j.VariableDeclarator).get().node.init = node;
18+
return root.toSource().replace("const __x = ", "").replace(/;\s*$/, "");
19+
}
20+
21+
describe("replaceIdentifierInAst", () => {
22+
it("replaces identifier inside a template literal", () => {
23+
const node = parseExpr("`${size}px`");
24+
replaceIdentifierInAst(j, node, "size");
25+
expect(print(node)).toBe("`${props.size}px`");
26+
});
27+
28+
it("replaces identifier in a function call argument", () => {
29+
const node = parseExpr("clamp(size)");
30+
replaceIdentifierInAst(j, node, "size");
31+
expect(print(node)).toBe("clamp(props.size)");
32+
});
33+
34+
it("does NOT replace non-computed MemberExpression property names", () => {
35+
// Math.min(min, 100) — `min` in `Math.min` should NOT be replaced
36+
const node = parseExpr("Math.min(min, 100)");
37+
replaceIdentifierInAst(j, node, "min");
38+
expect(print(node)).toBe("Math.min(props.min, 100)");
39+
});
40+
41+
it("does NOT replace non-computed property access matching param name", () => {
42+
// obj.size should remain obj.size, not obj.props.size
43+
const node = parseExpr("obj.size + size");
44+
replaceIdentifierInAst(j, node, "size");
45+
expect(print(node)).toBe("obj.size + props.size");
46+
});
47+
48+
it("DOES replace computed MemberExpression property matching param name", () => {
49+
// obj[size] should become obj[props.size]
50+
const node = parseExpr("obj[size]");
51+
replaceIdentifierInAst(j, node, "size");
52+
expect(print(node)).toBe("obj[props.size]");
53+
});
54+
55+
it("does NOT replace nested non-computed member property", () => {
56+
// config.theme.color — if param is "color", only the argument should change
57+
const node = parseExpr("fn(config.color, color)");
58+
replaceIdentifierInAst(j, node, "color");
59+
expect(print(node)).toBe("fn(config.color, props.color)");
60+
});
61+
62+
it("replaces in spread element", () => {
63+
const node = parseExpr("({ ...rest })");
64+
replaceIdentifierInAst(j, node, "rest");
65+
expect(print(node)).toContain("props.rest");
66+
});
67+
68+
it("handles shorthand properties built by jscodeshift builders", () => {
69+
// Build an ObjectExpression using j.property() (which creates "Property" nodes,
70+
// matching what the codemod's lowering pipeline produces)
71+
const obj = j.objectExpression([
72+
j.property("init", j.identifier("color"), j.identifier("color")),
73+
]);
74+
// Mark as shorthand (jscodeshift builder doesn't do this automatically)
75+
(obj.properties[0] as unknown as Record<string, unknown>).shorthand = true;
76+
replaceIdentifierInAst(j, obj, "color");
77+
const prop = obj.properties[0] as unknown as Record<string, unknown>;
78+
expect(prop.shorthand).toBe(false);
79+
expect((prop.value as unknown as { type: string }).type).toBe("MemberExpression");
80+
});
81+
});

0 commit comments

Comments
 (0)