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
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.
source: null, | ||
message: err.message, | ||
line: comment.loc.start.line, | ||
column: comment.loc.start.column + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think good if it defines endLine
and endColumn
with comment.loc.end
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me, but I wanted to leave some thoughts inline and see what you think.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
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
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.
Is there anything you'd like reviewers to focus on?
Nothing in particular