Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: Known hooks/nonescaping scopes dont count as pruned #29820

Merged
merged 3 commits into from
Jun 10, 2024
Merged
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 @@ -66,6 +66,16 @@
this.visitScope(scope, innerState);
outerState.hasHook ||= innerState.hasHook;
if (innerState.hasHook) {
if (scope.instructions.length === 1) {
// This was a scope just for a hook call, which doesn't need memoization.

Check failure on line 70 in compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts

View workflow job for this annotation

GitHub Actions / Lint babel-plugin-react-compiler

Expected a block comment instead of consecutive line comments
// flatten it away
return {
kind: "replace-many",
value: scope.instructions,
};
}
// else this scope had multiple instructions and produced some other value:

Check failure on line 77 in compiler/packages/babel-plugin-react-compiler/src/ReactiveScopes/FlattenScopesWithHooksOrUse.ts

View workflow job for this annotation

GitHub Actions / Lint babel-plugin-react-compiler

Expected a block comment instead of consecutive line comments
// mark it as pruned
return {
kind: "replace",
value: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -951,12 +951,8 @@ class PruneScopesTransform extends ReactiveFunctionTransform<
} else {
this.prunedScopes.add(scopeBlock.scope.id);
return {
kind: "replace",
value: {
kind: "pruned-scope",
scope: scopeBlock.scope,
instructions: scopeBlock.instructions,
},
kind: "replace-many",
value: scopeBlock.instructions,
};
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@

## Input

```javascript
// @flow
function Component() {
return (
<div
className={stylex(
// this value is a) in its own scope, b) non-reactive, and c) non-escaping
// its scope gets pruned bc it's non-escaping, but this doesn't mean we need to
// create a temporary for it
flags.feature("feature-name") ? styles.featureNameStyle : null
)}
></div>
);
}

```

## Code

```javascript
import { c as _c } from "react/compiler-runtime";
function Component() {
const $ = _c(1);
let t0;
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
t0 = (
<div
className={stylex(
flags.feature("feature-name") ? styles.featureNameStyle : null,
)}
/>
);
Comment on lines +28 to +35
Copy link
Contributor Author

@josephsavona josephsavona Jun 8, 2024

Choose a reason for hiding this comment

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

This output matches what we have on main today. Whereas with the previous PR in the stack the output was:

const t0 = flags.feature("feature-name") ? styles.featureNameStyle : null;
  let t1;

  if ($[0] !== t0) {
    t1 = stylex(t0);
    $[0] = t0;
    $[1] = t1;
  } else {
    t1 = $[1];
  }

  let t2;

  if ($[2] !== t1) {
    t2 = <div className={t1} />;
    $[2] = t1;
    $[3] = t2;
  } else {
    t2 = $[3];
  }

  return t2;

Way more code and 4 cache slots instead of 1.

$[0] = t0;
} else {
t0 = $[0];
}
return t0;
}

```

### Eval output
(kind: exception) Fixture not implemented
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// @flow
function Component() {
return (
<div
className={stylex(
// this value is a) in its own scope, b) non-reactive, and c) non-escaping
// its scope gets pruned bc it's non-escaping, but this doesn't mean we need to
// create a temporary for it
flags.feature("feature-name") ? styles.featureNameStyle : null
)}
></div>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export const FIXTURE_ENTRYPOINT = {
```javascript
import { c as _c } from "react/compiler-runtime";
function Component(props) {
const $ = _c(2);
const $ = _c(1);

const a = [];
const b = [];
Expand All @@ -53,12 +53,11 @@ function Component(props) {
x = 1;
}
let t0;
if ($[0] !== x) {
if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

x is non-escaping (and therefore counted as pruned) and non-reactive. before this PR, we would have promoted x to reactive because of the pruned+nonreactive combination — now it doesn't count as pruned so we revert back to the output from main

t0 = [x];
$[0] = x;
$[1] = t0;
$[0] = t0;
} else {
t0 = $[1];
t0 = $[0];
}
return t0;
}
Expand Down
Loading