Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow circular references in config #17752

Merged
merged 2 commits into from Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 27 additions & 8 deletions lib/config/flat-config-schema.js
Expand Up @@ -61,13 +61,17 @@ function isUndefined(value) {
return typeof value === "undefined";
}

// A unique empty object to be used internally as a mapping key in `deepMerge`.
const EMPTY_OBJECT = {};

/**
* Deeply merges two objects.
* @param {Object} first The base object.
* @param {Object} second The overrides object.
* @param {any} second The overrides value.
* @param {Map<string, Map<string, Object>>} [mergeMap] Maps the combination of first and second arguments to a merged result.
* @returns {Object} An object with properties from both first and second.
*/
function deepMerge(first = {}, second = {}) {
function deepMerge(first, second = {}, mergeMap = new Map()) {

/*
* If the second value is an array, just return it. We don't merge
Expand All @@ -77,8 +81,23 @@ function deepMerge(first = {}, second = {}) {
return second;
}

let secondMergeMap = mergeMap.get(first);

if (secondMergeMap) {
const result = secondMergeMap.get(second);

if (result) {

// If this combination of first and second arguments has been already visited, return the previously created result.
return result;
}
} else {
secondMergeMap = new Map();
mergeMap.set(first, secondMergeMap);
}

/*
* First create a result object where properties from the second object
* First create a result object where properties from the second value
* overwrite properties from the first. This sets up a baseline to use
* later rather than needing to inspect and change every property
* individually.
Expand All @@ -88,6 +107,9 @@ function deepMerge(first = {}, second = {}) {
...second
};

// Store the pending result for this combination of first and second arguments.
secondMergeMap.set(second, result);

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

// avoid hairy edge case
Expand All @@ -99,13 +121,10 @@ function deepMerge(first = {}, second = {}) {
const secondValue = second[key];

if (isNonNullObject(firstValue)) {
result[key] = deepMerge(firstValue, secondValue);
result[key] = deepMerge(firstValue, secondValue, mergeMap);
} else if (isUndefined(firstValue)) {
if (isNonNullObject(secondValue)) {
result[key] = deepMerge(
Array.isArray(secondValue) ? [] : {},
secondValue
);
result[key] = deepMerge(EMPTY_OBJECT, secondValue, mergeMap);
} else if (!isUndefined(secondValue)) {
result[key] = secondValue;
}
Expand Down
228 changes: 228 additions & 0 deletions tests/lib/config/flat-config-schema.js
@@ -0,0 +1,228 @@
/**
* @fileoverview Tests for flatConfigSchema
* @author Francesco Trotta
*/

"use strict";

const { flatConfigSchema } = require("../../../lib/config/flat-config-schema");
const { assert } = require("chai");

describe("merge", () => {

const { merge } = flatConfigSchema.settings;

it("merges two objects", () => {
const first = { foo: 42 };
const second = { bar: "baz" };
const result = merge(first, second);

assert.deepStrictEqual(result, { ...first, ...second });
});

it("overrides an object with an array", () => {
const first = { foo: 42 };
const second = ["bar", "baz"];
const result = merge(first, second);

assert.strictEqual(result, second);
});

it("merges an array with an object", () => {
const first = ["foo", "bar"];
const second = { baz: 42 };
const result = merge(first, second);

assert.deepStrictEqual(result, { 0: "foo", 1: "bar", baz: 42 });
});

it("overrides an array with another array", () => {
const first = ["foo", "bar"];
const second = ["baz", "qux"];
const result = merge(first, second);

assert.strictEqual(result, second);
});

it("returns an emtpy object if both values are undefined", () => {
const result = merge(void 0, void 0);

assert.deepStrictEqual(result, {});
});

it("returns an object equal to the first one if the second one is undefined", () => {
const first = { foo: 42, bar: "baz" };
const result = merge(first, void 0);

assert.deepStrictEqual(result, first);
assert.notStrictEqual(result, first);
});

it("returns an object equal to the second one if the first one is undefined", () => {
const second = { foo: 42, bar: "baz" };
const result = merge(void 0, second);

assert.deepStrictEqual(result, second);
assert.notStrictEqual(result, second);
});

it("merges two objects in a property", () => {
const first = { foo: { bar: "baz" } };
const second = { foo: { qux: 42 } };
const result = merge(first, second);

assert.deepStrictEqual(result, { foo: { bar: "baz", qux: 42 } });
});

it("does not override a value in a property with undefined", () => {
const first = { foo: { bar: "baz" } };
const second = { foo: void 0 };
const result = merge(first, second);

assert.deepStrictEqual(result, first);
assert.notStrictEqual(result, first);
});

it("does not change the prototype of a merged object", () => {
const first = { foo: 42 };
const second = { bar: "baz", ["__proto__"]: { qux: true } };
const result = merge(first, second);

assert.strictEqual(Object.getPrototypeOf(result), Object.prototype);
});

it("does not merge the '__proto__' property", () => {
const first = { ["__proto__"]: { foo: 42 } };
const second = { ["__proto__"]: { bar: "baz" } };
const result = merge(first, second);

assert.deepStrictEqual(result, second);
assert.notStrictEqual(result, second);
});

it("throws an error if a value in a property is overriden with null", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor

@bradzacher bradzacher Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

const first = { foo: { bar: "baz" } };
const second = { foo: null };

assert.throws(() => merge(first, second), TypeError);
});

it("does not override a value in a property with a primitive", () => {
const first = { foo: { bar: "baz" } };
const second = { foo: 42 };
const result = merge(first, second);

assert.deepStrictEqual(result, first);
assert.notStrictEqual(result, first);
});

it("merges an object in a property with a string", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const first = { foo: { bar: "baz" } };
const second = { foo: "qux" };
const result = merge(first, second);

assert.deepStrictEqual(result, { foo: { 0: "q", 1: "u", 2: "x", bar: "baz" } });
});

it("merges objects with self-references", () => {
const first = { foo: 42 };

first.first = first;
const second = { bar: "baz" };

second.second = second;
const result = merge(first, second);

assert.strictEqual(result.first, first);
assert.deepStrictEqual(result.second, second);

const expected = { foo: 42, bar: "baz" };

expected.first = first;
expected.second = second;
assert.deepStrictEqual(result, expected);
});

it("merges objects with overlapping self-references", () => {
const first = { foo: 42 };

first.reference = first;
const second = { bar: "baz" };

second.reference = second;

const result = merge(first, second);

assert.strictEqual(result.reference, result);

const expected = { foo: 42, bar: "baz" };

expected.reference = expected;
assert.deepStrictEqual(result, expected);
});

it("merges objects with cross-references", () => {
const first = { foo: 42 };
const second = { bar: "baz" };

first.second = second;
second.first = first;

const result = merge(first, second);

assert.deepStrictEqual(result.first, first);
assert.strictEqual(result.second, second);

const expected = { foo: 42, bar: "baz" };

expected.first = first;
expected.second = second;
assert.deepStrictEqual(result, expected);
});

it("merges objects with overlapping cross-references", () => {
const first = { foo: 42 };
const second = { bar: "baz" };

first.reference = second;
second.reference = first;

const result = merge(first, second);

assert.strictEqual(result, result.reference.reference);

const expected = { foo: 42, bar: "baz", reference: { foo: 42, bar: "baz" } };

expected.reference.reference = expected;
assert.deepStrictEqual(result, expected);
});

it("produces the same results for the same combinations of property values", () => {
const firstCommon = { foo: 42 };
const secondCommon = { bar: "baz" };
const first = {
a: firstCommon,
b: firstCommon,
c: { foo: "different" },
d: firstCommon
};
const second = {
a: secondCommon,
b: { bar: "something else" },
c: secondCommon,
d: secondCommon
};
const result = merge(first, second);

assert.deepStrictEqual(result.a, result.d);

const expected = {
a: { foo: 42, bar: "baz" },
b: { foo: 42, bar: "something else" },
c: { foo: "different", bar: "baz" },
d: { foo: 42, bar: "baz" }
};

assert.deepStrictEqual(result, expected);
});
});
66 changes: 66 additions & 0 deletions tests/lib/eslint/flat-eslint.js
Expand Up @@ -6031,6 +6031,72 @@ describe("FlatESLint", () => {
});
}

describe("config with circular references", () => {
it("in 'settings'", async () => {
let resolvedSettings = null;

const circular = {};

circular.self = circular;

const eslint = new FlatESLint({
overrideConfigFile: true,
baseConfig: {
settings: {
sharedData: circular
},
rules: {
"test-plugin/test-rule": 1
}
},
plugins: {
"test-plugin": {
rules: {
"test-rule": {
create(context) {
resolvedSettings = context.settings;
return { };
}
}
}
}
}
});

await eslint.lintText("debugger;");

assert.deepStrictEqual(resolvedSettings.sharedData, circular);
});

it("in 'parserOptions'", async () => {
let resolvedParserOptions = null;

const circular = {};

circular.self = circular;

const eslint = new FlatESLint({
overrideConfigFile: true,
baseConfig: {
languageOptions: {
parser: {
parse(text, parserOptions) {
resolvedParserOptions = parserOptions;
}
},
parserOptions: {
testOption: circular
}
}
}
});

await eslint.lintText("debugger;");

assert.deepStrictEqual(resolvedParserOptions.testOption, circular);
});
});

});

describe("shouldUseFlatConfig", () => {
Expand Down