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

[not-an-aardvark]

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

[platinumazure]

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

[aladdin-add]

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]

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]

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]

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]

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