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][eslint] remove compilationMode override; report bailouts on first line #30423

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Jul 22, 2024

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

Copy link

vercel bot commented Jul 22, 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 23, 2024 6:20pm

mofeiZ added a commit that referenced this pull request Jul 22, 2024
…n first line

ghstack-source-id: 4b6a3b181d4c3eb2f2e11b22455066c57aa2e018
Pull Request resolved: #30423
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 22, 2024
@mofeiZ mofeiZ marked this pull request as ready for review July 22, 2024 20:39
@react-sizebot
Copy link

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

@@ -100,7 +100,6 @@ function makeSuggestions(

const COMPILER_OPTIONS: Partial<PluginOptions> = {
noEmit: true,
compilationMode: "infer",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +163 to +169
const firstLineLoc = {
start: event.fnLoc.start,
end: {
line: event.fnLoc.start.line,
column: 10e3,
},
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh i've wanted that a few times

Comment on lines +163 to +169
const firstLineLoc = {
start: event.fnLoc.start,
end: {
line: event.fnLoc.start.line,
column: 10e3,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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]
mofeiZ added a commit that referenced this pull request Jul 23, 2024
…n first line

ghstack-source-id: a1e27f431afc692b1b30738443bcfad3c9677568
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]
mofeiZ added a commit that referenced this pull request Jul 23, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: #30423
@mofeiZ mofeiZ merged commit f3a9dc0 into gh/mofeiZ/14/base Jul 23, 2024
21 checks passed
mofeiZ added a commit that referenced this pull request Jul 23, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: #30423
@mofeiZ 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
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
…n first line

ghstack-source-id: 1870c7aa09d455bea89171b77468baeef5b3a63b
Pull Request resolved: facebook#30423
@poteto
Copy link
Member

poteto commented Aug 21, 2024

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

mofeiZ added a commit that referenced this pull request Sep 16, 2024
…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.)
github-actions bot pushed a commit that referenced this pull request Sep 16, 2024
…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)
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.

6 participants