Skip to content

Commit ba288b2

Browse files
committed
[compiler] Avoid bailouts when inserting gating
This change fixes a coverage hole in rolling out with `gating`. Prior to this PR, configuring `gating` causes React Compiler to bail out of optimizing some functions. This means that it's not entirely safe to cutover from `gating` enabled for all users (i.e. rolled out 100%) to removing the `gating` config altogether, as new functions may be opted into compilation when they stop bailing out due to gating-specific logic. This is technically slightly slower due to the additional function indirection. An alternative approach is to recommend running a codemod to insert `use no memo`s on currently-bailing out functions before removing the`gating` config. --- Tested [internally]( https://fburl.com/diff/q982ovua) by enabling on a page that previously had a few hundred bailouts due to gating + hoisted function declarations and (1) clicking around locally and (2) running a bunch of e2e tests
1 parent 0a7da4c commit ba288b2

17 files changed

+525
-93
lines changed

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

Lines changed: 116 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,117 @@ import * as t from '@babel/types';
1010
import {PluginOptions} from './Options';
1111
import {CompilerError} from '../CompilerError';
1212

13+
/**
14+
* Gating rewrite for function declarations which are referenced before their
15+
* declaration site.
16+
*
17+
* ```js
18+
* // original
19+
* export default React.memo(Foo);
20+
* function Foo() { ... }
21+
*
22+
* // React compiler optimized + gated
23+
* import {gating} from 'myGating';
24+
* export default React.memo(Foo);
25+
* const gating_result = gating(); <- inserted
26+
* function Foo_optimized() {} <- inserted
27+
* function Foo_unoptimized() {} <- renamed from Foo
28+
* function Foo() { <- inserted function, which can be hoisted by JS engines
29+
* if (gating_result) return Foo_optimized();
30+
* else return Foo_unoptimized();
31+
* }
32+
* ```
33+
*/
34+
function insertAdditionalFunctionDeclaration(
35+
fnPath: NodePath<t.FunctionDeclaration>,
36+
compiled: t.FunctionDeclaration,
37+
gating: NonNullable<PluginOptions['gating']>,
38+
): void {
39+
const originalFnName = fnPath.node.id;
40+
const originalFnParams = fnPath.node.params;
41+
const compiledParams = fnPath.node.params;
42+
/**
43+
* Note that other than `export default function() {}`, all other function
44+
* declarations must have a binding identifier. Since default exports cannot
45+
* be referenced, it's safe to assume that all function declarations passed
46+
* here will have an identifier.
47+
* https://tc39.es/ecma262/multipage/ecmascript-language-functions-and-classes.html#sec-function-definitions
48+
*/
49+
CompilerError.invariant(originalFnName != null && compiled.id != null, {
50+
reason:
51+
'Expected function declarations that are referenced elsewhere to have a named identifier',
52+
loc: fnPath.node.loc ?? null,
53+
});
54+
CompilerError.invariant(originalFnParams.length === compiledParams.length, {
55+
reason:
56+
'Expected React Compiler optimized function declarations to have the same number of parameters as source',
57+
loc: fnPath.node.loc ?? null,
58+
});
59+
60+
const gatingCondition = fnPath.scope.generateUidIdentifier(
61+
`${gating.importSpecifierName}_result`,
62+
);
63+
const unoptimizedFnName = fnPath.scope.generateUidIdentifier(
64+
`${originalFnName.name}_unoptimized`,
65+
);
66+
const optimizedFnName = fnPath.scope.generateUidIdentifier(
67+
`${originalFnName.name}_optimized`,
68+
);
69+
/**
70+
* Step 1: rename existing functions
71+
*/
72+
compiled.id.name = optimizedFnName.name;
73+
fnPath.get('id').replaceInline(unoptimizedFnName);
74+
75+
/**
76+
* Step 2: insert new function declaration
77+
*/
78+
const newParams: Array<t.Identifier | t.RestElement> = [];
79+
const genNewArgs: Array<() => t.Identifier | t.SpreadElement> = [];
80+
for (let i = 0; i < originalFnParams.length; i++) {
81+
const argName = `arg${i}`;
82+
if (originalFnParams[i].type === 'RestElement') {
83+
newParams.push(t.restElement(t.identifier(argName)));
84+
genNewArgs.push(() => t.spreadElement(t.identifier(argName)));
85+
} else {
86+
newParams.push(t.identifier(argName));
87+
genNewArgs.push(() => t.identifier(argName));
88+
}
89+
}
90+
// insertAfter called in reverse order of how nodes should appear in program
91+
fnPath.insertAfter(
92+
t.functionDeclaration(
93+
originalFnName,
94+
newParams,
95+
t.blockStatement([
96+
t.ifStatement(
97+
gatingCondition,
98+
t.returnStatement(
99+
t.callExpression(
100+
compiled.id,
101+
genNewArgs.map(fn => fn()),
102+
),
103+
),
104+
t.returnStatement(
105+
t.callExpression(
106+
unoptimizedFnName,
107+
genNewArgs.map(fn => fn()),
108+
),
109+
),
110+
),
111+
]),
112+
),
113+
);
114+
fnPath.insertBefore(
115+
t.variableDeclaration('const', [
116+
t.variableDeclarator(
117+
gatingCondition,
118+
t.callExpression(t.identifier(gating.importSpecifierName), []),
119+
),
120+
]),
121+
);
122+
fnPath.insertBefore(compiled);
123+
}
13124
export function insertGatedFunctionDeclaration(
14125
fnPath: NodePath<
15126
t.FunctionDeclaration | t.ArrowFunctionExpression | t.FunctionExpression
@@ -21,13 +132,13 @@ export function insertGatedFunctionDeclaration(
21132
gating: NonNullable<PluginOptions['gating']>,
22133
referencedBeforeDeclaration: boolean,
23134
): void {
24-
if (referencedBeforeDeclaration) {
25-
CompilerError.throwTodo({
26-
reason: `Encountered a function used before its declaration, which breaks Forget's gating codegen due to hoisting`,
27-
description: `Rewrite the reference to ${fnPath.node.id?.name ?? 'this function'} to not rely on hoisting to fix this issue`,
135+
if (referencedBeforeDeclaration && fnPath.isFunctionDeclaration()) {
136+
CompilerError.invariant(compiled.type === 'FunctionDeclaration', {
137+
reason: 'Expected compiled node type to match input type',
138+
description: `Got ${compiled.type} but expected FunctionDeclaration`,
28139
loc: fnPath.node.loc ?? null,
29-
suggestions: null,
30140
});
141+
insertAdditionalFunctionDeclaration(fnPath, compiled, gating);
31142
} else {
32143
const gatingExpression = t.conditionalExpression(
33144
t.callExpression(t.identifier(gating.importSpecifierName), []),
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @flow @gating
6+
import {Stringify} from 'shared-runtime';
7+
import * as React from 'react';
8+
9+
component Foo(ref: React.RefSetter<Controls>) {
10+
return <Stringify ref={ref} />;
11+
}
12+
13+
export const FIXTURE_ENTRYPOINT = {
14+
fn: eval('(...args) => React.createElement(Foo, args)'),
15+
params: [{ref: React.createRef()}],
16+
};
17+
18+
```
19+
20+
## Code
21+
22+
```javascript
23+
import { isForgetEnabled_Fixtures } from "ReactForgetFeatureFlag";
24+
import { c as _c } from "react/compiler-runtime";
25+
import { Stringify } from "shared-runtime";
26+
import * as React from "react";
27+
28+
const Foo = React.forwardRef(Foo_withRef);
29+
const _isForgetEnabled_Fixtures_result = isForgetEnabled_Fixtures();
30+
function _Foo_withRef_optimized(_$$empty_props_placeholder$$, ref) {
31+
const $ = _c(2);
32+
let t0;
33+
if ($[0] !== ref) {
34+
t0 = <Stringify ref={ref} />;
35+
$[0] = ref;
36+
$[1] = t0;
37+
} else {
38+
t0 = $[1];
39+
}
40+
return t0;
41+
}
42+
function _Foo_withRef_unoptimized(
43+
_$$empty_props_placeholder$$: $ReadOnly<{}>,
44+
ref: React.RefSetter<Controls>,
45+
): React.Node {
46+
return <Stringify ref={ref} />;
47+
}
48+
function Foo_withRef(arg0, arg1) {
49+
if (_isForgetEnabled_Fixtures_result)
50+
return _Foo_withRef_optimized(arg0, arg1);
51+
else return _Foo_withRef_unoptimized(arg0, arg1);
52+
}
53+
54+
export const FIXTURE_ENTRYPOINT = {
55+
fn: eval("(...args) => React.createElement(Foo, args)"),
56+
params: [{ ref: React.createRef() }],
57+
};
58+
59+
```
60+
61+
### Eval output
62+
(kind: ok) <div>{"ref":null}</div>
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// @flow @gating
2+
import {Stringify} from 'shared-runtime';
3+
import * as React from 'react';
4+
5+
component Foo(ref: React.RefSetter<Controls>) {
6+
return <Stringify ref={ref} />;
7+
}
8+
9+
export const FIXTURE_ENTRYPOINT = {
10+
fn: eval('(...args) => React.createElement(Foo, args)'),
11+
params: [{ref: React.createRef()}],
12+
};

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.component-syntax-ref-gating.flow.expect.md

Lines changed: 0 additions & 24 deletions
This file was deleted.

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.component-syntax-ref-gating.flow.js

Lines changed: 0 additions & 4 deletions
This file was deleted.

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.gating-hoisting.expect.md

Lines changed: 0 additions & 26 deletions
This file was deleted.

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.gating-hoisting.js

Lines changed: 0 additions & 5 deletions
This file was deleted.

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.gating-use-before-decl.expect.md

Lines changed: 0 additions & 24 deletions
This file was deleted.

compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/gating/error.gating-use-before-decl.js

Lines changed: 0 additions & 5 deletions
This file was deleted.
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
2+
## Input
3+
4+
```javascript
5+
// @gating
6+
import {createRef, forwardRef} from 'react';
7+
import {Stringify} from 'shared-runtime';
8+
9+
const Foo = forwardRef(Foo_withRef);
10+
function Foo_withRef(props, ref) {
11+
return <Stringify ref={ref} {...props} />;
12+
}
13+
14+
export const FIXTURE_ENTRYPOINT = {
15+
fn: eval('(...args) => React.createElement(Foo, args)'),
16+
params: [{prop1: 1, prop2: 2, ref: createRef()}],
17+
};
18+
19+
```
20+
21+
## Code
22+
23+
```javascript
24+
import { isForgetEnabled_Fixtures } from "ReactForgetFeatureFlag";
25+
import { c as _c } from "react/compiler-runtime"; // @gating
26+
import { createRef, forwardRef } from "react";
27+
import { Stringify } from "shared-runtime";
28+
29+
const Foo = forwardRef(Foo_withRef);
30+
const _isForgetEnabled_Fixtures_result = isForgetEnabled_Fixtures();
31+
function _Foo_withRef_optimized(props, ref) {
32+
const $ = _c(3);
33+
let t0;
34+
if ($[0] !== props || $[1] !== ref) {
35+
t0 = <Stringify ref={ref} {...props} />;
36+
$[0] = props;
37+
$[1] = ref;
38+
$[2] = t0;
39+
} else {
40+
t0 = $[2];
41+
}
42+
return t0;
43+
}
44+
function _Foo_withRef_unoptimized(props, ref) {
45+
return <Stringify ref={ref} {...props} />;
46+
}
47+
function Foo_withRef(arg0, arg1) {
48+
if (_isForgetEnabled_Fixtures_result)
49+
return _Foo_withRef_optimized(arg0, arg1);
50+
else return _Foo_withRef_unoptimized(arg0, arg1);
51+
}
52+
53+
export const FIXTURE_ENTRYPOINT = {
54+
fn: eval("(...args) => React.createElement(Foo, args)"),
55+
params: [{ prop1: 1, prop2: 2, ref: createRef() }],
56+
};
57+
58+
```
59+
60+
### Eval output
61+
(kind: ok) <div>{"0":{"prop1":1,"prop2":2,"ref":{"current":null}},"ref":"[[ cyclic ref *3 ]]"}</div>

0 commit comments

Comments
 (0)