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

Update: remove invalid defaults from core rules (fixes #11415) #11427

Merged
merged 1 commit into from Feb 25, 2019

Conversation

not-an-aardvark
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix (#11415)

What changes did you make? (Give an overview)

Ajv ignores the default property in JSON schemas when it appears inside oneOf, anyOf, or not. This commit removes ignored default properties and fixes a few bugs in rules that were incorrectly assuming the default properties would get processed.

Is there anything you'd like reviewers to focus on?

Nothing in particular

Ajv ignores the `default` property in JSON schemas when it appears inside `oneOf`, `anyOf`, or `not`. This commit removes ignored `default` properties and fixes a few bugs in rules that were incorrectly assuming the `default` properties would get processed.
@not-an-aardvark not-an-aardvark added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Feb 21, 2019
@aladdin-add aladdin-add self-requested a review February 21, 2019 08:20
@platinumazure
Copy link
Member

Do we plan to report this upstream? Seems ajv could go back and apply defaults once it has identified which schema is being used.

Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

  • should the tag be Fix? since it seems a bugfix.
  • it would not hurt to add a test to avoid regression.

    eslint/tests/lib/linter.js

    Lines 2344 to 2356 in f6ba633

    it("should report no violation", () => {
    const code = [
    "/*eslint-disable no-unused-vars */",
    "var foo; // eslint-disable-line no-unused-vars",
    "var bar;",
    "/* eslint-enable no-unused-vars */" // here
    ].join("\n");
    const config = { rules: { "no-unused-vars": 2 } };
    const messages = linter.verify(code, config, filename);
    assert.strictEqual(messages.length, 0);
    });

@not-an-aardvark
Copy link
Member Author

This is intentional behavior in Ajv (see here).

The commit tag is Update because this fix can cause new errors in rules like max-lines. (Previously, max-lines would never report errors when passed an empty object as an option, because it would interpret the configured maximum as undefined, which would get cast to NaN, so all queries about whether the number of lines exceeded the limit would return false.)

@aladdin-add I'm not sure what kind of test you mean. I did add some tests for rules that had behavior changes -- are there other tests you would recommend?

(I suppose we could possibly enhance RuleTester to look at a rule's schema and check for invalid defaults. Another possibility would be to see if there's a way to make Ajv raise an error for invalid defaults rather than ignoring them.)

@aladdin-add
Copy link
Member

aladdin-add commented Feb 22, 2019

sorry for unclear, I mean adding a test like in the issue:

 it("should report no violation", () => { 

     const code =`
var x = 20;
console.log(x >> 5); // eslint-disable-line no-bitwise`;

     const config = { rules: { "no-bitwise": "error", } }; 
     const messages = linter.verify(code, config, filename); 

     assert.strictEqual(messages.length, 0); 

 }); 

as to validate the schema, I opened a PR a while ago, but it was closed(#10713).

@not-an-aardvark
Copy link
Member Author

Does the test added in tests/lib/rules/line-comment-position.js cover that case? (The issue was that line-comment-position was reporting an error for eslint-disable-line comments, and the test should ensure that it no longer does that.)


Oops, I forgot about #10713. However, it seems like invalid default properties are not actually caught by validateSchema: true, because RuleTester was not reporting any errors for rules with invalid default properties.

I think my current position relating to that PR is:

  • We should do a lot of validation in RuleTester if it helps catch errors.
  • We shouldn't do rule schema validation during regular linting runs if it would impose a significant performance cost.

@aladdin-add
Copy link
Member

aladdin-add commented Feb 22, 2019

ooops, sorry, I had misread the code in the issue. (。・∀・)ノ

@not-an-aardvark not-an-aardvark merged commit de5cbc5 into master Feb 25, 2019
@not-an-aardvark not-an-aardvark deleted the remove-invalid-defaults branch February 25, 2019 07:59
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 25, 2019
@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 Aug 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants