[compiler][eslint] remove compilationMode override; report bailouts on first line by mofeiZ · Pull Request #30423 · facebook/react

mofeiZ · 2024-07-22T20:35:31Z

Stack from ghstack (oldest at bottom):

Before:

observe that both functions are compiled, even though useFoo is not defined using component syntax
eslint diagnostic highlights entire function body

before.mov

After:

compilationMode config is respected
eslint diagnostic highlights only first line

Screen.Recording.2024-07-22.at.3.59.59.PM.mov

…n first line [ghstack-poisoned]

vercel · 2024-07-22T20:35:33Z

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 23, 2024 6:20pm

…n first line ghstack-source-id: 4b6a3b1 Pull Request resolved: #30423

react-sizebot · 2024-07-22T20:40:12Z

Comparing: b2ec044...686b477

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.05%	1.83 kB	1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js	=	499.44 kB	499.44 kB	=	89.59 kB	89.59 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	=	504.26 kB	504.26 kB	=	90.30 kB	90.31 kB
facebook-www/ReactDOM-prod.classic.js	=	599.20 kB	599.20 kB	=	105.79 kB	105.80 kB
facebook-www/ReactDOM-prod.modern.js	=	573.37 kB	573.37 kB	=	101.68 kB	101.68 kB
test_utils/ReactAllWarnings.js	Deleted	64.07 kB	0.00 kB	Deleted	15.86 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	64.07 kB	0.00 kB	Deleted	15.86 kB	0.00 kB

Generated by 🚫 dangerJS against 686b477

mofeiZ · 2024-07-22T20:40:26Z

compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts


 const COMPILER_OPTIONS: Partial<PluginOptions> = {
  noEmit: true,
-  compilationMode: "infer",


Note that infer is already the compilationMode default, so this override only affects user-specified compilationModes (by overriding all other options to infer).

mofeiZ · 2024-07-22T20:42:04Z

compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts

+            const firstLineLoc = {
+              start: event.fnLoc.start,
+              end: {
+                line: event.fnLoc.start.line,
+                column: 10e3,
+              },
+            };


Slightly hacky, but the alternative is to attach another loc object to events (e.g. function identifier, parameters, etc). This felt cleaner, as not all functions we compile have id or param nodes.

Worth noting that in Flow we have a sig_loc on functions, which is the location of, effectively, everything in a function other than its body (function keyword, name, type parameters, parameters including parentheses, => for arrow functions, return type annotation). This has been a useful thing for us to have in a lot of error messages and the compiler might benefit from something similar.

Ohhh i've wanted that a few times

mvitousek · 2024-07-22T22:07:57Z

compiler/packages/eslint-plugin-react-compiler/src/rules/ReactCompilerRule.ts

+            const firstLineLoc = {
+              start: event.fnLoc.start,
+              end: {
+                line: event.fnLoc.start.line,
+                column: 10e3,
+              },
+            };


Worth noting that in Flow we have a sig_loc on functions, which is the location of, effectively, everything in a function other than its body (function keyword, name, type parameters, parameters including parentheses, => for arrow functions, return type annotation). This has been a useful thing for us to have in a lot of error messages and the compiler might benefit from something similar.

… bailouts on first line" --- Before: - observe that both functions are compiled, even though `useFoo` is not defined using [component syntax](https://flow.org/en/docs/react/component-syntax/) - eslint diagnostic highlights entire function body https://github.com/user-attachments/assets/e4a26fa9-cebb-4a44-8789-4810a015a67b After: - `compilationMode` config is respected - eslint diagnostic highlights only first line https://github.com/user-attachments/assets/eb3f9b1a-b204-4f10-a920-d6fd6b821adc [ghstack-poisoned]

…n first line ghstack-source-id: a1e27f4 Pull Request resolved: #30423

… bailouts on first line" --- Before: - observe that both functions are compiled, even though `useFoo` is not defined using [component syntax](https://flow.org/en/docs/react/component-syntax/) - eslint diagnostic highlights entire function body https://github.com/user-attachments/assets/e4a26fa9-cebb-4a44-8789-4810a015a67b After: - `compilationMode` config is respected - eslint diagnostic highlights only first line https://github.com/user-attachments/assets/eb3f9b1a-b204-4f10-a920-d6fd6b821adc [ghstack-poisoned]

…n first line ghstack-source-id: 1870c7a Pull Request resolved: #30423

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

poteto · 2024-08-21T19:22:52Z

Sorry about not chiming in earlier, but I'm not sure I'm following that the fix is to truncate the error location to just the first line. Shouldn't we audit the locations we report instead and ensure that they're scoped to a more specific point in the source (not the component/hook itself)?

In any case, this approach doesn't seem to play well with our internal eslint infra which expects that lint message locations fall within the bounds of the actual source code. I think it'd be better to revert this change and then find the errors that span the whole function and fix their reported locations

…30977) Compiler bailout diagnostics should now highlight only the first line of the source location span. (Resubmission of #30423 which was reverted due to invalid column number.)

…30977) Compiler bailout diagnostics should now highlight only the first line of the source location span. (Resubmission of #30423 which was reverted due to invalid column number.) DiffTrain build for [a99d8e8](a99d8e8)

[compiler][eslint] remove compilationMode override; report bailouts o…

686b477

…n first line [ghstack-poisoned]

mofeiZ mentioned this pull request Jul 19, 2024

[compiler][repro] Test fixture for fbt whitespace bug #30394

Merged

mofeiZ added a commit that referenced this pull request Jul 22, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

c507998

…n first line ghstack-source-id: 4b6a3b1 Pull Request resolved: #30423

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 22, 2024

mofeiZ marked this pull request as ready for review July 22, 2024 20:39

vercel bot deployed to Preview July 22, 2024 20:40 View deployment

mofeiZ commented Jul 22, 2024

View reviewed changes

mvitousek approved these changes Jul 22, 2024

View reviewed changes

josephsavona approved these changes Jul 23, 2024

View reviewed changes

mofeiZ added a commit that referenced this pull request Jul 23, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

7a20846

…n first line ghstack-source-id: a1e27f4 Pull Request resolved: #30423

vercel bot deployed to Preview July 23, 2024 18:14 View deployment

mofeiZ added a commit that referenced this pull request Jul 23, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

e2457fa

…n first line ghstack-source-id: 1870c7a Pull Request resolved: #30423

vercel bot deployed to Preview July 23, 2024 18:20 View deployment

mofeiZ merged commit f3a9dc0 into gh/mofeiZ/14/base Jul 23, 2024

mofeiZ added a commit that referenced this pull request Jul 23, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

b34b750

…n first line ghstack-source-id: 1870c7a Pull Request resolved: #30423

mofeiZ deleted the gh/mofeiZ/14/head branch July 23, 2024 18:24

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

b42c27b

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

4573ef5

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

26d6fc9

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

cf8ebd7

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

d42652d

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024

[compiler][eslint] remove compilationMode override; report bailouts o…

e9ec91c

…n first line ghstack-source-id: 1870c7a Pull Request resolved: facebook#30423

This was referenced Jul 25, 2024

Update React from 6230622a1a-20240610 to 76002254-20240724 vercel/next.js#68149

Merged

Update React from 6230622a1a-20240610 to 3208e73e-20240730 vercel/next.js#68330

Merged

mofeiZ mentioned this pull request Sep 16, 2024

[compiler][eslint] Report bailout diagnostics with correct column # #30977

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler][eslint] remove compilationMode override; report bailouts on first line#30423

[compiler][eslint] remove compilationMode override; report bailouts on first line#30423
mofeiZ merged 3 commits intogh/mofeiZ/14/basefrom
gh/mofeiZ/14/head

mofeiZ commented Jul 22, 2024 •

edited

Loading

Uh oh!

vercel bot commented Jul 22, 2024 •

edited

Loading

Uh oh!

react-sizebot commented Jul 22, 2024

Uh oh!

mofeiZ Jul 22, 2024

Uh oh!

mofeiZ Jul 22, 2024

Uh oh!

mvitousek Jul 22, 2024

Uh oh!

josephsavona Jul 23, 2024

Uh oh!

mvitousek Jul 22, 2024

Uh oh!

poteto commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

mofeiZ commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

react-sizebot commented Jul 22, 2024

Critical size changes

Significant size changes

Uh oh!

mofeiZ Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

mofeiZ Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

mvitousek Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

josephsavona Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

mvitousek Jul 22, 2024

Choose a reason for hiding this comment

Uh oh!

poteto commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mofeiZ commented Jul 22, 2024 •

edited

Loading

vercel bot commented Jul 22, 2024 •

edited

Loading