Bug: FlatESLint getRulesMetaForResults fails on anonymous files when option cwd is set

[fasttime]

Environment

Node version: v18.10.0
npm version: v8.19.2
Local ESLint version: v8.25.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 21.6.0

What parser are you using?

Default (Espree)

What did you do?

Using FlatESLint, the method getRulesMetaForResults thows an error for certain values of the option cwd when passed a result of an anonymous file. This happens whenever cwd is set to a subdirectory of the current directory.

The following repro shows the problem.

import { join } from 'path';

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

const cwd = join(process.cwd(), 'test'); // subdir 'test' doesn't need to exist for this repro.
const eslint = new FlatESLint({
    cwd,
    overrideConfig: { rules: { 'no-undef': 'warn' } },
    overrideConfigFile: true,
});
const results = await eslint.lintText('foo()');
eslint.getRulesMetaForResults(results); // throws TypeError

What did you expect to happen?

Calling getRulesMetaForResults with the result of an anonymous file should not throw an error, regardless of the engine's cwd.

What actually happened?

The above repro fails with an error like this:

.../node_modules/eslint/lib/config/flat-config-helpers.js:54
    const plugin = config.plugins && config.plugins[pluginName];
                          ^

TypeError: Cannot read properties of undefined (reading 'plugins')
    at getRuleFromConfig (.../node_modules/eslint/lib/config/flat-config-helpers.js:54:27)
    at FlatESLint.getRulesMetaForResults (.../node_modules/eslint/lib/eslint/flat-eslint.js:703:30)
    at file://.../example.mjs:12:8

Participation

• I am willing to submit a pull request for this issue.

Additional comments

What strikes me in the implementation are the different values used to call configs.getConfig in the case of an anonymous file: lintText calls configs.getConfig with a full path like ${cwd}/__placeholder__.js

eslint/lib/eslint/flat-eslint.js

Line 956 in 173e820

const resolvedFilename = path.resolve(cwd, filePath || "__placeholder__.js");

whereas getRulesMetaForResults ends up calling configs.getConfig("__placeholder__.js")

eslint/lib/eslint/flat-eslint.js

Lines 689 to 699 in 173e820

	/*
	* 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);

Then it finds no config and it fails.

Another issue related to this is the unhelpful message of the TypeError.
When getRulesMetaForResults finds no matching config, it could simply tell the user that the results were not created from the current instance of ESLint (not the case here, but still).
The error message is already there, but it is currently only used in a more limited edge case:

eslint/lib/eslint/flat-eslint.js

Line 684 in 173e820

throw new TypeError("Results object was not created from this ESLint instance.");

[mdjermanovic]

Thanks for finding this, PR is welcome!

whereas getRulesMetaForResults ends up calling configs.getConfig("__placeholder__.js")

That's the bug here, "<text>" in getRulesMetaForResults should be resolved the same way as unspecified filePath in lintText.

When getRulesMetaForResults finds no matching config, it could simply tell the user that the results were not created from the current instance of ESLint (not the case here, but still).

Makes sense, but we should probably account for the case when the file is ignored (so it has no config) but appears in the results with a "File ignored..." warning.

[fasttime]

Thanks for the feedback @mdjermanovic.

we should probably account for the case when the file is ignored (so it has no config) but appears in the results with a "File ignored..." warning.

Absolutely. Looking at the code, the calculated config is only ever used to fetch the rules meta for those results that contain messages (or message with a ruleId, at least after #16402 is fixed).

eslint/lib/eslint/flat-eslint.js

Lines 696 to 703 in 173e820

	* All of the plugin and rule information is contained within the
	* calculated config for the given file.
	*/
	const config = configs.getConfig(filePath);
	const allMessages = result.messages.concat(result.suppressedMessages);
	for (const { ruleId } of allMessages) {
	const rule = getRuleFromConfig(ruleId, config);

Currently, files without rule-related messages have their configs calculated but never used, not even validated. These include ignored files, files without lint problems and files with syntax errors.

Now this leaves room for some optimization: we could calculate the config lazily, and do it only if one of the messages has a ruleId. If a config is sought but not found, throw an error with a useful error message. Results for ignored files would not throw an error, because they don't contain any messages with a ruleId. Same thing for valid files and for files with syntax errors.

Another option is to keep the config calculation for every result, except if the file is ignored. If no config is found, then throw an error.
This means that results without messages would start to have their file paths validated and produce errors if no config is found. So this change would produce more errors than before.

To me, the first solution seems preferable, because it's more conservative and we would not need to check if a file is ignored beforehand. Any thoughts?

[mdjermanovic]

getConfig() can be costly, but at that point config for the filePath should have already been calculated (because the file was linted) and cached. so it will just return the cached config object. After finding a message that has ruleId, we could check if config is not an object and throw a better error message, as it is unexpected that ignored files have messages from rules.

[fasttime]

@mdjermanovic That sounds reasonable. So we'll just (re)load the config whenever a message with a ruleId is encountered and check if it's an object or undefined.
I can work on this. Shall I extend #16409 or create a new PR?

[mdjermanovic]

Shall I extend #16409 or create a new PR?

As #16409 is already being reviewed, it might be better to create a new PR that fixes the two problems from this issue (handling the "<text>" file path, and improving the error message when file doesn't have a config).