Rules redefining: shallow or deep?

[vsemozhetbyt]

What version of ESLint are you using?

2.11.1

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

{
  "extends": [
    "eslint:recommended",
    "airbnb-base"
  ],

  "rules": {
    "eqeqeq": 2,
    "no-console": 0
  }
}

What did you do?...

After reading the documentation about cascading, hierarchy and rules redefining, I wrongly supposed that rules in the user .eslintrc file redefine rules in the extends files as a whole, i.e. if the airbnb-base sets 'no-unused-vars': [2, { 'vars': 'local', 'args': 'after-used' }] and I sets "no-unused-vars": 2 the results will be just "no-unused-vars": 2.

However, the eslint --print-config tests have taught me that I should redefine each option from extends file manually (i.e. the results above will be still "no-unused-vars": [2, { "vars": "local", "args": "after-used" }] if I does not redefine vars and args).

Maybe I've missed something in the docs. If I have not, maybe it is worth more clarirfication.

However, I don't understend how can I cancel array options from extends files rules. So, if airbnb-base sets 'eqeqeq': [2, 'allow-null'], how can I cancel 'allow-null'?

With .eslintrc.json example above and this file:

console.log(undefined == null);

console.log(undefined == 0);

eslint says just:

  3:23  error  Expected '===' and instead saw '=='  eqeqeq

✖ 1 problem (1 error, 0 warnings)

and eslint --print-config inticates that result rule is still

"eqeqeq": [2, "allow-null"]

[alberto]

This is a similar discussion to #6144. Currently, rule configurations are merged, not replaced. I don't think there is a way to unset options from object properties.

@eslint/eslint-team should we change this?

[platinumazure]

I would be 👍 to changing this. Three reasons:

1. It might not be possible/easy to remove (rather than change) a set option on the rule. For example, on lines-around-comment, they might want to remove the blockComment.after option. They can try setting to null but it is up to the rule to handle that correctly.
2. User confusion can arise when trying to override a complicated config with a simple one- instead a merge occurs. This violates principle of least surprise, IMO.
3. If user is taking the time to override a configuration, they probably want it fully spelled out for later maintainability.

[ilyavolodin]

I don't think we can change this. This would affect too many users. Even marking it as breaking might not be enough at this point.

[vsemozhetbyt]

@ilyavolodin Maybe it would be possible to add top parsing option for shallow redefining and turn it off by default to not break the old behavior unexpectedly?

[vsemozhetbyt]

I've tried to detect some rules with array options by this script:

const rules = JSON.parse(require('child_process').execSync(
  `eslint --print-config ${__filename}`, { encoding: 'utf8' }
)).rules;

console.log(
  Object.keys(rules)
    .filter(k => Array.isArray(rules[k]) &&
                 rules[k].some(param => typeof param === 'string')
    )
    .map(k => `${k}: ${JSON.stringify(rules[k])}`)
);

First I've run it with just this .eslintrc.json:

{
  "extends": [
    "eslint:recommended",
    "airbnb-base"
  ]
}

The output:

[ 'no-cond-assign: [2,"always"]',
  'no-extra-parens: [0,"all",{"conditionalAssign":true,"nestedBinaryExpressions":false}]',
  'no-restricted-syntax: [2,"DebuggerStatement","ForInStatement","LabeledStatement","WithStatement"]',
  'array-bracket-spacing: [2,"never"]',
  'arrow-body-style: [2,"as-needed"]',
  'block-spacing: [2,"always"]',
  'brace-style: [2,"1tbs",{"allowSingleLine":true}]',
  'comma-dangle: [2,"always-multiline"]',
  'comma-style: [2,"last"]',
  'computed-property-spacing: [2,"never"]',
  'curly: [2,"multi-line"]',
  'eqeqeq: [2,"allow-null"]',
  'object-curly-spacing: [2,"always"]',
  'object-shorthand: [2,"always"]',
  'one-var: [2,"never"]',
  'one-var-declaration-per-line: [2,"always"]',
  'padded-blocks: [2,"never"]',
  'quote-props: [2,"as-needed",{"keywords":false,"unnecessary":true,"numbers":false}]',
  'quotes: [2,"single","avoid-escape"]',
  'semi: [2,"always"]',
  'space-in-parens: [2,"never"]',
  'spaced-comment: [2,"always",{"exceptions":["-","+"],"markers":["=","!"]}]',
  'wrap-iife: [2,"outside"]',
  'yield-star-spacing: [2,"after"]',
  'import/extensions: [0,"never"]',
  'import/imports-first: [0,"absolute-first"]' ]

@alberto , you could analyze these rules if any of them are candidates for PR like your #6342.

However I've found some strange redefining behavior after I had run the script with my default rules set:

{
  "root": true,

  "extends": [
    "eslint:recommended",
    "airbnb-base"
  ],

  "parserOptions": {
    "ecmaVersion": 7,
    "sourceType": "script"
  },

  "env": {
    "browser": false,
    "es6": true,
    "node": true
  },

  "rules": {
    "arrow-parens": [2, "as-needed"],
    "eqeqeq": 2,
    "key-spacing": [2, { "beforeColon": false, "afterColon": true, "mode": "minimum" }],
    "no-console": 0,
    "no-eq-null": 2,
    "no-extra-parens": [2, "all"],
    "no-multi-spaces": 0,
    "no-unused-vars": [2, { "vars": "all", "args": "all" }],
    "no-use-before-define": [2, { "functions": false }],
    "quotes": [2, "single"],
    "spaced-comment": [2, "always", { "exceptions": ["/", "*"] }],
    "strict": [2, "global"],
    "unicode-bom": [2, "always"]
  }
}

These lines from script output puzzle me:

  'no-extra-parens: [2,"all"]',
  'quotes: [2,"single"]',
  'spaced-comment: [2,"always",{"exceptions":["/","*"]}]',

So:

no-extra-parens: [0, "all", {conditionalAssign: true, nestedBinaryExpressions: false}]
+
no-extra-parens: [2, "all"]
= 
no-extra-parens: [2, "all"]

quotes: [2, "single", "avoid-escape"]'
+
quotes: [2, "single"]
=
quotes: [2, "single"]

spaced-comment: [2, "always", { exceptions: ["-", "+"], markers: ["=", "!"]}]
+
spaced-comment: [2, "always", { exceptions: ["/", "*"] }]
=
spaced-comment: [2, "always", { exceptions: ["/", "*"] }]

It seems that the redefining is half-shallow and half-deep. I don't know if this is worth a new issue post.

[pedrottimark]

• This is another reason to provide a default option for all rules, which (I think) we already intend to do.
• This suggests that there is room for improvement in Configuring ESLint.
• This sounds like another variation on the theme of how to keep up with changes in devDependencies. Similar root problem as Configuration to enable all rules #6240 but a different pain point.

[nzakas]

I don't think it's worth making any code change here. We have millions of installs using the current system and once you understand how it works, and we can't risk breaking all those configs. We can probably better explain how merging works in the docs, though, to have some place to point people to.

[vsemozhetbyt]

@nzakas So is this an intentional merging scheme?

eqeqeq: [2, "allow-null"]
+
eqeqeq: 2
=
eqeqeq: [2, "allow-null"]

while

quotes: [2, "single", "avoid-escape"]'
+
quotes: [2, "single"]
=
quotes: [2, "single"]

[michaelficarra]

So we'll point users to documentation that tells them there's no way to do something that is very reasonable to want to do? 😟 I don't think that makes it much better. We should fix this. It's a broken design.

[nzakas]

@michaelficarra can you be more descriptive? What exactly do you think should change? And what would you suggest to avoid breaking the configurations of existing users?

[platinumazure]

I assume @michaelficarra will mostly agree with this (speak up if I've missed anything).

Definitions (I assume this is unnecessary but just to avoid ambiguity):

• Base configuration: A configuration that is inherited from, either via an "extends" directive in a derived configuration or by virtue of being in an ancestor directory to the derived configuration.
• Derived configuration: A configuration that is inheriting from one or more base configurations, including (though not limited to) a project maintainer's project configuration, which the maintainer is trying to configure to his or her needs.

Proposals:

1. Configuration of a rule in a derived configuration should override (at the rule level) the configuration of the rule in the base configuration.
   • Example One:
     • Base config: "eqeqeq": [2, "allow-null"]
     • Derived config: "eqeqeq": 2
     • Resulting actual config: "eqeqeq": 2
   • Example Two:
     • Base config: "eqeqeq": 2
     • Derived config: "eqeqeq": [2, "allow-null"]
     • Resulting actual config: "eqeqeq": [2, "allow-null"]
   • Example Three:
     • Base config: "no-implicit-coercion": [2, { "boolean": false, "number": true, "string": false }]
     • Derived config: `"no-implicit-coercion": [2, { "boolean": true }]
     • Resulting actual config: "no-implicit-coercion": [2, { "boolean": true }] (and also warns on numbers/strings since they are no longer specified)
   • Obviously a breaking change, but could be argued that it is worthwhile due to being an improved design (though that is subject to interpretation)
2. To avoid a breaking change, we could also add an enum top-level config value: "configMergeStyle": "deepMerge|overwrite", deepMerge being current behavior, overwrite being per my proposal above. That way people can opt in.

@nzakas Hope this helps
@michaelficarra Please add anything I missed

[vsemozhetbyt]

@platinumazure May be this case is also worth mention (current behavior):

quotes: [2, "single", "avoid-escape"]'
+
quotes: [2, "single"]
=
quotes: [2, "single"]

How deepMerge and overwrite apiece should treat avoid-escape (second param) if just single (first param) is defined/overwritten?

[ilyavolodin]

I think I'm with @platinumazure and @michaelficarra it's a broken design in a sense that it doesn't allow you to remove some of the properties that were set in the higher level config. I am, however, of the opinion that we couldn't change the default behavior right now. We can add it as a flag, that would be perfectly fine, but changing default behavior might be a bit too much, unfortunately.

[nzakas]

@platinumazure thanks, that does help. It also solidifies my belief that what we are doing right now makes the most sense.

This merging mechanism was designed for what we thought would be the common use case: changing a rule's severity without changing the options. So what we have right now:

• Base config: "eqeqeq": [2, "allow-null"]
• Derived config: "eqeqeq": 1
• Resulting actual config: "eqeqeq": [1, "allow-null"]

We definitely want people to be able to change severity without needing to re-state every other option that was already defined elsewhere. This is by design.

@vsemozhetbyt this behavior is intentional:

• Base config: "quotes": [2, "single", "avoid-escape"]'
• Derived config: "quotes": [2, "single"]
• Resulting actual config: "quotes": [2, "single"]

When you're changing just the severity, we can safely assume that you want all other options for the rule to remain as they were. When you change something other than the severity, we can't assume that you want every other option as it was previous defined except for the ones you specified. Doing that would mean you'd need to search through all other configs that have altered the options of the rule in some way to be 100% sure of the options you're now applying, and that's pretty confusing.

While I can understand how the current system is a bit confusing when you run into edge cases, I do think it's the probably the best way to handle things given our large user base and the design goals of the system.

[alberto]

@nzakas I don't think any of us thought rules overriding worked that way. I think it makes sense, and just needs to be documented

[nzakas]

@alberto I agree (#6361 (comment))

[vsemozhetbyt]

Well, it is really not so obvious principle, but I could obey the tradition)

By the way, then the team should check all the rules like old eqeqeq that have array options without posibility to override them.

[danny-andrews]

@vsemozhetbyt Agreed. The current behavior is more than just confusing in certain cases, so documentation will not help. It can prevent you from overwriting options inherited from parent configs, as detailed in #6144. This prevented me from extending the airbnb config entirely.

@alberto Thanks for adding the always option for eqeqeq! I will open new issues/PR's for any other un-overridable options I find!

[coderas]

(it's gone a bit quiet...) Is anyone working on documenting this, or introducing a new feature that doesn't break the world ?

[nzakas]

We agreed to update documentation, but that hasn't been done yet. Do you want to submit a PR?

[danny-andrews]

Got a PR up here!

[nzakas]

This was closed by #7499

[GGAlanSmithee]

@vsemozhetbyt thanks for sharing that script, made it really easy to figure out the proper value to override with 👍

[vsemozhetbyt]

@GGAlanSmithee There is another slightly different one. I use it to check the final unfiltered sum of the merged sets:

'use strict';

const rules = JSON.parse(require('child_process').execSync(
  `eslint --print-config ${__filename}`, { encoding: 'utf8' }
)).rules;

console.log(
  Object.keys(rules)
    .map(k => `${k}: ${JSON.stringify(rules[k])}`)
    .sort()
    .join('\n')
);

[CxRes]

Continuing from @vsemozhetbyt's example above, how does one achieve this? Is it even possible?

spaced-comment: [2, "always", { exceptions: ["-", "+"], markers: ["=", "!"]}]
+
spaced-comment: [2, "always", { exceptions: ["/", "*"] }]
=
spaced-comment: [2, "always", { exceptions: ["-", "+", "/", "*"], markers: ["=", "!"] }]

[danny-andrews]

@CxRes No, it's not possible. If you declare options, they are overridden by derived config's. Explanation: 3e6131e

[CxRes]

@danny-andrews Too bad. But thanks for the answer!!!! :)