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

Handle rule configuration errors more consistently #9373

Closed
not-an-aardvark opened this issue Sep 30, 2017 · 2 comments · Fixed by Urigo/tortilla#62, mono-js/mono-notifications#5, mono-js/mono-push#5 or terrajs/lib-starter#5 · May be fixed by ali8889/emerald-wallet#4
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features

Comments

@not-an-aardvark
Copy link
Member

I've noticed that our error-handling strategy is very inconsistent across different error scenarios. This inconsistency is a problem because it creates unexpected edge cases in integrations (causing bugs like eslint/archive-website#413).

Problem Current error-handling behavior
Unrecognized rule in config file Linting error
Invalid rule config in config file Fatal error
Invalid rule config in Linter#verify No error
Unrecognized rule in /* eslint */ comment Linting error
Unparseable rule config in /* eslint */ comment Linting error
Parseable, but invalid rule config in /* eslint */ comment Fatal error
Unrecognized rule in /* eslint-disable */ comment No error

I've italicized the cases that I find particularly surprising (namely, that linting can continue if an /* eslint */ comment contains a parsing error, but the process crashes if the rule config is parseable and doesn't match the schema). It seems like an unparseable rule config should always be a more severe issue than an invalid rule config, because all invalid rule configs are parseable.

This is also strange because Linter#verify doesn't verify rule schemas at all, so it ends up that only inline config schemas are verified, not the main config being used for the file.

I think we should make invalid rule configs in /* eslint */ comments into a linting error, rather than a fatal error. That would create a useful invariant where Linter#verify should never throw an error when given a valid config, regardless of the source text being linted.

Separately, I think we might want to reconsider #8325 (validating configs in Linter#verify). There were two objections brought up before: performance and separation of concerns.

  • I don't think performance would be a problem in practice, because we could cache config objects and avoid validating them again if we see a config which is known to be valid. This would prevent CLIEngine from slowing down.
  • Linter already validates rule schemas anyway for inline comments, so I don't think validating the top-level config would introduce any new issues relating to separation of concerns that aren't already present.
@not-an-aardvark not-an-aardvark added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 30, 2017
@mysticatea
Copy link
Member

mysticatea commented Nov 10, 2017

Sounds reasonable to me.

Our online demo does not work silently in the "Parseable, but invalid rule config in /* eslint */ comment" case. Because it's a problem in source code, it should be linting error rather than fatal error.

@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly and removed enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 9, 2017
@mysticatea
Copy link
Member

I re-labeled as a bug because ESLint crashes by the content of source code.

@not-an-aardvark not-an-aardvark self-assigned this Jan 7, 2018
not-an-aardvark added a commit that referenced this issue Jan 7, 2018
This updates `Linter` to treat malformed configuration comments as linting errors rather than fatal issues that crash the process. Previously, this could cause surprising behavior because it was the only known instance where a problem in the source code (given a valid configuration) could cause an error to be thrown.

For example, this also fixes eslint/archive-website#413.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 10, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.