Fix: avoid crashing on malformed configuration comments (fixes #9373) #9819
Conversation
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.
not-an-aardvark
added
accepted
| source: null, | ||
| message: err.message, | ||
| line: comment.loc.start.line, | ||
| column: comment.loc.start.column + 1, |
There was a problem hiding this comment.
I think good if it defines endLine and endColumn with comment.loc.end.
| @@ -112,7 +113,13 @@ function validateRuleOptions(rule, ruleId, options, source) { | |||
| validateRuleSchema(rule, Array.isArray(options) ? options.slice(1) : []); | |||
| } | |||
| } catch (err) { | |||
| throw new Error(`${source}:\n\tConfiguration for rule "${ruleId}" is invalid:\n${err.message}`); | |||
| const enhancedMessage = `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; | |||
There was a problem hiding this comment.
Wondering if this should say "Inline configuration" (or equivalent), in order to make it clear that it's from a configuration comment rather than from a user's configuration files?
There was a problem hiding this comment.
I went with the current behavior on the assumption that it would be clear because the reported problem is at the comment's location.
I'd be fine with changing it if you feel strongly about it, although I wonder if we would want to refactor this function to make the error messages more flexible.
| validator.validateRuleOptions(ruleMapper(name), name, ruleValue, `${filename} line ${comment.loc.start.line}`); | ||
| try { | ||
| validator.validateRuleOptions(ruleMapper(name), name, ruleValue); | ||
| } catch (err) { |
There was a problem hiding this comment.
Should we decorate the thrown error object with some property to indicate it is specifically the validation error we want to report? That way, if we check for that property here, an unexpected exception (from something not directly due to schema validation) would propagate as a crash.
There was a problem hiding this comment.
Seems reasonable, although I think we already use thrown errors to communicate schema issues in other places.
Maybe it would be better to switch to a result type instead, where validator.validateRuleOptions would return something like { success: true } or { success: false, error: err }. Then a thrown error would always indicate that something went wrong.
|
I don't feel strongly enough on any of these to demand a change- just
wanted your thoughts. Consider this approved on my end- we definitely need
to avoid crashing here. Thanks!
…On Jan 7, 2018 2:04 PM, "Teddy Katz" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/linter.js
<#9819 (comment)>:
> @@ -340,7 +340,19 @@ function modifyConfigsFromComments(filename, ast, config, ruleMapper) {
Object.keys(parseResult.config).forEach(name => {
const ruleValue = parseResult.config[name];
- validator.validateRuleOptions(ruleMapper(name), name, ruleValue, `${filename} line ${comment.loc.start.line}`);
+ try {
+ validator.validateRuleOptions(ruleMapper(name), name, ruleValue);
+ } catch (err) {
Seems reasonable, although I think we already use thrown errors to
communicate schema issues in other places.
Maybe it would be better to switch to a result type instead, where
validator.validateRuleOptions would return something like { success: true
} or { success: false, error: err }. Then a thrown error would always
indicate that something went wrong.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9819 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AARWep7OgG7O4np0QytUVTvxg89eC30hks5tISNRgaJpZM4RVjEc>
.
|
|
@not-an-aardvark Instead of calculating line/column/endLine/endColumn manually, can we just pass the comment token to |
|
I can look into doing that. I think it would be nontrivial because a |
Ack, for some reason I thought we were calling context.report, but you're quite right. I don't think we should pull out the report translator here. I'm fine with this going in as is. Maybe we could look into that functionality later... Thanks! |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (#9373)
What changes did you make? (Give an overview)
This updates
Linterto 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.
Is there anything you'd like reviewers to focus on?
Nothing in particular