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

fix: handle files with unspecified path in getRulesMetaForResults #16437

Merged
merged 4 commits into from Oct 31, 2022
Merged

fix: handle files with unspecified path in getRulesMetaForResults #16437

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
cf2c5a8
fix: handle files with unspecified path in `getRulesMetaForResults`
fasttime Oct 17, 2022
b756fda
Add a new unit test, move around an existing one
fasttime Oct 20, 2022
31d8e86
Add a unit test with linted and ignored files
fasttime Oct 24, 2022
90b7df2
Assert that there is an ignored file
fasttime Oct 29, 2022
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
48 changes: 36 additions & 12 deletions lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -161,6 +161,16 @@ function createRulesMeta(rules) {
}, {});
}

/**
* Return the absolute path of a file named `"__placeholder__.js"` in a given directory.
* This is used as a replacement for a missing file path.
* @param {string} cwd An absolute directory path.
* @returns {string} The absolute path of a file named `"__placeholder__.js"` in the given directory.
*/
function getPlaceholderPath(cwd) {
return path.join(cwd, "__placeholder__.js");
}

/** @type {WeakMap<ExtractedConfig, DeprecatedRuleInfo[]>} */
const usedDeprecatedRulesCache = new WeakMap();

Expand All @@ -177,7 +187,7 @@ function getOrFindUsedDeprecatedRules(eslint, maybeFilePath) {
} = privateMembers.get(eslint);
const filePath = path.isAbsolute(maybeFilePath)
? maybeFilePath
: path.join(cwd, "__placeholder__.js");
: getPlaceholderPath(cwd);
const config = configs.getConfig(filePath);

// Most files use the same config, so cache it.
Expand Down Expand Up @@ -427,7 +437,7 @@ function verifyText({
* `config.extractConfig(filePath)` requires an absolute path, but `linter`
* doesn't know CWD, so it gives `linter` an absolute path always.
*/
const filePathToVerify = filePath === "<text>" ? path.join(cwd, "__placeholder__.js") : filePath;
const filePathToVerify = filePath === "<text>" ? getPlaceholderPath(cwd) : filePath;
const { fixed, messages, output } = linter.verifyAndFix(
text,
configs,
Expand Down Expand Up @@ -524,6 +534,14 @@ function *iterateRuleDeprecationWarnings(configs) {
}
}

/**
* Creates an error to be thrown when an array of results passed to `getRulesMetaForResults` was not created by the current engine.
* @returns {TypeError} An error object.
*/
function createExtraneousResultsError() {
return new TypeError("Results object was not created from this ESLint instance.");
}

//-----------------------------------------------------------------------------
// Main API
//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -660,7 +678,10 @@ class FlatESLint {
}

const resultRules = new Map();
const { configs } = privateMembers.get(this);
const {
configs,
options: { cwd }
} = privateMembers.get(this);

/*
* We can only accurately return rules meta information for linting results if the
Expand All @@ -669,7 +690,7 @@ class FlatESLint {
* to let the user know we can't do anything here.
*/
if (!configs) {
throw new TypeError("Results object was not created from this ESLint instance.");
throw createExtraneousResultsError();
}

for (const result of results) {
Expand All @@ -678,20 +699,23 @@ class FlatESLint {
* Normalize filename for <text>.
*/
const filePath = result.filePath === "<text>"
? "__placeholder__.js" : result.filePath;

/*
* All of the plugin and rule information is contained within the
* calculated config for the given file.
*/
const config = configs.getConfig(filePath);
? getPlaceholderPath(cwd) : result.filePath;
const allMessages = result.messages.concat(result.suppressedMessages);

for (const { ruleId } of allMessages) {
if (!ruleId) {
continue;
}

/*
* All of the plugin and rule information is contained within the
* calculated config for the given file.
*/
const config = configs.getConfig(filePath);

if (!config) {
Copy link
Member

Choose a reason for hiding this comment

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

There may be an edge case here. getConfig returns undefined if a file is ignore by the configuration, so it doesn’t necessarily mean that the file wasn’t run in this instance. I think this will result in an error if the results contain a warning for a file that was ignored.

You can test this by passing in a file that is specifically ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I also thought about that. Now that #16402 has been fixed, the specific edge case you mention should be handled correctly. I will rebase the branch once again and add a new unit test to check the behavior in case an explicitly ignored file appears in the results when warnIgnored is set. Thanks for the feedback!

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a new unit test for when an anonymous file is ignored and linted with warnIgnored. A similar case, where the file has a specified path, is already covered in another test:

eslint/tests/lib/eslint/flat-eslint.js

Lines 3864 to 3867 in 740b208

const results = await engine.lintText("", { filePath: "ignored.js", warnIgnored: true });
const rulesMeta = engine.getRulesMetaForResults(results);
assert.deepStrictEqual(rulesMeta, {});

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a test with lintFiles() as well. We need to make sure that if there are multiple files linted and one ignored that this won't throw an error.

My concern: isFileIgnored() just does getConfig() === undefined, so what you are really testing here is if a file is ignored. Because of that, it still seems like there's an edge case here that will bite us.

I think it would be safe to remove this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ignored file results, as they don't have any messages with a ruleId, should be skipped by the continue statement a few lines above.

The point of checking if a config is found here is to avoid results linted by a different engine. If a result was not linted by the current engine, it may have no config without being ignored. This specific case is covered in this test. If this check is removed, getRulesMetaForResults will still fail, but with a non-specific error message (because config.plugins cannot be accessed at some point).

Adding a test with linted files and an ignored file at the same time is a good idea. There are no tests with multiple results so far. Thanks for the suggestion!

throw createExtraneousResultsError();
}
const rule = getRuleFromConfig(ruleId, config);

// ensure the rule exists
Expand Down Expand Up @@ -1029,7 +1053,7 @@ class FlatESLint {
const npmFormat = naming.normalizePackageName(normalizedFormatName, "eslint-formatter");

// TODO: This is pretty dirty...would be nice to clean up at some point.
formatterPath = ModuleResolver.resolve(npmFormat, path.join(cwd, "__placeholder__.js"));
formatterPath = ModuleResolver.resolve(npmFormat, getPlaceholderPath(cwd));
} catch {
formatterPath = path.resolve(__dirname, "../", "cli-engine", "formatters", `${normalizedFormatName}.js`);
}
Expand Down
96 changes: 94 additions & 2 deletions tests/lib/eslint/flat-eslint.js
View file
Open in desktop
Expand Up @@ -3707,7 +3707,7 @@ describe("FlatESLint", () => {

describe("getRulesMetaForResults()", () => {

it("should throw an error when results were not created from this instance", async () => {
it("should throw an error when this instance did not lint any files", async () => {
const engine = new FlatESLint({
overrideConfigFile: true
});
Expand Down Expand Up @@ -3744,7 +3744,99 @@ describe("FlatESLint", () => {
"var err = doStuff();\nif (err) console.log('failed tests: ' + err);\nprocess.exit(1);\n"
}
]);
}, /Results object was not created from this ESLint instance/u);
}, {
constructor: TypeError,
message: "Results object was not created from this ESLint instance."
});
});

it("should throw an error when results were created from a different instance", async () => {
const engine1 = new FlatESLint({
overrideConfigFile: true,
cwd: path.join(fixtureDir, "foo"),
overrideConfig: {
rules: {
semi: 2
}
}
});
const engine2 = new FlatESLint({
overrideConfigFile: true,
cwd: path.join(fixtureDir, "bar"),
overrideConfig: {
rules: {
semi: 2
}
}
});

const results1 = await engine1.lintText("1", { filePath: "file.js" });
const results2 = await engine2.lintText("2", { filePath: "file.js" });

engine1.getRulesMetaForResults(results1); // should not throw an error
assert.throws(() => {
engine1.getRulesMetaForResults(results2);
}, {
constructor: TypeError,
message: "Results object was not created from this ESLint instance."
});
});

it("should treat a result without `filePath` as if the file was located in `cwd`", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
cwd: path.join(fixtureDir, "foo", "bar"),
ignorePatterns: "*/**", // ignore all subdirectories of `cwd`
overrideConfig: {
rules: {
eqeqeq: "warn"
}
}
});

const results = await engine.lintText("a==b");
const rulesMeta = engine.getRulesMetaForResults(results);

assert.deepStrictEqual(rulesMeta.eqeqeq, coreRules.get("eqeqeq").meta);
});

it("should not throw an error if a result without `filePath` contains an ignored file warning", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
cwd: path.join(fixtureDir, "foo", "bar"),
ignorePatterns: "**"
});

const results = await engine.lintText("", { warnIgnored: true });
const rulesMeta = engine.getRulesMetaForResults(results);

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

it("should not throw an error if results contain linted files and one ignored file", async () => {
const engine = new FlatESLint({
overrideConfigFile: true,
cwd: getFixturePath(),
ignorePatterns: "passing*",
overrideConfig: {
rules: {
"no-undef": 2,
semi: 1
}
}
});

const results = await engine.lintFiles(["missing-semicolon.js", "passing.js", "undef.js"]);

assert(
results.some(({ messages }) => messages.some(({ message, ruleId }) => !ruleId && message.startsWith("File ignored"))),
"At least one file should be ignored but none is."
);

const rulesMeta = engine.getRulesMetaForResults(results);
Copy link
Member

Choose a reason for hiding this comment

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

Can we verify that a file is actually being ignored here before doing the other assertions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've added an assertion to make sure that one of the messages returned by lintFiles belongs to an ignored file.
I chose this implementation because it doesn't require calling another method on the engine under test.
Alternatively, we could pass the filePath of each result to isPathIgnored().

Copy link
Member

Choose a reason for hiding this comment

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

This works. 👍


assert.deepStrictEqual(rulesMeta["no-undef"], coreRules.get("no-undef").meta);
assert.deepStrictEqual(rulesMeta.semi, coreRules.get("semi").meta);
});

it("should return empty object when there are no linting errors", async () => {
Expand Down