Fix: clone config before validating (fixes #12592)

[anikethsaha]

Prerequisites checklist

• I have read the contributing guidelines.
• The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

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

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Cloning the rule config and other configs before validating them as they get mutated with default values when used useDefault : true in agv.
I couldn't find any other entry point for this issue other than this, do let me know if any!

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

fixes #12592

[kaicataldo]

I'm not actually finding where the validate function is used, and I'm wondering if this is dead code. Please let me know if I'm missing something.

Additionally, we're using ajv's useDefaults option (see here), and I think cloning it in the validation function means that these default values won't be used by the rules. Can we clone the config at a higher level and then pass that to the validator function?

We'll also want to include some tests for this, though I don't think this PR should require any rule test changes. I think we'll want to test that a config object doesn't get mutated after a run of the linter.

[kaicataldo]

Thanks for working on this!

[anikethsaha]

Can we clone the config at a higher level and then pass that to the validator function?

By higher level, you meant where we are reading/getting the config? then I think it needs to pass two configs to the validator one cloned and one original. cause only the cloned one will be validated. the original one will be passed to the rules

[kaicataldo]

Is there a reason we can't clone the config as soon as we have the full resolved config and use that for validation and then pass that to the rules?

[anikethsaha]

i will try that 👍

[anikethsaha]

@kaicataldo I updated the cloning to a higher level.
I think I might have missed some other entry points for configs.

Also, do you think it would be helpful to add a clone here instead

eslint/lib/cli-engine/config-array-factory.js

Lines 803 to 809 in 76324ac

	if (configData) {
	return this._normalizeConfigData(configData, {
	...ctx,
	filePath: plugin.filePath,
	name: `${ctx.name} » plugin:${plugin.id}/${configName}`
	});
	}

Like this

if (configData) {
  const clonedConfigData = lodash.cloneDeep(configData);
  return this._normalizeConfigData(
    clonedConfigData,
    plugin.filePath,
    `${importerName} » plugin:${plugin.id}/${configName}`
  );
}

[aladdin-add]

It had been cloned when ajv's useDefaults was introduced( see here
), but was removed somehow.

• I agreed it better to be in a higher level: reading config -> cloning config -> validating config.
• a test case is required!

thanks for working on this!

[anikethsaha]

@aladdin-add can you check if the current one is fine ?

let me know if this has to be changed. after that, I will add some tests

[aladdin-add]

I'd suggest it in something like linter.js, than in getRuleOptions -- it could be cloned multi-times.

[anikethsaha]

I'd suggest it in something like linter.js, than in getRuleOptions

I think the current one in this PR is something like this.

earlier I planned to have it clone and validate in config-validator.js alone. In this way, it will clone every config irrespective of where they are coming from.

But I think the current one is the entry point for the config (not for the directives,)

[anikethsaha]

@kaicataldo can you please take a look if this is the fine place to clone?

[nzakas]

@kaicataldo can you take a look at this again?

[kaicataldo]

Apologies for the delay! I've been sick the last few weeks and am just getting caught up.

[anikethsaha]

@nzakas feel free to finish this or I can try to finish this as well if the final direction has been set whether to use JSON.parse(JSON.stringify(rules)) or not.

Either way works for me.

👍

[nzakas]

The direction has been set. If you're able to update this, that would be great as I don't have a lot of time. But if you can't finish it, then I can take it over. It just might take some time for me to finish.

[anikethsaha]

The direction has been set.

@nzakas So are we moving forward with JSON.parse(JSON.stringify(rules)) instead of the current clone logic ?

If you're able to update this, that would be great as I don't have a lot of time. But if you can't finish it, then I can take it over. It just might take some time for me to finish.

ok cool, I will try to finish this weekend then.

[nzakas]

Correct, and thanks 👍

[anikethsaha]

Done 👍

[mysticatea]

The direction LGTM, thank you!
I have some small suggestions.

[mysticatea]

LGTM. Thank you for your contribution!

[nzakas]

Thanks so much for sticking with this! We really appreciate it.

Changes have been addressed - Kai on vacation

[anikethsaha]

@nzakas @mysticatea I think we need to revert this ? 7fb45cf#r40036492

Cause I guess users are using non serialized data for the config

[ljharb]

unfortunately i never saw this PR; JSON cloning should never be used, especially since eslint config values can be regexes, Infinity, NaN, and other things that don't survive JSON serialization.

[anikethsaha]

@ljharb We did try to avoid json cloning using this. But we tried using lodash cloneDeepWith with customizer but that had different issues like circular ref issue,

[JounQin]

This is a BREAKING CHANGE for a lot of plugins which support options via

regexes, Infinity, NaN, and other things that don't survive JSON serialization

For example: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/filename-case.md#ignore