Change Request: report unnecessary config overrides

[ljharb]

ESLint version

v8.5.0

What problem do you want to solve?

1. I extend a shared config in the root / I set a rule in the root
2. I override a rule from the shared config, or add a rule not present in the shared config / I override a rule set by the root in a nested config or override
3. Later, the shared config changes such that the local rule configuration is a unused, ie, prevents no warnings / Later, I change config in the root such that the nested config or override is unused, ie, prevents no warnings

The problem is that it's really difficult to figure out when the third step applies.

What do you think is the correct solution?

Like the reportUnusedDisableDirectives config option, I would like a config option that reports unnecessary configuration - in other words, any configuration that exists that matches the existing inferred configuration.

Participation

• I am willing to submit a pull request for this change.

Additional comments

I'd need many pointers to submit a PR for this, but I'm happy to help where I can.

(filed based on #15466 (comment))

[nzakas]

Interesting idea. I’m thinking through how we could do this in the new config system and I think it could be pretty easy to detect (Famous last words.) but I’m not sure what kind of behavior would make sense. Also, would this apply to rule configuration directive inside a source file?

What is the behavior you’d expect if a dupe was found? What would happen if one shared config you extend has the same rule options as another shared config you extend?

Note this is different than reporting disable directives because those are in the source file and so they can look like any other linting error.

[ljharb]

I’d expect anything that wasn’t the first definition for a rule, that matched a previously established one, to be warned on - inline or not.

i would not expect it to warn on anything from a shared config itself - only on things within the current repo. In other words, if after extending two shared config, i then defined something unnecessary, I’d expect my definition to be reported.

[nzakas]

Gotcha. And how would you expect it to be reported in the output?

[ljharb]

The same way that unused disable directives are - but i suppose the interesting question is “under what filename”? It seems like it’d show up under the file it was in, whether it was inline or a config file.

[nzakas]

Yes, that’s the problem, there’s not always going to be a filename to show (if you’re only using shared configs), and if there is a file, we don’t have any location information for where in the file the rule config is being set.

So, I don’t think we can treat this like a linting error.

[ljharb]

There would always be a filename tho - it wouldn’t report the shared config, only in-project config files

[nzakas]

Ah sorry, I missed that earlier. In the new config system, it’s not easy to tell the difference between shared configs and local configs, as it’s just an array of objects. We can detect dupes, but I don’t think we can get exactly what you want. The only real option is to just throw an error for any dupes (not treated like a linting error).

[ljharb]

That seems like a flaw in the new config system that still has time to be corrected?

[nzakas]

It's not a flaw, it's an intentional design decision. We no longer want ESLint to have to load modules. Instead, we want you to use import, require, etc. to pull in what you want and place it into the config array. That's how we will escape the shared config loading hell we can get stuck in right now.

I think the best course forward right now is just to wait until we have the new config system ready for testing and see what would make sense.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[ljharb]

The unfortunate aspect of that design decision is that it will no longer be possible to have a static configuration.

[JoshuaKGoldberg]

Oh hey, I just found this issue while doing research for #18230. It looks to me that #18230 is a duplicate of this one. Is there any functional difference between these two issues? And/or, is there still any reason for one to be marked as Blocked?

[nzakas]

@JoshuaKGoldberg #18230 is specifically about inline comments. This one is about what's happening in the config file. So if one object in the array has the same rule configuration as another object in the same array.

And yes, this isn't blocked anymore, good call out.

[JoshuaKGoldberg]

Ah, thanks. In that case I'm in favor of this issue. Config file overrides is a big source of unnecessary cruft. This is especially present in configs and plugins that provide presets.

Similar to the OP's user story, what I see often is:

1. Devs on an older codebase try to revamp their ESLint config
2. They add a preset config before their existing config (in eslintrc land, as an extends)
3. They keep the existing contents of their config file, not realizing much of it may be redundant

Having warnings for unnecessary config overrides would also be useful when updating to new major versions of ESLint or plugins. Right now, nothing tells people when a config override that used to do something is now unnecessary.

Question: would we want an RFC for this, similar to #18230 -> eslint/rfcs#121? And if so, should they be the same RFC to discuss the two likely-to-be-similar options together?

[nzakas]

Yes, we'd want an RFC for this. Because both proposals essentially deal with comparing two configs to find duplication, then I'd agree that they can be combined into a single RFC.

[nzakas]

Another thought: It's worth considering if this use case is better served by the config inspector rather than another CLI option.

[JoshuaKGoldberg]

Following up from eslint/rfcs#121: that RFC was accepted with the explicit note:

eslint/eslint#15476 Change Request: report unnecessary config overrides previously suggested also checking config files (eslint.config.*).
Doing so could be beneficial to flag values that become unnecessary in config files over time.
However, because flat config purely involves spreading objects, there's no way to know what objects originate from shared configs or shared packages.

Should this issue be closed out as wontfix?

[nzakas]

I still think this is something that could be included in the config inspector. I'm not sure we need to locate where each config came from (especially if they have name properties), but I think a view where we can see two or more configs contain the same rule configuration would be beneficial.

@antfu what do you think?

[antfu]

We kinda have that feature already - what addition would we need?

[nzakas]

Ooh nice! @ljharb do you think that "Overloaded" filter in the config inspector does what you're looking for?

[ljharb]

That seems like it lists all overloaded rules - i want to list only unnecessary overloads, ie, overloads that when removed, don’t change the resulting config.

I haven’t used the config inspector before tho, so maybe it’d work for me to be fair - but my goal is to put something in CI, not just expose it to humans, and I’m not sure the config inspector can do that?

[nzakas]

my goal is to put something in CI, not just expose it to humans, and I’m not sure the config inspector can do that?

Ah yeah, then that won't work.

However, because of flat config, this is something you can probably implement fairly easily yourself. Since the config file just exports an array of objects, you could just write a short script that loops through and checks for duplicates.

[ljharb]

Hmm, I suppose that's true - the OP was asking for eslintrc, which I still use. In flat config, I imagine the recursively nested config potential would make it hard to get right (even though I'm sure it'd be the same code needed in core).

[nzakas]

Yeah I'm sorry, we're not doing any further work on eslintrc.

For flat config, there is no nesting of configurations (hence the nickname, "flat").

[ljharb]

oh hm, good point :-) the config for each rule could be deeply nested, but that shouldn't be too tricky to compare.

since i need it for eslintrc too, seems like i'll have to write an external tool anyways.