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

New: Recoverable Error Handling #19

Merged
merged 5 commits into from
Jun 17, 2019
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Mar 29, 2019

Link to Rendered RFC.

Summary

ESLint cannot verify source code if the code has a syntax error. However, we can make valid AST even if several kinds of syntax errors existed. For example, conflict of variable names doesn't break AST. This RFC calls such a syntax error as "Recoverable Errors".

This RFC adds handling of Recoverable Errors into ESLint.

Related Issues

@mysticatea mysticatea added feature Initial Commenting This RFC is in the initial feedback stage labels Mar 29, 2019
@btmills
Copy link
Member

btmills commented Apr 1, 2019

I like the idea! It seems fairly straightforward. On possible addition - since rules might not handle an invalid AST in the presence of recoverable errors, and an incorrect fix could make things worse, should ESLint skip applying fixes if there are recoverable errors?

@not-an-aardvark
Copy link
Member

I generally like the idea, but I'm also concerned about how this would affect rules. In addition to what @btmills mentioned about autofixing, a rule might simply crash if certain invariants about the AST are violated. It seems like it could be confusing for separation of responsibility if a rule needs to fix bugs that pertain to invalid ASTs.

For example, if a parser started raising "recoverable" errors for missing closing parentheses rather than "unrecoverable" errors, I suspect a significant number of rules would start to crash because they assume all parentheses have a match. Would those rules need to be updated to address the new case from the parser? With some rules, it's not clear that there's a well-defined reasonable behavior when invalid syntax occurs.

Maybe one possibility would be to simply swallow rule crashes when a recoverable parser error occurs (i.e. effectively providing linting on a "best-effort" basis for that case).

- It describes autofix.
- It describes exceptions which are caused by recoverable errors.
- It describes the scope of this feature; especially, this feature
  doesn't intend to support invalid AST.
@mysticatea
Copy link
Member Author

Thank you for the great feedback. I added some description.

  • If the parser returned any recoverable errors, the Linter disables autofix as making disableFixes option true internally because it's considered not safe.
  • If the parser returned any recoverable errors, the Linter catches exceptions which were thrown from rules and reports the exceptions as regular messages (e.g., "'a-rule' failed to lint the code because of syntax error(s)") rather than crash. If a rule has both an exception and regular messages, the Linter drops the regular messages to avoid confusion of wrong messages.

@mysticatea mysticatea added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels May 23, 2019
@ilyavolodin
Copy link
Member

On one hand, I like the idea, and I advocated for it before. On the other, I agree with others that it might cause confusion with rules. Also disabling autofix on the whole file just because of one small error in the beginning of the file, might confuse the end user.

@kaicataldo
Copy link
Member

Thanks for writing this up!

While I understand why this could improve the user experience in some cases, I’m not convinced the value this feature adds outweighs the extra complexity introduced. I think we can eat the maintenance cost, but I am concerned with how this might introduce confusion and make it harder for end users to understand why something may or may not be working (i.e. understanding what a “recoverable error” is requires understanding some of the underlying implementation of what an AST is and what makes it valid or usable).

Additionally, I think end users will have to immediately fix the “recoverable error” anyway to ensure their linting results are 100% correct, so while it might surface some linting errors sooner, I’m not sure it would end up saving time in the end.

@mysticatea mysticatea added Initial Commenting This RFC is in the initial feedback stage and removed Final Commenting This RFC is in the final week of commenting labels Jun 7, 2019
@mysticatea
Copy link
Member Author

Hi. Thank you for your feedback.

I have updated this RFC to follow the feedback on the TSC meeting.

  • By default, ESLint doesn't run rules if recoverable errors existed. Instead, ESLint shows those recoverable errors. Practically, this is the support for multiple syntax errors.
  • verifyOnRecoverableParsingErrors option was added. This option is a core option rather than a parser option because it doesn't change the parser behavior. It changes how the Linter handles recoverable errors.
    If this option was given, ESLint runs rules even if recoverable errors existed. Then it shows both results.

Copy link
Member

@platinumazure platinumazure left a 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, thanks! Maybe others will have objections, though.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Nice work @mysticatea! LGTM. As a direction for future work, your suggestion about treating ES version errors as recoverable is intriguing. My most popular answer on Stack Overflow is explaining how to resolve a parsing error on async/await by setting the correct version. I’d be willing to sacrifice a source of recurring Internet points for a better user experience!

This RFC doesn't contain the update of custom parsers. But this section considers if some popular custom parsers can applicate this feature.

- `babel-eslint`<br>
I don't have enough knowledge about `babel-eslint` and recoverable errors.
Copy link
Member

Choose a reason for hiding this comment

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

@kaicataldo is there anything in this design that would preclude compatibility with Babel?

Copy link
Member

Choose a reason for hiding this comment

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

I think in worst case, babel will just continue to work exactly as it does now, by throwing unrecoverable error.

Copy link
Member

Choose a reason for hiding this comment

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

I’ll have to dig deeper for a more definitive answer, but if I’m reading this RFC correctly it would continue to work as it does now (as @ilyavolodin mentions above). I’ll try to take a look sometime over the next few days.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, a bit late to the party, but I looked over babel-parser's code and confirmed with the team that babel-parser does not currently support recoverable errors, however a member of the team did mention that they would be supportive of adding that feature if it's needed for babel-eslint. As of right now, I think we should move forward assuming babel-eslint will continue working as it does now.

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

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

With the new changes, this RFC looks good to me. Thanks.

@mysticatea mysticatea added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Jun 8, 2019
@kaicataldo
Copy link
Member

Here’s an example of how an incomplete AST can affect rules: typescript-eslint/typescript-eslint#441

@kaicataldo
Copy link
Member

kaicataldo commented Jun 15, 2019

After the TSC discussion, I was under the impression that we weren’t going to pursue linting with recoverable errors at all, but rather we were going to list all syntax errors until a file was no longer parseable. I still have the same concerns about being able to lint with recoverable errors as before.

Edit: Are we committing to updating all core rules to handle recoverable errors if we accept this RFC?

@mysticatea
Copy link
Member Author

mysticatea commented Jun 16, 2019

Multiple members had mentioned the option to configure how ESLint handle recoverable errors.

I believe we don't fix rules for invalid AST. But fixes rules for valid AST.

@mysticatea
Copy link
Member Author

About typescript-eslint/typescript-eslint#441, that looks like a valid AST, so we should fix our rules. ESTree spec doesn't guarantee that VariableDeclaration#declarations has 1 declarator at least.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@mysticatea
Copy link
Member Author

I'll merge this RFC. Thank you!

@mysticatea mysticatea merged commit e02058a into master Jun 17, 2019
@mysticatea mysticatea deleted the handling-recoverable-errors branch June 17, 2019 07:04
mysticatea added a commit to eslint/js that referenced this pull request Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants