New: Break on parsing errors

[A-Katopodis]

Summary

The suggested change is for ESLint to support a parameter that will break the ESLint run when it finds configuration errors. The option will be an opt-in argument called --break-on-error.

Related Issues

eslint/eslint#13711

[eslint-deprecated]

Hi @A-Katopodis!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[eslint-deprecated]

Hi @A-Katopodis!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag must be one of the following:

  The Tag is one of the following:

  • Fix - for a bug fix.
  • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
  • New - implements a new feature.
  • Breaking - for a backwards-incompatible enhancement or feature.
  • Docs - changes to documentation only.
  • Build - changes to build process only.
  • Upgrade - for a dependency upgrade.
  • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

  You can use the labels of the issue you are working on to determine the best tag.

• There should be a space following the initial tag and colon, for example 'New: Message'.

Read more about contributing to ESLint here

[ljharb]

I’m confused; eslint already exits nonzero when there’s a parsing error (or any lint error), so shouldn’t this already be the default behavior?

[A-Katopodis]

@ljharb The issue that this cannot be differentiated. As you mentioned it exits with no zero but if you specified a wrong type of sourceType in the parserOptions it would still exit as 1 instead of 2

Also updated the RFC to include some more information and examples found in the linked issue

[ljharb]

Your OP mentions CI; generally all nonzero exit codes are treated identically by everything but humans (they’re used for debugging, mainly). Is this RFC just asking for a distinct nonzero exit code for reports with linting errors and parsing errors, versus reports with linting errors but without parsing errors?

[A-Katopodis]

The exit code exists it's 2 but the behavior is kinda different. For example if we run eslint against this piece of code which has an obvious syntax error:
if()

As you can see from the image the Exit Code is reported as 1 despite the parsing error it produces.

This piece of code gives 1:
if()

This piece of code gives 1 again due to a found rule (no-unused-vars):
var foo = 2;

Furthermore this behavior is the same when eslint is not able to read tsconfig.json ( for example wrong path by the user, which should be a misconfiguration) are again reported as exit code 1.
So there is no distinction if eslint actually managed to go through the file and run any rules and rules due to the fact that even on parsing errors it reports with exit code 1.

What I would suggest in my opinion is using the already existing code 2

From the ESLint docs:

When a user passes the flag (--break-on-error or another name) it considers parsing errors as unsuccessful linting (Currently they are considered successful linting) and exits with code 2.

Of course we can create another exit code for this specific use case but since eslint reporting with a different exit code for some cases by default I would still keep the opt-in flag (--break-on-error)

[nzakas]

A parsing error results in a exit code of 1 because ESLint can continue linting other files. An exit code of 2 is used when ESLint needs to exit because it can't continue. (Parsing errors are currently considered linting errors.)

Also, note that ESLint doesn't read tsconfig.json, it's the TypeScript plugin that does that, and it's trying to play nice with ESLint by not throwing an error. You could probably open an issue on the plugin asking for that behavior to change.

[nzakas]

I'm not sure I understand the need for this option, especially if the main use case is when tsconfig.json fails to load, which ESLint itself doesn't do.

Additionally, you've left out a section for alternative approaches to solving the same problem. Please add that section and address the questions I've left, and we can see if there's support on the team for moving forward.

[nzakas]

Thanks for updating the RFC. I’m still not sure I understand the tsconfig.json use case. Can you explain that? It seems like maybe that’s something typescript-eslint could handle on its own?

[mdjermanovic]

Should this option apply to all fatal: true linting messages?

Aside from parsing errors, there is at least one other case where ESLint produces linting messages with fatal: true - when a configuration comment cannot be JSON-parsed (though this is also some sort of a parsing error).

[A-Katopodis]

In my opinion should be to all fatal: true messages.

Any type of error (coming from invalid JSON, invalid sourceType or a parser error) that does not allow the file to be linted and the rules to run should be reported.

[mdjermanovic]

ESLint will still lint a file with an invalid configuration comment. It can be said that the file wasn't successfully linted because the configuration comment wasn't applied, but there will be some inconsistencies.

For example:

/* eslint no-undef: ["error" */ // <-- missing ']', invalid JSON 

/* eslint no-void: ["error"] */

void a;

/* eslint no-undef: ["error", "foo"] */ // <-- invalid option

/* eslint no-void: ["error"] */

void a;

/* eslint no-undeff: ["error"] */ // <-- typo, this rule doesn't exist

/* eslint no-void: ["error"] */

void a;

In all three examples, ESLint lints the file and reports a no-void linting error. It also reports a linting error for the invalid configuration comment on the first line in all three examples, but only in the first example the error has fatal: true.

[paulsmithkc]

The behavior in those three cases makes sense to me, in terms of severity.

[A-Katopodis]

Added more clear explanations about the tsconfig.json and added the alternative approach section

[nzakas]

Thanks for the additional detail, I left a couple other comments. I'm still not 100% on board, but I'm starting to see how this could be useful. Interested in what other team members think.

[A-Katopodis]

Thanks @nzakas if anything else is needed let me know. I would also like to add that I would be glad to implement this change.

[nzakas]

I think the only open question I have is whether --fatal-parse-error makes sense as the flag name. It seems a bit unclear because all parse errors are flagged as fatal, so maybe something like --exit-on-fatal-error would better describe the purpose?

[A-Katopodis]

I agree that --exit-on-fatal-error indicates the functionality better. I have updated the RFC to reflect the new name

[nzakas]

Thanks. I think this looks good now.

[nzakas]

It looks like the majority of the feedback has been addressed, so moving to final commenting period.

[nzakas]

@eslint/eslint-tsc It's time to decide on this RFC. Please leave an approval if we can move forward or else leave your feedback.

[btmills]

The rationale for this makes sense after reading through the discussion, and there's enough detail to move forward with an implementation. Thanks @A-Katopodis! LGTM.

[nzakas]

Since we are already past the final commenting period without objections, I’m merging this.

Thanks @A-Katopodis for your hard work on this. We really appreciate it.