-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat!: check for parsing errors in suggestion fixes #101
Conversation
c4a3930
to
0fc6808
Compare
5243c16
to
8203128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an oversight not to have been in suggestions in the first place, tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought part of the point of suggestion fixers is that they could. Intentionally produce half-baked, unsafe, or broken fixers because they had to be manually applied by the user.
I don't know of any fixers that specifically do this though.
@bradzacher I agree that suggestions should be allowed to provide incomplete or unsafe fixes, but I don't think suggestions should be allowed to provide syntactically-invalid/unparsable fixes. Producing broken code puts an unnecessary burden on the user to figure out how to un-break it and prevents ESLint and other tools from running on the code in the meantime. There should be a way to turn any desired suggestion into valid code/syntax, even if it's still incomplete or not production ready. I have updated the RFC to mention this. |
c146f9b
to
4d20ba5
Compare
you can remove this section. | ||
--> | ||
|
||
1. Should we check for parsing errors when applying suggestions in core too (not just in rule tester)? Checking for parsing errors in suggestions doesn't have the same performance concerns that it would have for autofixes, as suggestions are applied one-at-a-time vs. autofixes which can be applied in bulk. Showing an exception to end-users about the parsing error could be more clear than simply allowing the suggestion to produce broken code for them, and could encourage the end-user to file a ticket to have the issue fixed with the plugin author. However, this additional assertion may be of limited value, as the vast majority of invalid suggestions would have already been caught by rule tests (as long as the rule actually has tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think performance concerns are the same because ESLint doesn't apply suggestions so if we want to warn about parse errors in advance, then we'd have to parse fixed versions of the code at the time the suggestions are created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By performance concerns, I was referring to how autofixes can be applied in bulk (e.g. potentially thousands at a time when running on a large codebase) and so any extra validation applied to autofixes could actually add up to a meaningful slowdown. Whereas, even if there's overhead from adding validation to a suggestion, it's still only a single suggestion.
Regardless, I lean toward not bothering with this validation in core. Autofixes and suggestions should both be held to the same validation standards, i.e. we can check them both in rule tester but not in core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's the same problem. If linting produces thousands of suggestions we'd need to additionally parse a thousand times as we don't know which of them will be applied and we don't get any notice when a suggestion is applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad, had a misconception about that. So as of now, only planning to check for parsing errors in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it doesn't makes sense to validate in the core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to reflect this decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. The more up-front validation we can do for rules via RuleTester
, the better off everyone is.
you can remove this section. | ||
--> | ||
|
||
1. Should we check for parsing errors when applying suggestions in core too (not just in rule tester)? Checking for parsing errors in suggestions doesn't have the same performance concerns that it would have for autofixes, as suggestions are applied one-at-a-time vs. autofixes which can be applied in bulk. Showing an exception to end-users about the parsing error could be more clear than simply allowing the suggestion to produce broken code for them, and could encourage the end-user to file a ticket to have the issue fixed with the plugin author. However, this additional assertion may be of limited value, as the vast majority of invalid suggestions would have already been caught by rule tests (as long as the rule actually has tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it doesn't makes sense to validate in the core.
4d20ba5
to
f411892
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
FWIW, I agree with this. The end result of apply any suggestion should be valid syntax even if the fix itself might be invalid on its own. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I agree that 1) suggestions should always be syntactically valid even if, in context, they might not always be the right solution, and 2) we don't need to validate this at runtime.
Looks like we've got a few days left before we can move to Final Commenting, so placing a bookmark to come back and do that. |
We've passed the minimum Initial Commenting period, so moved to Final Commenting! |
Oops, looks like we forgot to merge this. Approved! |
* main: chore: Add Mastodon status post to workflow (eslint#110) feat: ESLint Language Plugins (eslint#99) feat: support for testing invalid rule schemas and runtime exceptions (eslint#103) feat!: check for parsing errors in suggestion fixes (eslint#101)
Summary
Check for parsing errors in suggestion fixes when running rule tests, the same way we do with rule autofix output.
Related Issues