Skip to content

Conversation

josephsavona
Copy link
Member

@josephsavona josephsavona commented Jun 26, 2024

Stack from ghstack (oldest at bottom):

Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:

  1. Build up a mapping of context variables in the outer component/hook
  2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
  3. Propagate aliases of those functions via StoreLocal/LoadLocal
  4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

Copy link

vercel bot commented Jun 26, 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 Jun 26, 2024 11:07pm

@react-sizebot
Copy link

react-sizebot commented Jun 26, 2024

Comparing: ef0f44e...9f6ff54

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB +0.11% 1.82 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB +0.11% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 597.10 kB 597.10 kB = 105.31 kB 105.31 kB
facebook-www/ReactDOM-prod.modern.js = 571.44 kB 571.44 kB = 101.24 kB 101.24 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against 9f6ff54

…render"

Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 26, 2024
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

ghstack-source-id: 88eb900
Pull Request resolved: #30107
@josephsavona josephsavona marked this pull request as ready for review June 26, 2024 22:10
…render"

Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 26, 2024
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

ghstack-source-id: a1afaee
Pull Request resolved: #30107
…g reassigned after render"

Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

[ghstack-poisoned]
josephsavona added a commit that referenced this pull request Jun 26, 2024
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

ghstack-source-id: c2b22e3
Pull Request resolved: #30107
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.

Looks good--as discussed, this case is still problematic; will add this fixture

@josephsavona josephsavona merged commit 9f6ff54 into gh/josephsavona/33/base Jun 26, 2024
josephsavona added a commit that referenced this pull request Jun 26, 2024
Adds a pass which validates that local variables are not reassigned by functions which may be called after render. This is a straightforward forward data-flow analysis, where we:
1. Build up a mapping of context variables in the outer component/hook
2. Find ObjectMethod/FunctionExpressions which may reassign those context variables
3. Propagate aliases of those functions via StoreLocal/LoadLocal
4. Disallow passing those functions with a Freeze effect. This includes JSX arguments, hook arguments, hook return types, etc.

Conceptually, a function that reassigns a local is inherently mutable. Frozen functions must be side-effect free, so these two categories are incompatible and we can use the freeze effect to find all instances of where such functions are disallowed rather than special-casing eg hook calls and JSX.

ghstack-source-id: c2b22e3
Pull Request resolved: #30107
@josephsavona josephsavona deleted the gh/josephsavona/33/head branch June 26, 2024 23:51
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.

4 participants