fix: deep merge behavior in flat 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 #17820

Tell us about your environment (npx eslint --env-info):

Node version: v21.5.0
npm version: v10.2.4
Local ESLint version: v8.56.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 23.1.0

What parser are you using (place an "X" next to just one item)?

[x] Default (Espree)
[ ] @typescript-eslint/parser
[ ] @babel/eslint-parser
[ ] vue-eslint-parser
[ ] @angular-eslint/template-parser
[ ] Other

Please show your full configuration:

No config file.

What did you do? Please include the actual source code causing the issue.

I compared the merge behavior in flat config against eslintrc in different cases.

// test1.js
const { default: { FlatESLint } } = await import("eslint/use-at-your-own-risk");

const eslint = new FlatESLint({
    overrideConfigFile: true,
    baseConfig: [
        {
            languageOptions: {
                parserOptions: {
                    foo: {}
                }
            },
        },
        {
            languageOptions: {
                parserOptions: {
                    // null overwrites other values in eslintrc.
                    foo: null
                }
            },
        },
    ]
});
const { languageOptions: { parserOptions } } = await eslint.calculateConfigForFile("file.js");
console.log(parserOptions);

// test2.js
const { default: { FlatESLint } } = await import("eslint/use-at-your-own-risk");

const eslint = new FlatESLint({
    overrideConfigFile: true,
    baseConfig: [
        {
            languageOptions: {
                parserOptions: {
                    prop1: {},
                    prop2: {},
                    prop3: { 1: "one" },
                    prop4: ["a", "b", "c"],
                    prop5: { ["__proto__"]: 1 }
                }
            },
        },
        {
            languageOptions: {
                parserOptions: {
                    // In eslintrc, primitives overwrite other values.
                    prop1: 123,

                    // In eslintrc, strings are not spread.
                    prop2: "foo",

                    // In eslintrc, it is possible to merge an object or an array with another array.
                    // Properties in the second array override properties in the first value.
                    prop3: ["zero"],
                    prop4: ["d", "e"],

                    // An own property "__proto__" is not merged in eslintrc.
                    prop5: { ["__proto__"]: 2 }
                }
            },
        },
    ]
});
const { languageOptions: { parserOptions } } = await eslint.calculateConfigForFile("file.js");
console.log(parserOptions);

What did you expect to happen?

The merge behavior in flat config should be the same as in eslintrc.

❯ node test1.js
{ foo: null }

❯ node test2.js
{
  prop1: 123,
  prop2: 'foo',
  prop3: [ 'zero', 'one' ],
  prop4: [ 'd', 'e', 'c' ],
  prop5: {}
}

What actually happened? Please include the actual, raw output from ESLint.

There are some differences in the merge behavior, all addressed in this PR.

❯ node test1.js
Error: Key "languageOptions": Key "parserOptions": Cannot convert undefined or null to object

❯ node test2.js
{
  prop1: {},
  prop2: { '0': 'f', '1': 'o', '2': 'o' },
  prop3: [ 'zero' ],
  prop4: [ 'd', 'e' ],
  prop5: { ['__proto__']: 2 }
}

Repro: https://stackblitz.com/edit/stackblitz-starters-7ta5nx

What changes did you make? (Give an overview)

Updated the deepMerge algorithm in lib/config/flat-config-schema.js to make it more similar to eslintrc's mergeWithoutOverwrite. I think this was requested in #17820 (comment).

The new changes affect the way properties are merged: we could keep all of them or revert some to the current behavior.

• Primitive property values in the second object will always overwrite properties of the first object. Currently, if the first property value is an object and the second one is a primitive, the algorithm will try to merge the values, producing a new object that is mostly, but not always, a clone of the first one.
• null property values in the second object will no longer result in a runtime error but instead behave like other primitives and overwrite properties of the first object. This is the original bug reported in Bug: (flat config) ESLint crashes when merging a null value in a config #17820.
• String property values in the second object will behave like other primitives and no longer spread over their characters.
• When merging two objects with an own __proto__ property, the __proto__ property will no longer be merged. In the current merge implementation, the result will contain an own __proto__ property if the first object does.
• Arrays can merge with arrays and other objects like in eslintrc. The resulting value will be an array or a plain object, depending on the type of the second value. Currently, merging only occurs when the second value is not an array (although the first value may be any object). I'm not 100% sure of this change because of this comment. There is agreement that in the new config system arrays should not merge with other arrays or objects like in eslintrc.

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

All of the above points.

As a hint, you may find it helpful to play with the repro when not sure about how things work in the current implementation or in eslintrc.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	fac37ba
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/658e9c511a6d83000820bae4

[fasttime]

I believe that these cases are not actually supported in eslintrc or in flat config, because arrays are never merged directly. So I've replaced them with similar unit tests below where arrays are wrapped in a property.

[nzakas]

Thanks for doing this! I think that we should fix all of the eslintrc incompatibilities in this PR. It was definitely my intent to mirror eslintrc merging behavior. 👍

[nzakas]

If we don't want the __proto__, then I'd suggest using Object.create(null) to create result.

[fasttime]

I think the intent here was just to exclude __proto__ when it's an own property, because the inherited property Object.prototype.__proto__ should not be masked or accidentally overwritten (delete only removes own properties). By contrast, Object.create(null) creates an object with null prototype, that lacks all methods of Object.prototype but could still have an own property called __proto__, so it's not really the same thing.

[fasttime]

Alternatively, we could treat __proto__ as a regular property with the usual merge behavior as other properties. We'd just need to take care to use Object.defineProperty rather than a regular assignment (=) to set __proto__ in order to not mess up with the prototype (here and here).

[nzakas]

Ah gotcha, so this line is deleting an own property that might have been added through the spread operations. That makes sense to me. 👍

[mdjermanovic]

This is indeed eslintrc's behavior, but I'm not sure if it was intentional or maybe an unintentional effect of an intent to just deep-clone arrays. When I update mergeWithoutOverwrite so that arrays overwrite arrays instead of merging, no tests fail either in the eslintrc project or the eslint project.

function mergeWithoutOverwrite(target, source) {
    if (!isNonNullObject(source)) {
        return;
    }

    for (const key of Object.keys(source)) {
        if (key === "__proto__") {
            continue;
        }

-       if (isNonNullObject(target[key])) {
+       if (isNonNullObject(target[key]) && !Array.isArray(target[key])) {
            mergeWithoutOverwrite(target[key], source[key]);
        } else if (target[key] === void 0) {
-           if (isNonNullObject(source[key])) {
-               target[key] = Array.isArray(source[key]) ? [] : {};
+           if (isNonNullObject(source[key]) && !Array.isArray(source[key])) {
+               target[key] = {};
                mergeWithoutOverwrite(target[key], source[key]);
            } else if (source[key] !== void 0) {
                target[key] = source[key];
            }
        }
    }
}

I'd personally rather expect that a generic deep-merging just overwrites arrays, so that this example results in { someProperty: ["qux", void 0, 42] }. Merging item-by-item makes sense only when it is known that the array is meant to be used in a way that each position in the array has a specific meaning (i.e., like a tuple).

Now when searching through issues and PRs, I found that we mentioned this in #14321 (comment).

[fasttime]

Ah thanks, now that comment in code makes sense. So, should the same logic also apply when only one of the values is an array, i.e. should an array overwrite a plain object and vice-versa?

deepMerge({ someProperty: ["foo"] }, { someProperty: { bar: "baz" } });
// expected { someProperty: { bar: "baz" } }
// not      { someProperty: { 0: "foo", bar: "baz" } }

deepMerge({ someProperty: { 1: "foo" }, { someProperty: ["bar"] } });
// expected { someProperty: ["bar"] } }
// not      { someProperty: ["bar", "foo"] }

[mdjermanovic]

I think yes. I'd say the new config system is a good opportunity to change the confusing and possibly unintended specifics of eslintrc's deep merging, but would like to hear @nzakas's opinion too.

Merging array properties into objects and vice versa almost never makes sense.

Merging arrays into arrays might make sense in some specific cases, but I believe it's generally expected that arrays overwrite arrays.

So the behavior would be pretty simple to explain. Merging first with second results in:

• Deeply merged object if both first and second are plain objects (neither is an array).
• first if second is undefined.
• second in all other cases.

[nzakas]

I think those changes make sense. Let's do it. 👍

[fasttime]

Thanks! This should be working as suggested after 5d435f6.

[fasttime]

I've just noticed another edge case: when a property is undefined in both first and second object - but explicitly set in one or both - eslintrc does not set it at all in the result, while deepMerge would currently set it with value undefined.

deepMerge({ someProperty: void 0 }, { someProperty: void 0 });
// current result:           { someProperty: undefined }
// merge result in eslintrc: {}

Any advice/preference on how to handle this?

[mdjermanovic]

Looks like this can never happen, because second is always a non-array object?

[fasttime]

Thanks! Removed in 6e2da74.

[mdjermanovic]

I've just noticed another edge case: when a property is undefined in both first and second object - but explicitly set in one or both - eslintrc does not set it at all in the result, while deepMerge would currently set it with value undefined.

deepMerge({ someProperty: void 0 }, { someProperty: void 0 });
// current result:           { someProperty: undefined }
// merge result in eslintrc: {}

Any advice/preference on how to handle this?

I think it's fine to leave the property with undefined value in this case.

[fasttime]

Added a test to check that undefined properties are actually set (b6732ba). Also fixed an edge case to make sure that only own enumerable properties are set or merged (fac37ba). This matches the behavior in eslintrc.

[mdjermanovic]

I've updated the title since we're not fully replicating eslintrc behavior.

[mdjermanovic]

I'll close-reopen just to double check CI, since we had a few changes on main.

[mdjermanovic]

LGTM, thanks! Leaving open for @nzakas to verify.

One thing that is different with this implementation, compared to eslintrc and the previous implementation, is that they were deep-cloning all objects and arrays, while this keeps the same instances except for objects that are getting merged with other objects in the process. However, I don't think this is important, as we're not modifying any objects or arrays that were passed in.

[nzakas]

LGTM. Thanks for all your work on this. 🙏