Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import invariant from 'invariant';
import {isValidIdentifier} from '@babel/types';
import {Environment} from '../HIR';
import {
BasicBlock,
Expand Down Expand Up @@ -222,9 +223,19 @@ function collectProps(
let id = 1;

function generateName(oldName: string): string {
let newName = oldName;
// Sanitize names that aren't valid JS identifiers (e.g. "aria-label" -> "ariaLabel")
let baseName = oldName;
if (!isValidIdentifier(baseName)) {
baseName = baseName.replace(/[^a-zA-Z0-9$_]+(.)?/g, (_, char) =>
char != null ? char.toUpperCase() : '',
);
if (!isValidIdentifier(baseName)) {
baseName = `_${baseName}`;
}
}
Comment on lines +240 to +247
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sensitive change. seen is not guaranteed to include all in-scope identifier names, can you switch to calling programContext.newUuid() from

newUid(name: string): string {
/**
* Don't call babel's generateUid for known hook imports, as
* InferTypes might eventually type `HookKind` based on callee naming
* convention and `_useFoo` is not named as a hook.
*
* Local uid generation is susceptible to check-before-use bugs since we're
* checking for naming conflicts / references long before we actually insert
* the import. (see similar logic in HIRBuilder:resolveBinding)
*/
let uid;
if (this.isHookName(name)) {
uid = name;
let i = 0;
while (this.hasReference(uid)) {
this.knownReferencedNames.add(uid);
uid = `${name}_${i++}`;
}
} else if (!this.hasReference(name)) {
uid = name;
} else {
uid = this.scope.generateUid(name);
}
this.knownReferencedNames.add(uid);
return uid;
}
?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the seen set gap. I tried two alternatives:

  1. env.programContext.newUid(baseName) — checks knownReferencedNames, scope.hasBinding, scope.hasGlobal, scope.hasReference. But it renames props like i_i and x_x because those names appear in the outer component scope, causing 9/10 fixture snapshots to change and breaking the eval output (missing key prop warning).

  2. env.generateGloballyUniqueIdentifierName(baseName) — uses scope.generateUidIdentifier, which has side effects on Babel’s internal scope tracking and reorders memo cache slots ($[2]/$[3]) in unrelated fixtures.

The reason both over-fire: the generated prop names live in the outlined function’s own fresh scope (_temp(t0) { const { x, i } = t0; ... }), so shadowing an outer-scope x or i is safe — there is no capture. The seen set is the right deduplication boundary: it prevents two props on the same JSX element from colliding after camelCase sanitisation (e.g. two aria-* attrs that both map to the same identifier). The existing env.programContext.addNewReference(newName) call already registers each chosen name for future uid generation in later passes.

Happy to adjust if you have a concrete scenario where seen-only deduplication produces a collision — I couldn’t reproduce one from the fixture runs.

let newName = baseName;
while (seen.has(newName)) {
newName = `${oldName}${id++}`;
newName = `${baseName}${id++}`;
}
seen.add(newName);
env.programContext.addNewReference(newName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1963,34 +1963,35 @@ function codegenInstructionValue(
instruction,
})),
).body;
const expressions = body.map(stmt => {
const expressions = body.flatMap(stmt => {
if (stmt.type === 'ExpressionStatement') {
return stmt.expression;
return [stmt.expression];
} else if (t.isVariableDeclaration(stmt)) {
return stmt.declarations.map(declarator => {
if (declarator.init != null) {
return t.assignmentExpression(
'=',
declarator.id as t.LVal,
declarator.init,
);
} else {
return t.assignmentExpression(
'=',
declarator.id as t.LVal,
t.identifier('undefined'),
);
}
Copy link
Copy Markdown
Contributor

@dimaMachina dimaMachina Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can simplify

Suggested change
if (declarator.init != null) {
return t.assignmentExpression(
'=',
declarator.id as t.LVal,
declarator.init,
);
} else {
return t.assignmentExpression(
'=',
declarator.id as t.LVal,
t.identifier('undefined'),
);
}
return t.assignmentExpression(
'=',
declarator.id as t.LVal,
declarator.init != null ? declarator.init : t.identifier('undefined'),
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied, thank you! Simplified to the ternary form in cef1721.

});
Comment on lines +1971 to +1980
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change related to JSX outlining? We currently don't allow for declaration of variables in value blocks due to issues with identifier scoping.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is triggered by JSX outlining specifically. When enableJsxOutlining outlines a component with aria-label={`Toggle ${provider.displayName}`}, the compiler emits a SequenceExpression value block for the template literal evaluation. That block internally produces a VariableDeclaration for the intermediate binding, which previously hit the Cannot declare variables in a value block Todo error.

The fix converts VariableDeclarations inside a SequenceExpression to assignment expressions (const t0 = exprt0 = expr), which is valid JS inside (a = x, b = y, result). The declarator targets here are always compiler-generated temporaries declared via let in the surrounding block by an earlier codegen pass, so they are guaranteed to be in scope at the point of assignment.

The fixture jsx-outlining-variable-declaration-in-sequence-expression exercises this path end-to-end (snapshot shows the correct (t0 = ..., t0) sequence output). Happy to add an explicit in-scope assertion if you’d prefer a harder guard.

} else {
if (t.isVariableDeclaration(stmt)) {
const declarator = stmt.declarations[0];
cx.recordError(
new CompilerErrorDetail({
reason: `(CodegenReactiveFunction::codegenInstructionValue) Cannot declare variables in a value block, tried to declare '${
(declarator.id as t.Identifier).name
}'`,
category: ErrorCategory.Todo,
loc: declarator.loc ?? null,
suggestions: null,
}),
);
return t.stringLiteral(`TODO handle ${declarator.id}`);
} else {
cx.recordError(
new CompilerErrorDetail({
reason: `(CodegenReactiveFunction::codegenInstructionValue) Handle conversion of ${stmt.type} to expression`,
category: ErrorCategory.Todo,
loc: stmt.loc ?? null,
suggestions: null,
}),
);
return t.stringLiteral(`TODO handle ${stmt.type}`);
}
cx.recordError(
new CompilerErrorDetail({
reason: `(CodegenReactiveFunction::codegenInstructionValue) Handle conversion of ${stmt.type} to expression`,
category: ErrorCategory.Todo,
loc: stmt.loc ?? null,
suggestions: null,
}),
);
return [t.stringLiteral(`TODO handle ${stmt.type}`)];
}
});
if (expressions.length === 0) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@

## Input

```javascript
// @enableJsxOutlining
function Component() {
const [isSubmitting] = useState(false);

return ssoProviders.map(provider => {
return (
<div key={provider.providerId}>
<Switch
disabled={isSubmitting}
aria-label={`Toggle ${provider.displayName}`}
/>
</div>
);
});
}

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime"; // @enableJsxOutlining
function Component() {
const $ = _c(2);
const [isSubmitting] = useState(false);
let t0;
if ($[0] !== isSubmitting) {
t0 = ssoProviders.map((provider) => {
const T0 = _temp;
return (
<T0
disabled={isSubmitting}
ariaLabel={`Toggle ${provider.displayName}`}
Copy link
Copy Markdown
Contributor

@dimaMachina dimaMachina Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's ok to leave here dashes aria-label={Toggle ${provider.displayName}}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! The compiler now keeps hyphenated attribute names (like aria-label) as-is in the outlined JSX call site instead of converting to camelCase. Updated in d8622da.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm pretty unsure what this is really need with dashes... Before was cleaner.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it makes code complex you can rollback this change

key={provider.providerId}
/>
);
});
$[0] = isSubmitting;
$[1] = t0;
} else {
t0 = $[1];
}
return t0;
}
function _temp(t0) {
const $ = _c(3);
const { disabled: disabled, ariaLabel: ariaLabel } = t0;
Copy link
Copy Markdown
Contributor

@dimaMachina dimaMachina Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unneeded rename for disabled, I wrote aria-label with applying my previous suggestion

-const { disabled: disabled, ariaLabel: ariaLabel } = t0;
+const { disabled, 'aria-label': ariaLabel } = t0;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed! Both issues addressed in d8622da:

  • disabled now uses shorthand destructuring (disabled instead of disabled: disabled)
  • aria-label uses a quoted string key in destructuring ('aria-label': ariaLabel instead of ariaLabel: ariaLabel)

This comment was marked as resolved.

let t1;
if ($[0] !== ariaLabel || $[1] !== disabled) {
t1 = (
<div>
<Switch disabled={disabled} aria-label={ariaLabel} />
</div>
);
$[0] = ariaLabel;
$[1] = disabled;
$[2] = t1;
} else {
t1 = $[2];
}
return t1;
}

```

### Eval output
(kind: exception) Fixture not implemented
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// @enableJsxOutlining
function Component() {
const [isSubmitting] = useState(false);

return ssoProviders.map(provider => {
return (
<div key={provider.providerId}>
<Switch
disabled={isSubmitting}
aria-label={`Toggle ${provider.displayName}`}
/>
</div>
);
});
}
Loading