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] Delete propagateScopeDeps (non-hir) #31198

Closed
wants to merge 3 commits into from
Closed

[compiler] Delete propagateScopeDeps (non-hir) #31198

wants to merge 3 commits into from

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Oct 11, 2024

enablePropagateScopeDepsHIR is now used extensively in Meta. This has been tested for over two weeks in our e2e tests and production.

The rest of this stack deletes LoweredFunction.dependencies, which the non-hir version of PropagateScopeDeps depends on. To avoid a more forked HIR (non-hir with dependencies and hir with no dependencies), let's go ahead and clean up the non-hir version of PropagateScopeDepsHIR.

Note that all fixture changes in this PR were previously reviewed when they were copied to propagate-scope-deps-hir-fork. Will clean up / merge these duplicate fixtures in a later PR

'

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link

vercel bot commented Oct 11, 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 Oct 25, 2024 6:42pm

Extends #31066 to ObjectMethods (somehow missed this before).

'
`PropertyPathRegistry` is responsible for uniqueing identifier and property paths. This is necessary for the hoistability CFG merging logic which takes unions and intersections of these nodes to determine a basic block's hoistable reads, as a function of its neighbors. We also depend on this to merge optional chained and non-optional chained property paths

This fixes a small bug in #31066 in which we create a new registry for nested functions. Now, we use the same registry for a component / hook and all its inner functions

'
`enablePropagateScopeDepsHIR` is now used extensively in Meta. This has been tested for over two weeks in our e2e tests and production.

The rest of this stack deletes `LoweredFunction.dependencies`, which the non-hir version of `PropagateScopeDeps` depends on. To avoid a more forked HIR (non-hir with dependencies and hir with no dependencies), let's go ahead and clean up the non-hir version of PropagateScopeDepsHIR.

Note that all fixture changes in this PR were previously reviewed when they were copied to `propagate-scope-deps-hir-fork`. Will clean up / merge these duplicate fixtures in a later PR

'
@mofeiZ mofeiZ changed the title [compiler][waittocommit] default to enablePropagateScopeDepsHIR [compiler] Delete propagateScopeDeps (non-hir) Oct 25, 2024
@mofeiZ mofeiZ marked this pull request as ready for review November 5, 2024 18:52
@mofeiZ
Copy link
Contributor Author

mofeiZ commented Nov 5, 2024

Closing as this is a duplicate of #31199

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.

2 participants