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 global mutation effects for objects passed as props #30458

Closed
wants to merge 4 commits into from

Conversation

gsathya
Copy link
Contributor

@gsathya gsathya commented Jul 25, 2024

Stack from ghstack (oldest at bottom):

With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

[ghstack-poisoned]
Copy link

vercel bot commented Jul 25, 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 25, 2024 1:14pm

gsathya added a commit that referenced this pull request Jul 25, 2024
With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

ghstack-source-id: 0b20e2bc244302d4b4a65d494021dd1ecdd04cc1
Pull Request resolved: #30458
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 25, 2024
…ed as props"

With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

[ghstack-poisoned]
gsathya added a commit that referenced this pull request Jul 25, 2024
With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

ghstack-source-id: 4a318a55d7c5544c5ff2205fe6337777ed2a84da
Pull Request resolved: #30458
…ed as props"

With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

[ghstack-poisoned]
gsathya added a commit that referenced this pull request Jul 25, 2024
With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

ghstack-source-id: 738ed70ed2a9e204155d1f75f03357bf6f923ba0
Pull Request resolved: #30458

for (const effect of propEffects) {
if (effect.kind === 'GlobalMutation') {
functionEffects.delete(effect);
Copy link
Contributor Author

@gsathya gsathya Jul 25, 2024

Choose a reason for hiding this comment

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

This is the key line, rest of the PR is about switching from Array to Set

…ed as props"

With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

[ghstack-poisoned]
gsathya added a commit that referenced this pull request Jul 25, 2024
With the tracked list of effects from the previous PR, we delete the
global mutation effects of object expressions that are passed as props.

To make this deletion easier, this PR changes the FunctionEffects to be
a Set.

While using the identity of the effect for comparison may seem less than
ideal, adding extra infrastructure for this purpose doesn't seem
worthwhile for now.

ghstack-source-id: 0ef1609a6e37d01cb20bebbfb7ff1244b01fa031
Pull Request resolved: #30458
@gsathya
Copy link
Contributor Author

gsathya commented Jul 25, 2024

I considered other types not just object expressions, but we can't really be sure they're not synchronously invoking the captured function. Only object expressions seem safe.

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.

I'm not sure that deleting the effect makes sense to me here. In particular, just because the GlobalMutation effect shouldn't be triggered by passing to JSX doesn't mean that it can't be triggered elsewhere--consider

// @flow

let global = 42;

component Foo() {
  const x = { f() { global = 23 } };
  x.f();
  return <Bar x={x} />;
}

I believe this approach would result in no error being raised here, which is definitely not what we want!

I wonder if instead it would make sense for objects (and arrays?) to track the effects of their properties separately, just like functions themselves do; when we reference the properties we'd add the effects to the effects of the object rather than the enclosing function, and we'd then add the effects to the function when we reference the object -- unless we're referencing it in a JSX prop, just like how we handle function expressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants