[compiler] Aggregate error reporting, separate eslint rules #34176
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
React Core Team
josephsavona
changed the title
josephsavona
force-pushed
the
pr34176
branch
11 times, most recently
from
fbaad2c to
3e5400f
Compare
josephsavona
commented
Aug 20, 2025
|
|
||
| class OkImpl<T> implements Result<T, never> { | ||
| constructor(private val: T) {} | ||
| #val: T; |
There was a problem hiding this comment.
I think this is the only place we use this syntax and rollup didn't like it, and it was easier to switch to private identifiers than deal with more babel plugins
josephsavona
commented
Aug 20, 2025
Comment on lines
+24
to
+26
| ...Object.fromEntries( | ||
| Object.keys(recommendedRules).map(name => ['react-hooks/' + name, 'error']), | ||
| ), |
There was a problem hiding this comment.
but by default only the recommended rules are activated. and this is all driven dynamically based on the categories
josephsavona
commented
Aug 20, 2025
Comment on lines
+836
to
+849
| default: { | ||
| assertExhaustive(category, `Unsupported category ${category}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export const LintRules: Array<LintRule> = Object.keys(ErrorCategory).map( | ||
| category => getRuleForCategory(category as any), | ||
| ); |
There was a problem hiding this comment.
The getRuleForCategory() function with its exhaustive switch, and the dynamic iteration over ErrorCategory, mean we statically enforce that we have an ESLint rule for every category. LintRules is what powers the set of rules in the plugin.
poteto
approved these changes
Aug 20, 2025
There was a problem hiding this comment.
Assuming the cache does work and can be shared across our multiple rules, lgtm! thanks so much for this work @josephsavona and @mofeiZ!
| entry.sourceCode === sourceCode.text && | ||
| isDeepStrictEqual(entry.userOpts, userOpts) | ||
| ) { | ||
| return entry; |
There was a problem hiding this comment.
just curious, can we include a test plan in the PR showing that we do get cache hits?
There was a problem hiding this comment.
I verified locally that we see one cache miss followed by lots of cache hits, so it looks like this is working
josephsavona
force-pushed
the
pr34176
branch
2 times, most recently
from
b969d98 to
7752f95
Compare
josephsavona
force-pushed
the
pr34176
branch
4 times, most recently
from
53bcd0d to
5a8b1c8
Compare
NOTE: this is a merged version of @mofeiZ's original PR along with my edits per offline discussion. The description is updated to reflect the latest approach. The key problem we're trying to solve with this PR is to allow developers more control over the compiler's various validations. The idea is to have a number of rules targeting a specific category of issues, such as enforcing immutability of props/state/etc or disallowing access to refs during render. We don't want to have to run the compiler again for every single rule, though, so @mofeiZ added an LRU cache that caches the full compilation output of N most recent files. The first rule to run on a given file will cause it to get cached, and then subsequent rules can pull from the cache, with each rule filtering down to its specific category of errors. For the categories, I went through and assigned a category roughly 1:1 to existing validations, and then used my judgement on some places that felt distinct enough to warrant a separate error. Every error in the compiler now has to supply both a severity (for legacy reasons) and a category (for ESLint). Each category corresponds 1:1 to a ESLint rule definition, so that the set of rules is automatically populated based on the defined categories. Categories include a flag for whether they should be in the recommended set or not. Note that as with the original version of this PR, only eslint-plugin-react-compiler is changed. We still have to update the main lint rule.
|
In case this gets lost on the next Sapling push: Test Plan
|
github-actions bot
pushed a commit
that referenced
this pull request
Aug 21, 2025
NOTE: this is a merged version of @mofeiZ's original PR along with my edits per offline discussion. The description is updated to reflect the latest approach. The key problem we're trying to solve with this PR is to allow developers more control over the compiler's various validations. The idea is to have a number of rules targeting a specific category of issues, such as enforcing immutability of props/state/etc or disallowing access to refs during render. We don't want to have to run the compiler again for every single rule, though, so @mofeiZ added an LRU cache that caches the full compilation output of N most recent files. The first rule to run on a given file will cause it to get cached, and then subsequent rules can pull from the cache, with each rule filtering down to its specific category of errors. For the categories, I went through and assigned a category roughly 1:1 to existing validations, and then used my judgement on some places that felt distinct enough to warrant a separate error. Every error in the compiler now has to supply both a severity (for legacy reasons) and a category (for ESLint). Each category corresponds 1:1 to a ESLint rule definition, so that the set of rules is automatically populated based on the defined categories. Categories include a flag for whether they should be in the recommended set or not. Note that as with the original version of this PR, only eslint-plugin-react-compiler is changed. We still have to update the main lint rule. ## Test Plan * Created a sample project using ESLint v9 and verified that the plugin can be configured correctly and detects errors * Edited `fixtures/eslint-v9` and introduced errors, verified that the w latest config changes in that fixture it correctly detects the errors * In the sample project, confirmed that the LRU caching is correctly caching compiler output, ie compiling files just once. Co-authored-by: Mofei Zhang <feifei0@meta.com> DiffTrain build for [7d29ecb](7d29ecb)
github-actions bot
pushed a commit
that referenced
this pull request
Aug 21, 2025
NOTE: this is a merged version of @mofeiZ's original PR along with my edits per offline discussion. The description is updated to reflect the latest approach. The key problem we're trying to solve with this PR is to allow developers more control over the compiler's various validations. The idea is to have a number of rules targeting a specific category of issues, such as enforcing immutability of props/state/etc or disallowing access to refs during render. We don't want to have to run the compiler again for every single rule, though, so @mofeiZ added an LRU cache that caches the full compilation output of N most recent files. The first rule to run on a given file will cause it to get cached, and then subsequent rules can pull from the cache, with each rule filtering down to its specific category of errors. For the categories, I went through and assigned a category roughly 1:1 to existing validations, and then used my judgement on some places that felt distinct enough to warrant a separate error. Every error in the compiler now has to supply both a severity (for legacy reasons) and a category (for ESLint). Each category corresponds 1:1 to a ESLint rule definition, so that the set of rules is automatically populated based on the defined categories. Categories include a flag for whether they should be in the recommended set or not. Note that as with the original version of this PR, only eslint-plugin-react-compiler is changed. We still have to update the main lint rule. ## Test Plan * Created a sample project using ESLint v9 and verified that the plugin can be configured correctly and detects errors * Edited `fixtures/eslint-v9` and introduced errors, verified that the w latest config changes in that fixture it correctly detects the errors * In the sample project, confirmed that the LRU caching is correctly caching compiler output, ie compiling files just once. Co-authored-by: Mofei Zhang <feifei0@meta.com> DiffTrain build for [7d29ecb](7d29ecb)
1 task
poteto
added a commit
that referenced
this pull request
Sep 5, 2025
With #34176 we now have granular lint rules created for each compiler ErrorCategory. However, we had remnants of our old error severities still in use which makes reporting errors quite clunky. Previously you would need to specify both a category and severity which often ended up being the same. This PR moves severity definition into our rules which are generated from our categories. For now I decided to defer "upgrading" categories from a simple string to a sum type since we are only using severities to map errors to eslint severity.
poteto
added a commit
that referenced
this pull request
Sep 5, 2025
With #34176 we now have granular lint rules created for each compiler ErrorCategory. However, we had remnants of our old error severities still in use which makes reporting errors quite clunky. Previously you would need to specify both a category and severity which often ended up being the same. This PR moves severity definition into our rules which are generated from our categories. For now I decided to defer "upgrading" categories from a simple string to a sum type since we are only using severities to map errors to eslint severity.
poteto
added a commit
that referenced
this pull request
Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler ErrorCategory. However, we had remnants of our old error severities still in use which makes reporting errors quite clunky. Previously you would need to specify both a category and severity which often ended up being the same. This PR moves severity definition into our rules which are generated from our categories. For now I decided to defer "upgrading" categories from a simple string to a sum type since we are only using severities to map errors to eslint severity. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401). * #34409 * #34404 * #34403 * #34402 * __->__ #34401
github-actions bot
pushed a commit
that referenced
this pull request
Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler ErrorCategory. However, we had remnants of our old error severities still in use which makes reporting errors quite clunky. Previously you would need to specify both a category and severity which often ended up being the same. This PR moves severity definition into our rules which are generated from our categories. For now I decided to defer "upgrading" categories from a simple string to a sum type since we are only using severities to map errors to eslint severity. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401). * #34409 * #34404 * #34403 * #34402 * __->__ #34401 DiffTrain build for [60d9b97](60d9b97)
github-actions bot
pushed a commit
that referenced
this pull request
Sep 6, 2025
With #34176 we now have granular lint rules created for each compiler ErrorCategory. However, we had remnants of our old error severities still in use which makes reporting errors quite clunky. Previously you would need to specify both a category and severity which often ended up being the same. This PR moves severity definition into our rules which are generated from our categories. For now I decided to defer "upgrading" categories from a simple string to a sum type since we are only using severities to map errors to eslint severity. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401). * #34409 * #34404 * #34403 * #34402 * __->__ #34401 DiffTrain build for [60d9b97](60d9b97)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: this is a merged version of @mofeiZ's original PR along with my edits per offline discussion. The description is updated to reflect the latest approach.
The key problem we're trying to solve with this PR is to allow developers more control over the compiler's various validations. The idea is to have a number of rules targeting a specific category of issues, such as enforcing immutability of props/state/etc or disallowing access to refs during render. We don't want to have to run the compiler again for every single rule, though, so @mofeiZ added an LRU cache that caches the full compilation output of N most recent files. The first rule to run on a given file will cause it to get cached, and then subsequent rules can pull from the cache, with each rule filtering down to its specific category of errors.
For the categories, I went through and assigned a category roughly 1:1 to existing validations, and then used my judgement on some places that felt distinct enough to warrant a separate error. Every error in the compiler now has to supply both a severity (for legacy reasons) and a category (for ESLint). Each category corresponds 1:1 to a ESLint rule definition, so that the set of rules is automatically populated based on the defined categories.
Categories include a flag for whether they should be in the recommended set or not.
Note that as with the original version of this PR, only eslint-plugin-react-compiler is changed. We still have to update the main lint rule.
Test Plan
fixtures/eslint-v9and introduced errors, verified that the w latest config changes in that fixture it correctly detects the errors