Skip to content

Conversation

@mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Jul 30, 2024

Stack from ghstack (oldest at bottom):


(pasted from test fixture comments)
Note that the fbt transform looks for callsites with a fbt-named callee. This is incompatible with react-compiler as we rename local variables in HIRBuilder + RenameVariables.

@vercel
Copy link

vercel bot commented Jul 30, 2024

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 30, 2024 1:29am

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 30, 2024
@mofeiZ mofeiZ marked this pull request as ready for review July 30, 2024 01:24
Copy link
Contributor

@mvitousek mvitousek left a comment

Choose a reason for hiding this comment

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

Thank you for pushing so hard to deal with this annoying transform!

This stack looks good, but I wanted to check if we've thought about running fbt before forget, and if that would help matters at all -- I know the intent is that forget is the first transform in the pipeline, but maybe that doesn't make sense with macro-like transforms; in languages with macro systems they'll run before a compiler typically, after all.

@mofeiZ
Copy link
Contributor Author

mofeiZ commented Jul 30, 2024

Thank you for pushing so hard to deal with this annoying transform!

This stack looks good, but I wanted to check if we've thought about running fbt before forget, and if that would help matters at all -- I know the intent is that forget is the first transform in the pipeline, but maybe that doesn't make sense with macro-like transforms; in languages with macro systems they'll run before a compiler typically, after all.

@mvitousek That's a good point, thank you for bringing this up! I think there's a few reasons we've avoided doing so, so far:

  • Macro transforms often produce additional "cruft" that we don't want Forget to operate on (e.g. we should be optimizing close-to-source). I think an example here might be idx (recently deprecated internally), and maybe even fbt (which produces fbt._("__FBT__<encoded table>__FBT__") calls).
  • Some of these transforms also require additional invariants to be preserved after the transform (for example, we have a Hack transform that runs after babel that decodes ___FBT___ strings; stylex attaches metadata fields to nodes, etc). Most of these additional invariants are likely to be preserved by Forget, but I don't think it's necessarily guaranteed.
  • There's not just one or two macro-like transforms, but probably on the scale of 5-10 (see our internal config file of macro names). These are currently not ordered to be the first to run. We could reorder them to the front + compare transpilation outputs to validate, but I'm not sure how much work this entails

@mofeiZ mofeiZ merged commit f9259fb into gh/mofeiZ/14/base Jul 30, 2024
mofeiZ added a commit that referenced this pull request Jul 30, 2024
ghstack-source-id: ec256a3
Pull Request resolved: #30523
@mofeiZ mofeiZ deleted the gh/mofeiZ/14/head branch July 30, 2024 20:32
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