[compiler] Correctly insert (Arrow)FunctionExpressions #30446

poteto · 2024-07-24T17:32:25Z

Stack from ghstack (oldest at bottom):

Previously we would insert new (Arrow)FunctionExpressions as a sibling
of the original function. However this would break in the outlining case
as it would cause the original function expression's parent to become a
SequenceExpression, breaking a bunch of assumptions in the babel plugin.

To get around this, we synthesize a new VariableDeclaration to contain
the newly inserted function expression and therefore insert it as a true
sibling to the original function.

Yeah, it's kinda gross

[ghstack-poisoned]

vercel · 2024-07-24T17:32:27Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-compiler-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 24, 2024 8:02pm

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: 95babb9810dd11adf9585efdd17988ec4b6eda56 Pull Request resolved: #30446

[ghstack-poisoned]

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: aefc2cdb2fa1b2f17fc6b9e5fc22b57ded78fd34 Pull Request resolved: #30446

[ghstack-poisoned]

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: b432abc3143a2a7551b9dd1886d60695f541b071 Pull Request resolved: #30446

[ghstack-poisoned]

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: 8a458917c1c314c5b94426238e9d9575e0b71e69 Pull Request resolved: #30446

mofeiZ · 2024-07-24T18:16:38Z

compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts

+      varDecl.insertAfter(
+        t.variableDeclaration('const', [
+          t.variableDeclarator(compiledFn.id, funcExpr),
+        ]),
+      );
+      CompilerError.invariant(typeof varDecl.key === 'number', {
+        reason:
+          'Just Babel things: expected the VariableDeclaration containing the compiled function expression to have a number key',
+        loc: null,
+      });
+      const insertedCompiledFnVarDecl = varDecl.getSibling(varDecl.key + 1);
+      CompilerError.invariant(
+        insertedCompiledFnVarDecl.isVariableDeclaration(),
+        {
+          reason: 'Expected inserted sibling to be a VariableDeclaration',
+          loc: null,
+        },
+      );


Nit: (please disregard if this is intentional to double check babel-things) I think we might be able to just use the return value of insertAfter instead of going through getSibling etc.

mofeiZ · 2024-07-24T18:17:19Z

...babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlining-in-func-expr.expect.md

+```
+
+### Eval output
+(kind: exception) Fixture not implemented


Nice! Can we add a fixture entrypoint here?

export const FIXTURE_ENTRYPOINT = { fn: Component2, params: [ { items: [ {id: 2, name: 'foo'}, {id: 3, name: 'bar'}, ], }, ], };

good call, thanks!

mofeiZ

Thanks for the fix, this makes sense!

I'm a bit confused on the original outlining function-creation logic added by #30331 -- not sure why we want to insert function declarations / expressions / arrows based on the type of the component / hook function. But that's separate of this

[ghstack-poisoned]

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: cdf36f3913415ebad5a96896812ef58f3c5d0a26 Pull Request resolved: #30446

[ghstack-poisoned]

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: df13e3b439962b95af4bbd82ef4302624668faf7 Pull Request resolved: #30446

Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: df13e3b439962b95af4bbd82ef4302624668faf7 Pull Request resolved: facebook#30446

josephsavona · 2024-07-24T23:32:54Z

Thanks for the fix. I think we can simplify this by always creating the outlined function as a function declaration at the top level of the program. The name is already guaranteed to be unique in the file so this won’t cause conflicts.

josephsavona · 2024-07-24T23:34:55Z

not sure why we want to insert function declarations / expressions / arrows based on the type of the component / hook function. But that's separate of this

I reused the existing logic to create new functions and it didn’t seem to really matter in practice what type of function we created. But yeah we should change this to always produce a FunctionDeclaration for outlined functions.

Addresses follow up feedback from #30446. Since the outlined function is guaranteed to have a module-scoped unique identifier name, we can simplify the insertion logic for the outlined function to always emit a function declaration rather than switch based on the original function type. This is fine because the outlined function is synthetic anyway. ghstack-source-id: 0a4d1f7b0a5a34f8ed68c463b1a153c51c3ea75e Pull Request resolved: #30464

Update

f37cc8a

[ghstack-poisoned]

poteto mentioned this pull request Jul 24, 2024

[compiler] Repro for outlining bug in funcexprs #30443

Merged

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 24, 2024

Update

4eec254

[ghstack-poisoned]

Update

48b11e3

[ghstack-poisoned]

vercel bot deployed to Preview July 24, 2024 17:42 View deployment

poteto requested review from josephsavona, gsathya and mofeiZ July 24, 2024 17:47

Update

f9e0a5f

[ghstack-poisoned]

vercel bot deployed to Preview July 24, 2024 18:15 View deployment

mofeiZ reviewed Jul 24, 2024

View reviewed changes

mofeiZ approved these changes Jul 24, 2024

View reviewed changes

Update

28a407a

[ghstack-poisoned]

vercel bot deployed to Preview July 24, 2024 19:52 View deployment

Update

58fb44a

[ghstack-poisoned]

vercel bot deployed to Preview July 24, 2024 20:02 View deployment

poteto merged commit 58fb44a into gh/poteto/36/base Jul 24, 2024
21 checks passed

poteto deleted the gh/poteto/36/head branch July 24, 2024 20:02

eps1lon mentioned this pull request Jul 25, 2024

Update React from 6230622a1a-20240610 to 76002254-20240724 vercel/next.js#68149

Merged

poteto mentioned this pull request Jul 25, 2024

[compiler] Always emit FuncDecl for outlined functions #30464

Merged

eps1lon mentioned this pull request Jul 30, 2024

Update React from 6230622a1a-20240610 to 3208e73e-20240730 vercel/next.js#68330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] Correctly insert (Arrow)FunctionExpressions #30446

[compiler] Correctly insert (Arrow)FunctionExpressions #30446

poteto commented Jul 24, 2024 •

edited

Loading

vercel bot commented Jul 24, 2024 •

edited

Loading

mofeiZ Jul 24, 2024

mofeiZ Jul 24, 2024

poteto Jul 24, 2024

mofeiZ left a comment

josephsavona commented Jul 24, 2024 •

edited

Loading

josephsavona commented Jul 24, 2024

[compiler] Correctly insert (Arrow)FunctionExpressions #30446

[compiler] Correctly insert (Arrow)FunctionExpressions #30446

Conversation

poteto commented Jul 24, 2024 • edited Loading

vercel bot commented Jul 24, 2024 • edited Loading

mofeiZ Jul 24, 2024

Choose a reason for hiding this comment

mofeiZ Jul 24, 2024

Choose a reason for hiding this comment

poteto Jul 24, 2024

Choose a reason for hiding this comment

mofeiZ left a comment

Choose a reason for hiding this comment

josephsavona commented Jul 24, 2024 • edited Loading

josephsavona commented Jul 24, 2024

poteto commented Jul 24, 2024 •

edited

Loading

vercel bot commented Jul 24, 2024 •

edited

Loading

josephsavona commented Jul 24, 2024 •

edited

Loading