[compiler] Improved ref validation for non-mutating functions#35893
[compiler] Improved ref validation for non-mutating functions#35893josephsavona merged 1 commit intomainfrom
Conversation
|
cc @lunaleaps |
|
|
||
| function Foo(props, ref) { | ||
| console.log(ref.current); | ||
| mutate(ref.current); |
There was a problem hiding this comment.
console.log() was a bad example here since it's a function we know doesn't mutate it's arguments, so changing to one that might
62feadf to
db77ad9
Compare
If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render. An example is React Native's PanResponder, which is designed for use in interaction handling. Calling `PanResponder.create()` creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze <place> and ImmutableCapture <place> for the same place in the same instruction, we know that it's not being mutated. Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this.
db77ad9 to
5570223
Compare
There was a problem hiding this comment.
Makes sense! Thanks for the thorough comments and PR summary.
We probably already do this in other places, but it took a while to wrap my head around using Frozen to describe internally mutable values that aren't accessed during render. This is a pretty different use-case of Frozen from hook and jsx captures, but it makes sense. This transfers ownership to non-render code, and it has retains the same reordering / observability properties (all references after the Freeze should be reorderable, since the references should be happening in non-render code anyways)
If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render. An example is React Native's PanResponder, which is designed for use in interaction handling. Calling `PanResponder.create()` creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze <place> and ImmutableCapture <place> for the same place in the same instruction, we know that it's not being mutated. Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this. DiffTrain build for [e33071c](e33071c)
If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render. An example is React Native's PanResponder, which is designed for use in interaction handling. Calling `PanResponder.create()` creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze <place> and ImmutableCapture <place> for the same place in the same instruction, we know that it's not being mutated. Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this. DiffTrain build for [e33071c](e33071c)
…ok#35893) If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render. An example is React Native's PanResponder, which is designed for use in interaction handling. Calling `PanResponder.create()` creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze <place> and ImmutableCapture <place> for the same place in the same instruction, we know that it's not being mutated. Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this. DiffTrain build for [e33071c](facebook@e33071c)
…ok#35893) If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render. An example is React Native's PanResponder, which is designed for use in interaction handling. Calling `PanResponder.create()` creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze <place> and ImmutableCapture <place> for the same place in the same instruction, we know that it's not being mutated. Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this. DiffTrain build for [e33071c](facebook@e33071c)
This MR contains the following updates: | Package | Type | Update | Change | OpenSSF | |---|---|---|---|---| | [@typescript-eslint/eslint-plugin](https://typescript-eslint.io/packages/eslint-plugin) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin)) | devDependencies | minor | [`8.58.2` → `8.59.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/8.58.2/8.59.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/typescript-eslint/typescript-eslint) | | [@typescript-eslint/parser](https://typescript-eslint.io/packages/parser) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser)) | devDependencies | minor | [`8.58.2` → `8.59.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/8.58.2/8.59.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/typescript-eslint/typescript-eslint) | | [ajv](https://ajv.js.org) ([source](https://github.com/ajv-validator/ajv)) | dependencies | minor | [`8.18.0` → `8.20.0`](https://renovatebot.com/diffs/npm/ajv/8.18.0/8.20.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/ajv-validator/ajv) | | [eslint-plugin-react-hooks](https://react.dev/) ([source](https://github.com/facebook/react/tree/HEAD/packages/eslint-plugin-react-hooks)) | devDependencies | minor | [`7.0.1` → `7.1.1`](https://renovatebot.com/diffs/npm/eslint-plugin-react-hooks/7.0.1/7.1.1) | [](https://securityscorecards.dev/viewer/?uri=github.com/facebook/react) | | [react-toastify](https://github.com/fkhadra/react-toastify) | dependencies | minor | [`11.0.5` → `11.1.0`](https://renovatebot.com/diffs/npm/react-toastify/11.0.5/11.1.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/fkhadra/react-toastify) | | [typescript-eslint](https://typescript-eslint.io/packages/typescript-eslint) ([source](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/typescript-eslint)) | devDependencies | minor | [`8.58.2` → `8.59.0`](https://renovatebot.com/diffs/npm/typescript-eslint/8.58.2/8.59.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/typescript-eslint/typescript-eslint) | | [vite-plugin-static-copy](https://github.com/sapphi-red/vite-plugin-static-copy) | devDependencies | minor | [`4.0.1` → `4.1.0`](https://renovatebot.com/diffs/npm/vite-plugin-static-copy/4.0.1/4.1.0) | [](https://securityscorecards.dev/viewer/?uri=github.com/sapphi-red/vite-plugin-static-copy) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/eslint-plugin)</summary> ### [`v8.59.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8590-2026-04-20) [Compare Source](typescript-eslint/typescript-eslint@v8.58.2...v8.59.0) ##### 🚀 Features - **eslint-plugin:** \[no-unnecessary-type-assertion] report more cases based on assignability ([#​11789](typescript-eslint/typescript-eslint#11789)) ##### ❤️ Thank You - Ulrich Stark See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.59.0) for more information. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>typescript-eslint/typescript-eslint (@​typescript-eslint/parser)</summary> ### [`v8.59.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#8590-2026-04-20) [Compare Source](typescript-eslint/typescript-eslint@v8.58.2...v8.59.0) This was a version bump only for parser to align it with other projects, there were no code changes. See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.59.0) for more information. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>ajv-validator/ajv (ajv)</summary> ### [`v8.20.0`](https://github.com/ajv-validator/ajv/releases/tag/v8.20.0) [Compare Source](ajv-validator/ajv@v8.18.0...v8.20.0) #### What's Changed - fix: add support for node 22/24, drop node 16/21 by [@​jasoniangreen](https://github.com/jasoniangreen) in [#​2580](ajv-validator/ajv#2580) - fix: add ES2022.RegExp for RegExpIndicesArray by [@​SignpostMarv](https://github.com/SignpostMarv) in [#​2604](ajv-validator/ajv#2604) **Full Changelog**: <ajv-validator/ajv@v8.19.0...v8.20.0> </details> <details> <summary>facebook/react (eslint-plugin-react-hooks)</summary> ### [`v7.1.1`](https://github.com/facebook/react/blob/HEAD/packages/eslint-plugin-react-hooks/CHANGELOG.md#711) [Compare Source](https://github.com/facebook/react/compare/eslint-plugin-react-hooks@7.1.0...eslint-plugin-react-hooks@7.1.1) **Note:** 7.1.0 accidentally removed the `component-hook-factories` rule, causing errors for users who referenced it in their ESLint config. This is now fixed. - Add deprecated no-op `component-hook-factories` rule for backwards compatibility. ([@​mofeiZ](https://github.com/mofeiZ) in [#​36307](facebook/react#36307)) ### [`v7.1.0`](https://github.com/facebook/react/blob/HEAD/packages/eslint-plugin-react-hooks/CHANGELOG.md#710) [Compare Source](https://github.com/facebook/react/compare/408b38ef7304faf022d2a37110c57efce12c6bad...eslint-plugin-react-hooks@7.1.0) This release adds ESLint v10 support, improves performance by skipping compilation for non-React files, and includes compiler lint improvements including better `set-state-in-effect` detection, improved ref validation, and more helpful error reporting. - Add ESLint v10 support. ([@​azat-io](https://github.com/azat-io) in [#​35720](facebook/react#35720)) - Skip compilation for non-React files to improve performance. ([@​josephsavona](https://github.com/josephsavona) in [#​35589](facebook/react#35589)) - Fix exhaustive deps bug with Flow type casting. ([@​jorge-cab](https://github.com/jorge-cab) in [#​35691](facebook/react#35691)) - Fix `useEffectEvent` checks in component syntax. ([@​jbrown215](https://github.com/jbrown215) in [#​35041](facebook/react#35041)) - Improved `set-state-in-effect` validation with fewer false negatives. ([@​jorge-cab](https://github.com/jorge-cab) in [#​35134](facebook/react#35134), [@​josephsavona](https://github.com/josephsavona) in [#​35147](facebook/react#35147), [@​jackpope](https://github.com/jackpope) in [#​35214](facebook/react#35214), [@​chesnokov-tony](https://github.com/chesnokov-tony) in [#​35419](facebook/react#35419), [@​jsleitor](https://github.com/jsleitor) in [#​36107](facebook/react#36107)) - Improved ref validation for non-mutating functions and event handler props. ([@​josephsavona](https://github.com/josephsavona) in [#​35893](facebook/react#35893), [@​kolvian](https://github.com/kolvian) in [#​35062](facebook/react#35062)) - Compiler now reports all errors instead of stopping at the first. ([@​josephsavona](https://github.com/josephsavona) in [#​35873](https://github.com/facebook/react/pull/35873)–[#​35884](https://github.com/facebook/react/pull/35884)) - Improved source locations and error display in compiler diagnostics. ([@​nathanmarks](https://github.com/nathanmarks) in [#​35348](facebook/react#35348), [@​josephsavona](https://github.com/josephsavona) in [#​34963](facebook/react#34963)) </details> <details> <summary>fkhadra/react-toastify (react-toastify)</summary> ### [`v11.1.0`](https://github.com/fkhadra/react-toastify/releases/tag/v11.1.0) [Compare Source](fkhadra/react-toastify@v11.0.5...v11.1.0) ### Release Notes #### Features - **CSP nonce support.** `<ToastContainer nonce={...}>` applies the nonce to the injected `<style>` tag. Closes [#​1209](fkhadra/react-toastify#1209). #### Fixes - `onChange` fires `status: 'removed'` synchronously on `toast.dismiss()` instead of after the exit animation — observers (incl. `useNotificationCenter`) now see correctly ordered events. Also guards against double-`onClose`. Closes [#​1275](fkhadra/react-toastify#1275). - Touch drag no longer re-pauses the toast on release — the old check compared a PointerEvent against `'touchend'`, which never matched. Closes [#​1217](fkhadra/react-toastify#1217). - Vertical drag now visually moves the toast (`--y` gets a unit). Thanks [@​janpaepke](https://github.com/janpaepke), [#​1277](fkhadra/react-toastify#1277). - Stacked scale is clamped at 0.5, preventing zero/negative scale in deep stacks. Closes [#​1171](fkhadra/react-toastify#1171), [#​1174](fkhadra/react-toastify#1174). - Stacked container respects mobile `100vw` again. Closes [#​1234](fkhadra/react-toastify#1234). #### Accessibility - `role="progressbar"` now includes `aria-valuenow`, `aria-valuemin`, `aria-valuemax`. Thanks [@​singhankit001](https://github.com/singhankit001), [#​1283](fkhadra/react-toastify#1283). Closes [#​1259](fkhadra/react-toastify#1259). #### Internal - Migrated to a pnpm workspace (`pnpm link .` no longer required for contributors). Publish layout unchanged — addon still ships inside the main package. - CSS now injected at mount via `useStyleSheet` (prerequisite for `nonce`). - Dep bumps: TypeScript 6, Vite 8, Cypress 15, React 19.2, plus the rest. - CI: `upload-artifact` v3 → v4. Thanks to [@​janpaepke](https://github.com/janpaepke), [@​singhankit001](https://github.com/singhankit001), and reporters of the fixed issues. </details> <details> <summary>typescript-eslint/typescript-eslint (typescript-eslint)</summary> ### [`v8.59.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/typescript-eslint/CHANGELOG.md#8590-2026-04-20) [Compare Source](typescript-eslint/typescript-eslint@v8.58.2...v8.59.0) This was a version bump only for typescript-eslint to align it with other projects, there were no code changes. See [GitHub Releases](https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.59.0) for more information. You can read about our [versioning strategy](https://typescript-eslint.io/users/versioning) and [releases](https://typescript-eslint.io/users/releases) on our website. </details> <details> <summary>sapphi-red/vite-plugin-static-copy (vite-plugin-static-copy)</summary> ### [`v4.1.0`](https://github.com/sapphi-red/vite-plugin-static-copy/blob/HEAD/CHANGELOG.md#410) [Compare Source](https://github.com/sapphi-red/vite-plugin-static-copy/compare/vite-plugin-static-copy@4.0.1...vite-plugin-static-copy@4.1.0) ##### Minor Changes - [#​251](sapphi-red/vite-plugin-static-copy#251) [`7672842`](sapphi-red/vite-plugin-static-copy@7672842) Thanks [@​sapphi-red](https://github.com/sapphi-red)! - Add `name` property to the `rename` object form and allow rename functions to return a `RenameObject`. The `name` property replaces the file's basename (filename + extension), and can be combined with `stripBase` to both flatten directory structure and rename the file in one step. Rename functions can now return `{ name, stripBase }` objects instead of only strings, making it easier to declaratively control output paths from dynamic rename logic. ```js // node_modules/lib/dist/index.js → vendor/lib.js { src: 'node_modules/lib/dist/index.js', dest: 'vendor', rename: { name: 'lib.js', stripBase: true } } // src/pages/events/test.html → dist/events/index.html { src: 'src/pages/**/*.html', dest: 'dist/', rename: { stripBase: 2, name: 'index.html' } } ``` </details> --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Mend Renovate](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMjkuMCIsInVwZGF0ZWRJblZlciI6IjQzLjE0MS4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZSJdfQ==--> See merge request swiss-armed-forces/cyber-command/cea/loom!480 Co-authored-by: Loom MR Pipeline Trigger <group_103951964_bot_9504bb8dead6d4e406ad817a607f24be@noreply.gitlab.com>
If a function is known to freeze its inputs, and captures refs, then we can safely assume those refs are not mutated during render.
An example is React Native's PanResponder, which is designed for use in interaction handling. Calling
PanResponder.create()creates an object that shouldn't be interacted with at render time, so we can treat it as freezing its arguments, returning a frozen value, and not accessing any refs in the callbacks passed to it. ValidateNoRefAccessInRender is updated accordingly - if we see a Freeze and ImmutableCapture for the same place in the same instruction, we know that it's not being mutated.Note that this is a pretty targeted fix. One weakness is that we may not always emit a Freeze effect if a value is already frozen, which could cause this optimization not to kick in. The worst case there is that you'd just get a ref access in render error though, not miscompilation. And we could always choose to always emit Freeze effects, even for frozen values, just to retain the information for validations like this.