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.

Sign up for GitHub

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

Jump to bottom

feat!: Switch Linter to flat config by default #17851

Merged
merged 4 commits into from Dec 24, 2023
Merged

feat!: Switch Linter to flat config by default #17851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c2ab16
feat!: Switch Linter to flat config by default
nzakas Dec 13, 2023
232eac3
Fix failing tests
nzakas Dec 14, 2023
e163306
Fix fuzzer
nzakas Dec 20, 2023
667fb02
Ensure verify() defaults to flat config
nzakas Dec 22, 2023
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
2 changes: 1 addition & 1 deletion lib/cli-engine/cli-engine.js
View file
Open in desktop
Expand Up @@ -650,7 +650,7 @@ class CLIEngine {
});
const lintResultCache =
options.cache ? new LintResultCache(cacheFilePath, options.cacheStrategy) : null;
const linter = new Linter({ cwd: options.cwd });
const linter = new Linter({ cwd: options.cwd, configType: "eslintrc" });

/** @type {ConfigArray[]} */
const lastConfigArrays = [configArrayFactory.getConfigArrayForFile()];
Expand Down
48 changes: 24 additions & 24 deletions lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -1172,9 +1172,9 @@ class Linter {
* Initialize the Linter.
* @param {Object} [config] the config object
* @param {string} [config.cwd] path to a directory that should be considered as the current working directory, can be undefined.
* @param {"flat"|"eslintrc"} [config.configType="eslintrc"] the type of config used.
* @param {"flat"|"eslintrc"} [config.configType="flat"] the type of config used.
*/
constructor({ cwd, configType } = {}) {
constructor({ cwd, configType = "flat" } = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

When config argument is not passed to verify/verifyAndFix, Linter still runs eslintrc code paths, which is probably not what we want if "flat" is the new default.

An observable difference:

const { Linter } = require("eslint");

const linter = new Linter();

const results = linter.verify(`

    /* eslint no-undef: 2 */
    /* eslint-env browser */

    window;

`);

console.log(results); // [], but there should be a no-undef lint message in flat config mode

The cause is the condition it uses to switch between modes:

eslint/lib/linter/linter.js

Lines 1371 to 1372 in a9a17b3

if (config) {
if (configType === "flat") {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, in that case adding a default value to the constructor won't fix the problem, because we are still checking for config. Probably need to revisit all of that logic.

internalSlotsMap.set(this, {
cwd: normalizeCwd(cwd),
lastConfigArray: null,
Expand Down Expand Up @@ -1368,29 +1368,29 @@ class Linter {
? { filename: filenameOrOptions }
: filenameOrOptions || {};

if (config) {
if (configType === "flat") {

/*
* Because of how Webpack packages up the files, we can't
* compare directly to `FlatConfigArray` using `instanceof`
* because it's not the same `FlatConfigArray` as in the tests.
* So, we work around it by assuming an array is, in fact, a
* `FlatConfigArray` if it has a `getConfig()` method.
*/
let configArray = config;

if (!Array.isArray(config) || typeof config.getConfig !== "function") {
configArray = new FlatConfigArray(config, { basePath: cwd });
configArray.normalizeSync();
}
const configToUse = config ?? {};

return this._distinguishSuppressedMessages(this._verifyWithFlatConfigArray(textOrSourceCode, configArray, options, true));
}
if (configType !== "eslintrc") {

/*
* Because of how Webpack packages up the files, we can't
* compare directly to `FlatConfigArray` using `instanceof`
* because it's not the same `FlatConfigArray` as in the tests.
* So, we work around it by assuming an array is, in fact, a
* `FlatConfigArray` if it has a `getConfig()` method.
*/
let configArray = configToUse;

if (typeof config.extractConfig === "function") {
return this._distinguishSuppressedMessages(this._verifyWithConfigArray(textOrSourceCode, config, options));
if (!Array.isArray(configToUse) || typeof configToUse.getConfig !== "function") {
configArray = new FlatConfigArray(configToUse, { basePath: cwd });
configArray.normalizeSync();
}

return this._distinguishSuppressedMessages(this._verifyWithFlatConfigArray(textOrSourceCode, configArray, options, true));
}

if (typeof configToUse.extractConfig === "function") {
return this._distinguishSuppressedMessages(this._verifyWithConfigArray(textOrSourceCode, configToUse, options));
}

/*
Expand All @@ -1403,9 +1403,9 @@ class Linter {
* So we cannot apply multiple processors.
*/
if (options.preprocess || options.postprocess) {
return this._distinguishSuppressedMessages(this._verifyWithProcessor(textOrSourceCode, config, options));
return this._distinguishSuppressedMessages(this._verifyWithProcessor(textOrSourceCode, configToUse, options));
}
return this._distinguishSuppressedMessages(this._verifyWithoutProcessors(textOrSourceCode, config, options));
return this._distinguishSuppressedMessages(this._verifyWithoutProcessors(textOrSourceCode, configToUse, options));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion lib/rule-tester/rule-tester.js
View file
Open in desktop
Expand Up @@ -426,7 +426,7 @@ class RuleTester {
* @type {Object}
*/
this.rules = {};
this.linter = new Linter();
this.linter = new Linter({ configType: "eslintrc" });
Copy link
Member Author

Choose a reason for hiding this comment

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

Temporary measure until we remove RuleTester

}

/**
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/linter/code-path-analysis/code-path-analyzer.js
View file
Open in desktop
Expand Up @@ -31,7 +31,7 @@ const STANDARD_ESQUERY_OPTION = { visitorKeys: vk.KEYS, fallback: Traverser.getK

const expectedPattern = /\/\*expected\s+((?:.|[\r\n])+?)\s*\*\//gu;
const lineEndingPattern = /\r?\n/gu;
const linter = new Linter();
const linter = new Linter({ configType: "eslintrc" });

/**
* Extracts the content of `/*expected` comments from a given source code.
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/linter/code-path-analysis/code-path.js
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@

const assert = require("assert"),
{ Linter } = require("../../../../lib/linter");
const linter = new Linter();
const linter = new Linter({ configType: "eslintrc" });

//------------------------------------------------------------------------------
// Helpers
Expand Down
67 changes: 28 additions & 39 deletions tests/lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ describe("Linter", () => {
let linter;

beforeEach(() => {
linter = new Linter();
linter = new Linter({ configType: "eslintrc" });
});

afterEach(() => {
Expand Down Expand Up @@ -1307,7 +1307,7 @@ describe("Linter", () => {

describe("when evaluating code with invalid comments to enable rules", () => {
it("should report a violation when the config is not a valid rule configuration", () => {
const messages = linter.verify("/*eslint no-alert:true*/ alert('test');", {});
const messages = linter.verify("/*eslint no-alert:true*/ alert('test');");
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(
Expand All @@ -1330,7 +1330,7 @@ describe("Linter", () => {
});

it("should report a violation when the config violates a rule's schema", () => {
const messages = linter.verify("/* eslint no-alert: [error, {nonExistentPropertyName: true}]*/", {});
const messages = linter.verify("/* eslint no-alert: [error, {nonExistentPropertyName: true}]*/");
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(
Expand Down Expand Up @@ -3419,7 +3419,7 @@ var a = "test2";

it("should get cwd correctly in the context", () => {
const cwd = "cwd";
const linterWithOption = new Linter({ cwd });
const linterWithOption = new Linter({ cwd, configType: "eslintrc" });
let spy;

linterWithOption.defineRule("checker", {
Expand All @@ -3438,7 +3438,7 @@ var a = "test2";

it("should assign process.cwd() to it if cwd is undefined", () => {
let spy;
const linterWithOption = new Linter({ });
const linterWithOption = new Linter({ configType: "eslintrc" });

linterWithOption.defineRule("checker", {
create(context) {
Expand Down Expand Up @@ -6483,8 +6483,8 @@ var a = "test2";
let linter2 = null;

beforeEach(() => {
linter1 = new Linter();
linter2 = new Linter();
linter1 = new Linter({ configType: "eslintrc" });
linter2 = new Linter({ configType: "eslintrc" });
});

describe("rules", () => {
Expand Down Expand Up @@ -10100,7 +10100,7 @@ describe("Linter with FlatConfigArray", () => {

describe("when evaluating code with invalid comments to enable rules", () => {
it("should report a violation when the config is not a valid rule configuration", () => {
const messages = linter.verify("/*eslint no-alert:true*/ alert('test');", {});
const messages = linter.verify("/*eslint no-alert:true*/ alert('test');");
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(
Expand All @@ -10123,7 +10123,7 @@ describe("Linter with FlatConfigArray", () => {
});

it("should report a violation when a rule is configured using a string severity that contains uppercase letters", () => {
const messages = linter.verify("/*eslint no-alert: \"Error\"*/ alert('test');", {});
const messages = linter.verify("/*eslint no-alert: \"Error\"*/ alert('test');");
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(
Expand All @@ -10146,7 +10146,7 @@ describe("Linter with FlatConfigArray", () => {
});

it("should report a violation when the config violates a rule's schema", () => {
const messages = linter.verify("/* eslint no-alert: [error, {nonExistentPropertyName: true}]*/", {});
const messages = linter.verify("/* eslint no-alert: [error, {nonExistentPropertyName: true}]*/");
const suppressedMessages = linter.getSuppressedMessages();

assert.deepStrictEqual(
Expand Down Expand Up @@ -10175,7 +10175,7 @@ describe("Linter with FlatConfigArray", () => {
"foo(); // <-- expected no-undef error here"
].join("\n");

const messages = linter.verify(code, {});
const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

// different engines have different JSON parsing error messages
Expand Down Expand Up @@ -14288,7 +14288,7 @@ var a = "test2";
const code = BROKEN_TEST_CODE;

it("should report a violation with a useful parse error prefix", () => {
const messages = linter.verify(code, {});
const messages = linter.verify(code);
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 1);
Expand Down Expand Up @@ -14334,6 +14334,21 @@ var a = "test2";
});

});

it("should default to flat config mode when a config isn't passed", () => {

// eslint-env should not be honored
const messages = linter.verify("/*eslint no-undef:error*//*eslint-env browser*/\nwindow;");
const suppressedMessages = linter.getSuppressedMessages();

assert.strictEqual(messages.length, 1);
assert.strictEqual(messages[0].ruleId, "no-undef");
assert.strictEqual(messages[0].severity, 2);
assert.strictEqual(messages[0].line, 2);
assert.strictEqual(messages[0].column, 1);

assert.strictEqual(suppressedMessages.length, 0);
});
});

describe("getSourceCode()", () => {
Expand Down Expand Up @@ -14638,32 +14653,6 @@ var a = "test2";
});
});

describe("Mutability", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why these are still here. There's no way to test this using flat config.

let linter1 = null;
let linter2 = null;

beforeEach(() => {
linter1 = new Linter();
linter2 = new Linter();
});

describe("rules", () => {
it("with no changes, same rules are loaded", () => {
assert.sameDeepMembers(Array.from(linter1.getRules().keys()), Array.from(linter2.getRules().keys()));
});

it("loading rule in one doesn't change the other", () => {
linter1.defineRule("mock-rule", {
create: () => ({})
});

assert.isTrue(linter1.getRules().has("mock-rule"), "mock rule is present");
assert.isFalse(linter2.getRules().has("mock-rule"), "mock rule is not present");
});
});
});


describe("processors", () => {
let receivedFilenames = [];
let receivedPhysicalFilenames = [];
Expand Down Expand Up @@ -15245,7 +15234,7 @@ var a = "test2";
});

it("should not crash when invalid parentheses syntax is encountered", () => {
linter.verify("left = (aSize.width/2) - ()", {});
linter.verify("left = (aSize.width/2) - ()");
});

it("should not crash when let is used inside of switch case", () => {
Expand Down
27 changes: 8 additions & 19 deletions tests/lib/linter/rules.js
View file
Open in desktop
Expand Up @@ -58,30 +58,19 @@ describe("rules", () => {

const linter = new Linter();

const problems = linter.verify("foo", { rules: { "test-rule": "error" } });

assert.lengthOf(problems, 1);
assert.strictEqual(problems[0].message, "Definition for rule 'test-rule' was not found.");
assert.strictEqual(problems[0].line, 1);
assert.strictEqual(problems[0].column, 1);
assert.strictEqual(problems[0].endLine, 1);
assert.strictEqual(problems[0].endColumn, 2);
assert.throws(() => {
linter.verify("foo", { rules: { "test-rule": "error" } });
}, TypeError, "Could not find \"test-rule\" in plugin \"@\".");

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is a test file for the linter/rules.js module, which is only used in eslintrc mode so it might make more sense to keep the tests in eslintrc mode. However, I'm not sure why these particular tests were in this file since they relate to functionalities implemented in the Linter class, so seems best to revisit this at some later point.

});


it("should report a linting error that lists replacements if a rule is known to have been replaced", () => {
const linter = new Linter();
const problems = linter.verify("foo", { rules: { "no-arrow-condition": "error" } });

assert.lengthOf(problems, 1);
assert.strictEqual(
problems[0].message,
"Rule 'no-arrow-condition' was removed and replaced by: no-confusing-arrow, no-constant-condition"
);
assert.strictEqual(problems[0].line, 1);
assert.strictEqual(problems[0].column, 1);
assert.strictEqual(problems[0].endLine, 1);
assert.strictEqual(problems[0].endColumn, 2);

assert.throws(() => {
linter.verify("foo", { rules: { "no-arrow-condition": "error" } });
}, TypeError, "Key \"rules\": Key \"no-arrow-condition\": Rule \"no-arrow-condition\" was removed and replaced by \"no-confusing-arrow,no-constant-condition\".");
});
});

Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/utils/ast-utils.js
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ const ESPREE_CONFIG = {
range: true,
loc: true
};
const linter = new Linter();
const linter = new Linter({ configType: "eslintrc" });

describe("ast-utils", () => {
let callCounts;
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/shared/config-validator.js
View file
Open in desktop
Expand Up @@ -30,7 +30,7 @@ const assert = require("chai").assert,
// Helpers
//------------------------------------------------------------------------------

const linter = new Linter();
const linter = new Linter({ configType: "eslintrc" });

const mockRule = {
meta: {
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/source-code/source-code.js
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ const DEFAULT_CONFIG = {
range: true,
loc: true
};
const linter = new Linter();
const linter = new Linter({ configType: "eslintrc" });
const flatLinter = new Linter({ configType: "flat" });
const AST = espree.parse("let foo = bar;", DEFAULT_CONFIG),
TEST_CODE = "var answer = 6 * 7;",
Expand Down
2 changes: 1 addition & 1 deletion tests/tools/eslint-fuzzer.js
View file
Open in desktop
Expand Up @@ -23,7 +23,7 @@ describe("eslint-fuzzer", function() {
*/
this.timeout(15000); // eslint-disable-line no-invalid-this -- Mocha timeout

const linter = new eslint.Linter();
const linter = new eslint.Linter({ configType: "eslintrc" });
Copy link
Member Author

Choose a reason for hiding this comment

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

When we remove eslintrc mode, we'll need to revisit how the fuzzer works.

const coreRules = linter.getRules();
const fixableRuleNames = Array.from(coreRules)
.filter(rulePair => rulePair[1].meta && rulePair[1].meta.fixable)
Expand Down
2 changes: 1 addition & 1 deletion tools/fuzzer-runner.js
View file
Open in desktop
Expand Up @@ -12,7 +12,7 @@
const ProgressBar = require("progress");
const fuzz = require("./eslint-fuzzer");
const eslint = require("..");
const linter = new eslint.Linter();
const linter = new eslint.Linter({ configType: "eslintrc" });

//------------------------------------------------------------------------------
// Helpers
Expand Down