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

Report fatal parsing errors differently than standard errors #13711

Closed
tosmolka opened this issue Sep 23, 2020 · 18 comments
Closed

Report fatal parsing errors differently than standard errors #13711

tosmolka opened this issue Sep 23, 2020 · 18 comments

Comments

@tosmolka
Copy link

@tosmolka tosmolka commented Sep 23, 2020

eslint v7.9.0

We have eslint integrated in our CI/CD pipelines and we would like to distinguish between cases when linting was successful and when linting failed due to fatal errors during parsing. Such errors are often caused by misconfigured parser or by trying to lint files with syntax errors.

Sample error:

{
    "ruleId": null,
    "fatal": true,
    "severity": 2,
    "message": "Parsing error: 'import' and 'export' may appear only with 'sourceType: module'",
    "line": 1,
    "column": 1
}

We see such cases as unsuccessful linting and would like to propose exiting with exit code 2. This seems in line with what can be read in the Exit codes docs:

When linting files, ESLint will exit with one of the following exit codes:

0: Linting was successful and there are no linting errors. If the --max-warnings flag is set to n, the number of linting warnings is at most n.
1: Linting was successful and there is at least one linting error, or there are more linting warnings than allowed by the --max-warnings option.
2: Linting was unsuccessful due to a configuration problem or an internal error.

If eslint starts returning 2 we could fail early in the pipeline without parsing the result files (which we do only in later stages) and let the user know that linting was actually not successful and configs need to be changed (e.g. update parser options or ignore invalid files).

Alternatively, we could consider introducing new exit code 3 that would cover just this case.

Can you please review the proposal and let us know? We might have capacity for contribution but we would like to agree on the approach first. Thanks.

@nzakas
Copy link
Member

@nzakas nzakas commented Sep 25, 2020

Thanks for the suggestion. I don’t think we can change the meaning of exit code 2 at this point, as that would be fairly unexpected to a lot of systems. Right now 2 effectively means ESLint crashed and changing that has consequences.

Adding a 3 that is less severe than 2 also doesn’t seem like a good idea.

An alternative approach could be to add a command line option so you could opt-in to the behavior you’re describing. I’m not opposed to something like that as I think reporting parsing errors in a different way can make sense in some situations.

Just FYI: a proposal like this would need to go through our RFC process, and I’d wait for more feedback from the team before starting that.

@tosmolka
Copy link
Author

@tosmolka tosmolka commented Sep 25, 2020

@nzakas , thank you for the feedback, I agree with your point of view. What about introducing option called --max-fatal-errors? This feels somehow consistent with --max-warnings option that is already there and won't change behavior unless you use it.

Sample proposal for docs to better describe what I meant:

--max-fatal-errors

This option allows you to specify a fatal error threshold, which can be used to force ESLint to exit with an error code 3 if there are too many errors in your project that are considered fatal.

Normally, ESLint treats fatal and non-fatal errors in the same way and exits with error code 1 if there is at least one. Error code 1 code indicates that linting was successful but fatal errors are often parsing errors that effectively prevented the linting. Many are caused by misconfigurations and are fixable. This threshold enables ESLint to explicitly warn user with an error message and different exit code.

Example:

eslint --max-fatal-errors 0 file.js

file.js
  1:1  error  Parsing error: The keyword 'import' is reserved

✖ 1 problem (1 error, 0 warnings)

ESLint found too many fatal parsing errors (maximum: 0).

I'll be waiting for the feedback, thank you.

@tosmolka tosmolka changed the title Return exit code 2 when there are fatal errors Report fatal parsing errors differently than standard errors Sep 25, 2020
@nzakas
Copy link
Member

@nzakas nzakas commented Sep 25, 2020

That could work. We need to get some more feedback from the team before proceeding to the RFC phase.

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented Sep 30, 2020

Does parsing errors come under internal error? if yes, I guess we can go with the exit code 2 itself (breaking change)?
Otherwise, the --max-fatal-errors do looks like a good approach.
What would be the exit code if both --max-fatal-errors and --max-warnings is triggered? 3 ?

tosmolka added a commit to tosmolka/eslint that referenced this issue Sep 30, 2020
@tosmolka
Copy link
Author

@tosmolka tosmolka commented Sep 30, 2020

I think keeping a split between ESLint internal errors (exit code 2) and parser errors makes sense - ESLint is not responsible for what custom parser is doing.

I did a quick & dirty implementation of the proposal (mainly as an exercise): master...tosmolka:max-fatal-errors . In there --max-fatal-errors takes precedence over --max-warnings and when both are triggered, ESLint shows warning message (ESLint found too many warnings...) but exits with 3.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 30, 2020

I agree with @nzakas that we shouldn't change the default behavior.

Parsing error can be just an error in the code being linted (like a missing } somewhere in the code, or anything else) in which case treating it as a linting error with exit code 1 seems reasonable. Checking and reporting syntax errors is usually considered as a part of the linting process. So, I think that 1: Linting was successful and there is at least one linting error is generally correct default behavior.

An option to change this behavior if a different exit code would be more suitable for some use cases makes sense to me.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 30, 2020

I think --max-fatal-errors would be confusing for users.

It has a similar name as --max-warnings, but --max-warnings makes a difference between success and failure while --max-fatal-errors only changes the severity of a failure. I'd expect exit code 0 when there are no more than max fatal errors.

Maybe we could add a boolean option instead of a threshold number?

Also, 3 usually indicates a more severe error than 2, while this is at most equal to scenarios that produce exit code 2, so the same exit code 2 might make more sense than introducing a new exit code 3?

@tosmolka
Copy link
Author

@tosmolka tosmolka commented Oct 26, 2020

@nzakas, @anikethsaha, @mdjermanovic, @mysticatea, any updates regarding the proposal? I think open questions need some attention from eslint team. We are ok with starting to return 2 when this new option is used and linting fails due to fatal/parsing errors. Boolean option would work as well, users might not need the granularity of --max-fatal-errors.

@btmills
Copy link
Member

@btmills btmills commented Oct 29, 2020

Like @nzakas I'm not opposed to an opt-in option to distinguish parsing errors from regular linting errors. I'm not thrilled with --max-fatal-errors for the reasons @mdjermanovic mentioned. What about something like --exit-parsing-errors? That seems more obviously related to an exit code for parsing errors. It could either hard-code 3 or accept a configurable number.

@A-Katopodis
Copy link
Contributor

@A-Katopodis A-Katopodis commented Nov 26, 2020

To add in this discussion I do agree that having this functionality would be better to have it as an argument/opt-in option as @nzakas mentioned.

I do agree more with the suggestion by @mdjermanovic that having a bool options and simply fail in case of parser errors does make more sense from a user standpoint.

For the names I would go for something like --break-on-parse-error, strict-parsing, fail-on-parse-error.

Are there any plans on picking up this functionality?

@nzakas
Copy link
Member

@nzakas nzakas commented Nov 27, 2020

@A-Katopodis while this isn't on the official roadmap (meaning that the core team won't be working on it), we are open to adding it if someone in the community would like to develop it. The next step would be to create an RFC with a concrete proposal that we can review.

@eslint-deprecated
Copy link

@eslint-deprecated eslint-deprecated bot commented Dec 28, 2020

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jan 28, 2021

@nzakas
Copy link
Member

@nzakas nzakas commented Jan 29, 2021

@mdjermanovic i think you meant to reopen this.

@nzakas nzakas reopened this Jan 29, 2021
@nzakas nzakas added this to RFC Opened in Triage Jan 29, 2021
@nzakas nzakas moved this from RFC Opened to Ready to Implement in Triage Mar 30, 2021
@nzakas nzakas removed the evaluating label Mar 30, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Mar 30, 2021

RFC has been merged and this is now ready to implement.

@nzakas
Copy link
Member

@nzakas nzakas commented Jun 8, 2021

@A-Katopodis just checking in to see if you intend to implement this?

@A-Katopodis
Copy link
Contributor

@A-Katopodis A-Katopodis commented Jun 9, 2021

@nzakas I do plan starting the implementation next week. So you can expect it in 1-2 weeks time.

@nzakas
Copy link
Member

@nzakas nzakas commented Jun 10, 2021

Great, thanks!

@nzakas nzakas linked a pull request that will close this issue Jun 26, 2021
1 task
@nzakas nzakas moved this from Ready to Implement to Pull Request Opened in Triage Jun 26, 2021
Triage automation moved this from Pull Request Opened to Complete Jul 30, 2021
btmills pushed a commit that referenced this issue Jul 30, 2021
* Docs: Updated docs for exit-on-fatal-error

* New: Added exit-on-fatal-error feature

* Addressed PR feedback

* Made description consistent

* Updated dev docs for nodejs-api

* Updated cmd docs

* Added test case for 1 exit code

* Fixed typos

Co-authored-by: A-Katopodis <ankatopo@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
Complete
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants