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][rfc] Hacky retry pipeline for fire #32164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Jan 22, 2025

Hacky retry pipeline for when transforming fire(...) calls encounters validation, todo, or memoization invariant bailouts. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)

Some observations:

  • Compiler "front-end" passes (e.g. lower, type, effect, and mutability inferences) should be shared for all compiler features -- memo and otherwise
  • Many passes (anything dealing with reactive scope ranges, scope blocks / dependencies, and optimizations such as ReactiveIR [compiler] Early sketch of ReactiveIR #31974) can be left out of the retry pipeline. This PR hackily skips memoization features by removing reactive scope creation, but we probably should restructure the pipeline to skip these entirely on a retry
  • We should maintain a canonical set of "validation flags"

Note the newly added fixtures are prefixed with bailout-... when the retry fire pipeline is used. These fixture outputs contain correctly inserted useFire calls and no memoization.

Start of a simple retry pipeline. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)
Comment on lines +29 to +41
8 | useEffect(() => {
> 9 | fire(foo(props), bar);
| ^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received multiple arguments (9:9)

InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received a spread argument (10:10)

InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (11:11)

InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (12:12)
10 | fire(...foo);
11 | fire(bar);
12 | fire(props.foo());
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we continue to emit fire-related bailouts

Comment on lines +27 to +42
// @enableFire
import { fire } from "react";

function Component({ props, bar }) {
"use no memo";
const foo = () => {
console.log(props);
};
useEffect(() => {
fire(foo(props));
fire(foo());
fire(bar());
});

return null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a todo fixture because "use no memo" directives probably should directly invoke the retry pipeline (e.g. no memo, no validation, just lower -> transformFire -> codegen)

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

awesome!

@josephsavona
Copy link
Contributor

Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be:

  • Full: transform, validate, memoize. This is the default today.
  • Transform: Along the lines of the backup mode in this PR. Only applies transforms that do not require following the rules of React, like fire().
  • Validate: This could be used for ESLint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants