Skip to content

Commit 2a2edd7

Browse files
cursoragentskovhus
andcommitted
Use free-variable collection for sibling binding detection
collectIdentifiers was too broad — it included member property names like `color` from `theme.color.bgFocus`, causing false positives when a destructured sibling shared that name. Replace with collectFreeIdentifiers which skips non-computed property names in member expressions, collecting only actual variable references (roots of expression chains). Co-authored-by: Kenneth Skovhus <skovhus@users.noreply.github.com>
1 parent f27da11 commit 2a2edd7

File tree

2 files changed

+97
-2
lines changed

2 files changed

+97
-2
lines changed

src/__tests__/transform.test.ts

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3759,6 +3759,58 @@ export const App = () => <Box enabled>Test</Box>;
37593759
expect(result.code).toBeNull();
37603760
});
37613761

3762+
it("should bail when both theme boolean branches are unresolvable", () => {
3763+
// When resolveValueOptional returns undefined for both branches of a
3764+
// theme.isDark ternary, the codemod must bail rather than fall through
3765+
// to generic handlers that would emit the arrow function as a string.
3766+
const source = `
3767+
import styled from "styled-components";
3768+
3769+
const Box = styled.div\`
3770+
color: \${(p) =>
3771+
p.theme.isDark ? p.theme.baseTheme?.color.bgSub : p.theme.baseTheme?.color.bgBase};
3772+
\`;
3773+
3774+
export const App = () => <Box>Test</Box>;
3775+
`;
3776+
3777+
const result = transformWithWarnings(
3778+
{ source, path: "theme-both-unresolved.tsx" },
3779+
{ jscodeshift: j, j, stats: () => {}, report: () => {} },
3780+
{ adapter: fixtureAdapter },
3781+
);
3782+
3783+
expect(result.code).toBeNull();
3784+
});
3785+
3786+
it("should not reject sibling binding when name collides with theme property path segment", () => {
3787+
// collectIdentifiers picks up member property names like `color` from
3788+
// `theme.color.bgFocus`. A destructured sibling named `color` must not
3789+
// cause a false positive rejection — only actual free variable references
3790+
// should be checked.
3791+
const source = `
3792+
import styled from "styled-components";
3793+
3794+
const Box = styled.div\`
3795+
background-color: \${({ theme, color }) =>
3796+
theme.isDark ? theme.baseTheme?.color.bgSub : theme.color.bgFocus};
3797+
\`;
3798+
3799+
export const App = () => <Box color="red">Test</Box>;
3800+
`;
3801+
3802+
const result = transformWithWarnings(
3803+
{ source, path: "theme-color-sibling.tsx" },
3804+
{ jscodeshift: j, j, stats: () => {}, report: () => {} },
3805+
{ adapter: fixtureAdapter },
3806+
);
3807+
3808+
// The `color` destructured param is never referenced in the expression
3809+
// (only theme.color is used), so the transform should succeed
3810+
expect(result.code).not.toBeNull();
3811+
expect(result.code).toContain("useTheme");
3812+
});
3813+
37623814
it("should bail on theme access inside pseudo/media context (module-scoped style fn)", () => {
37633815
// Theme-rewritten expressions can't be placed in module-scoped stylex.create functions.
37643816
// The `theme` variable from useTheme() is only available in the wrapper component body.

src/internal/builtin-handlers/conditionals.ts

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import {
77
type ArrowFnParamBindings,
88
type CallExpressionNode,
99
cloneAstNode,
10-
collectIdentifiers,
1110
getArrowFnParamBindings,
1211
getArrowFnSingleParamName,
1312
getFunctionBodyExpr,
@@ -1678,7 +1677,7 @@ function isFullyTransformedThemeExpr(
16781677
info: ThemeParamInfo | null,
16791678
): boolean {
16801679
const ids = new Set<string>();
1681-
collectIdentifiers(transformed, ids);
1680+
collectFreeIdentifiers(transformed, ids);
16821681
if (paramName && ids.has(paramName)) {
16831682
return false;
16841683
}
@@ -1696,6 +1695,50 @@ function isFullyTransformedThemeExpr(
16961695
return true;
16971696
}
16981697

1698+
/**
1699+
* Collects free variable identifiers from an AST node, excluding
1700+
* non-computed property names in member expressions (e.g., in
1701+
* `theme.color.bgSub`, only `theme` is a free variable; `color`
1702+
* and `bgSub` are property accesses, not variable references).
1703+
*/
1704+
function collectFreeIdentifiers(node: unknown, out: Set<string>): void {
1705+
if (!node || typeof node !== "object") {
1706+
return;
1707+
}
1708+
if (Array.isArray(node)) {
1709+
for (const child of node) {
1710+
collectFreeIdentifiers(child, out);
1711+
}
1712+
return;
1713+
}
1714+
const typed = node as Record<string, unknown>;
1715+
const nodeType = typed.type as string | undefined;
1716+
1717+
// For member expressions, only recurse into the object (left side).
1718+
// Skip the property name unless it's computed (bracket notation).
1719+
if (nodeType === "MemberExpression" || nodeType === "OptionalMemberExpression") {
1720+
collectFreeIdentifiers(typed.object, out);
1721+
if (typed.computed) {
1722+
collectFreeIdentifiers(typed.property, out);
1723+
}
1724+
return;
1725+
}
1726+
1727+
if (nodeType === "Identifier" && typeof typed.name === "string") {
1728+
out.add(typed.name);
1729+
}
1730+
1731+
for (const key of Object.keys(typed)) {
1732+
if (key === "loc" || key === "comments" || key === "type") {
1733+
continue;
1734+
}
1735+
const child = typed[key];
1736+
if (child && typeof child === "object") {
1737+
collectFreeIdentifiers(child, out);
1738+
}
1739+
}
1740+
}
1741+
16991742
/**
17001743
* Deep-clones an expression and replaces `<paramName>.theme.*` or `theme.*`
17011744
* (for destructured theme binding) with `theme.*` references suitable for

0 commit comments

Comments
 (0)