fix: allow circular references in config

[fasttime]

Prerequisites checklist

• I have read the contributing guidelines.

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 autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #17665

What changes did you make? (Give an overview)

This PR fixes an issue that would cause a stack overflow when configuration settings or parserOptions contain a circular reference.

The fix uses a two-dimensional map in the deepMerge function to keep track of the results previously created when merging pairs of arguments. If the same pair of arguments is encountered later on while merging another property, the previous result will be returned immediately. This avoids infinite recursion.

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

I have questions about two important implementation details. Would like reviewers to confirm if my approach is okay or how to change it.

Question 1

What is the expected result when merging an object with a primitive or a function? E.g.

// current result is { a: { 0: "b", 1: "a", 2: "z", foo: "bar" } }
// new result is { a: "baz" }
merge(
    { a: { foo: "bar" } },
    { a: "baz" }
);

// currently throws TypeError
// new result is { a: null }
merge(
    { a: { foo: "bar" } },
    { a: null }
);

// current result is { a: { foo: "bar" } }
// new result is { a: () => { } }
merge(
    { a: { foo: "bar" } },
    { a: () => { } }
);

To me, none of the above examples seems to behave as intended in the current implementation. In my unit tests, I'm assuming that when two values cannot be merged because they are not both objects, the expectation is to just pick the second value if it's not undefined. That's how Lodash's merge works, for example, except that it also merges arrays while we don't.

I'm leaving this as a question for reviewers if we want to keep this behavior like that, revert it to how it was before, or change it in another way.

Question 2

There is still some potential to produce a stack overflow even without a circular reference, for example if a property evaluates to a different object on every access. Here's an example:

// eslint.config.js

function createObject(parent, generation) {
    return {
        parent,
        generation,
        get child() {
            return createObject(this, this.generation + 1);
        }
    };
}

export default [{
    settings: {
        sharedData: createObject(null, 0)
    }
}];

I think this is a constructed case we don't need to care about, but if we decide to do so and handle this situation in a way or another, then we should also have a look at normalizeRuleOptions:

eslint/lib/config/flat-config-schema.js

Line 132 in 5de9637

return structuredClone(finalOptions);

The structuredClone algorithm we use there will similarly fail with an error when non-circular properties exceed the depth that fits inside the stack - but again, I don't expect this situation to happen for a reasonable config.

[eslint-github-bot]

Hi @fasttime!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

• The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
• There should be a space following the initial tag and colon, for example 'feat: Message'.

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

[netlify]

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	b20538c
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/6561fa6afd066e00099573e7
😎 Deploy Preview	https://deploy-preview-17752--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[snitin315]

What is the expected result when merging an object with a primitive or a function? E.g.

I expect the latest value to be prioritized and overridden correctly when merging, so the new behavior looks fine. If the new value isn't in the expected format the program should automatically throw errors.

There is still some potential to produce a stack overflow even without a circular reference, for example, if a property evaluates to a different object on every access.

I believe we don't need to solve this edge case at the moment, we can look at it later if there is such a requirement from any user.

[nzakas]

Question 1: I'm leaving this as a question for reviewers if we want to keep this behavior like that, revert it to how it was before, or change it in another way.

I think we should revert it to the current ESLint behavior. Merging is complicated enough, and I don't want to risk introducing breaking changes.

Question 2: There is still some potential to produce a stack overflow even without a circular reference, for example if a property evaluates to a different object on every access.

I agree with @snitin315, I don't think we need to worry about this edge case.

[fasttime]

Question 1: I'm leaving this as a question for reviewers if we want to keep this behavior like that, revert it to how it was before, or change it in another way.

I think we should revert it to the current ESLint behavior. Merging is complicated enough, and I don't want to risk introducing breaking changes.

Thanks! I've reverted to the current behavior. Note that some unit tests will now assert seemingly unwanted results, but if you prefer to avoid unintentional breaking changes, I think we should keep those tests as well.

[nzakas]

Thanks, just a couple of comments about the behavior.

(Please feel free to let me know if we are already doing what I'm asking about and I just missed it.)

[nzakas]

Why is string a special case? It seems odd that the behavior is different between numbers and strings.

[fasttime]

This is the current behavior. It's because of the destructuring here:

eslint/lib/config/flat-config-schema.js

Lines 86 to 89 in 5de9637

	const result = {
	...first,
	...second
	};

A primitive like "qux" is converted into an object during destructuring i.e. Object("qux"), which is similar to { "q": 0, "u": 1, "x": 2 }.

A number like 42 will be also converted into an object when destructured, but since Object(42) has no enumerable properties, the effect won't be visible, and it will behave like an empty object.

[nzakas]

Gotcha. Thanks for adding tests to make this clear. 👍

[nzakas]

Why is this an error?

[fasttime]

Again this is the current behavior, but this is happening because of the call to Object.keys(second) on this line:

eslint/lib/config/flat-config-schema.js

Line 91 in 5de9637

for (const key of Object.keys(second)) {

The problem is that Object.keys throws an error if called with an argument of null or undefined. Here, the argument (second) cannot be undefined, because it is assigned a default value in the declaration, but it can be null.

If we only want to fix this test case, and the previous case where a string is being destructured into an object, then we could make sure that second is an object before calling deepMerge (and if it is a primitive, replace it with an empty object):

-        const secondValue = second[key];
+        let secondValue = second[key];
 
         if (isNonNullObject(firstValue)) {
+            if (!isNonNullObject(secondValue)) {
+                secondValue = EMPTY_OBJECT;
+            }

This would avoid the TypeError when the second value is null, and treat "qux" as an empty object, rather than using its characters as properties, and it would keep the current behavior unchanged in all the other test cases.

[bradzacher]

As per #17820 - currently the ecosystem relies on the override behaviour - at least for null.

So crashing out here will impact migration efforts and will mean that we have to redesign parser options to account for this change.

---

For reference - typescript eslint handles parserOptions.project = null to mean "not set" (I.e. Type aware linting is turned off).

The current behaviour is that a user can turn on type-aware linting in their base config (eg parserOptions.project = ["some TS project path"]) then pick specific parts of their codebase for which to disable type-aware linting using eslintrc overrides and setting parserOptions.project = null.

For eslintrc merging - the null clobbers the value - enabling this behaviour.

There's no other way to clobber or erase the value in an overrides because an empty array just merges in and is thus a no-op.

As I said - removing this behaviour would set the ecosystem back unless an alternative is provided. Being able to "clear" a property in an override is quite a useful feature for complex configs.

[nzakas]

@fasttime ah okay, thanks for explaining.

@bradzacher for the purposes of this PR, we don't want to change the default behavior. We'll discuss on your issue.

[nzakas]

LGTM. Would like @mdjermanovic to take a look before merging.

[github-actions]

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

[mdjermanovic]

when two values cannot be merged because they are not both objects, the expectation is to just pick the second value if it's not undefined.

I also think it should work this way. We can fix this in a PR for #17820.

[mdjermanovic]

LGTM, thanks!