fix: handle files with unspecified path in `getRulesMetaForResults` #16437

fasttime · 2022-10-17T19:48:13Z

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 #16410

What changes did you make? (Give an overview)

In getRulesMetaForResults, files with an unspecified path are now treated as if they were located inside cwd.
In getRulesMetaForResults, when a result referencing a rule has no config, we will explicitly throw an error with a descriptive message.
Added top-level, internal functions getPlaceholderPath and createExtraneousResultsError to avoid code repetition.
Added two new unit tests.
Renamed an existing unit test to better disambiguate it from a new one. Also changed the assertion to check both error message and constructor.

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

netlify · 2022-10-17T19:48:19Z

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	`90b7df2`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/635d1277201d190008c3efb1

mdjermanovic

LGTM, but would like @nzakas to take a look.

mdjermanovic · 2022-10-18T22:08:49Z

Oh, and there's a merge conflict.

fasttime · 2022-10-18T22:55:45Z

Thanks for reviewing @mdjermanovic! I've rebased the branch and fixed the merge conflict.

nzakas · 2022-10-19T17:30:57Z

lib/eslint/flat-eslint.js

+                 */
+                const config = configs.getConfig(filePath);
+
+                if (!config) {


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.

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!

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, {});

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.

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!

* In `getRulesMetaForResults`, files with an unspecified path are now treated as if they were located inside `cwd`. * In `getRulesMetaForResults`, when a result referencing a rule has no config, we will explicitly throw an error with a descriptive message. * Added top-level, internal functions `getPlaceholderPath` and `createExtraneousResultsError` to avoid code repetition. * Added two new unit tests. * Renamed an existing unit test to better disambiguate it from a new one. Also changed the assertion to check both error message and constructor. Fixes #16410

fasttime · 2022-10-20T10:46:52Z

Rebased and updated as suggested by @nzakas. Since the new unit tests are only relevant to FlatESLint, I put them all at the start of the describe block, just under another existing, FlatESLint-specific unit test.

fasttime · 2022-10-24T07:43:03Z

Added another unit test with lintFiles() as suggested by @nzakas.

nzakas · 2022-10-28T21:02:42Z

tests/lib/eslint/flat-eslint.js

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


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

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().

This works. 👍

nzakas

LGTM. Thanks so much for this.

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.26.0` -> `8.27.0`](https://renovatebot.com/diffs/npm/eslint/8.26.0/8.27.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.27.0`](https://github.com/eslint/eslint/releases/tag/v8.27.0) [Compare Source](eslint/eslint@v8.26.0...v8.27.0) #### Features - [`f14587c`](eslint/eslint@f14587c) feat: new `no-new-native-nonconstructor` rule ([#16368](eslint/eslint#16368)) (Sosuke Suzuki) - [`978799b`](eslint/eslint@978799b) feat: add new rule `no-empty-static-block` ([#16325](eslint/eslint#16325)) (Sosuke Suzuki) - [`69216ee`](eslint/eslint@69216ee) feat: no-empty suggest to add comment in empty BlockStatement ([#16470](eslint/eslint#16470)) (Nitin Kumar) - [`319f0a5`](eslint/eslint@319f0a5) feat: use `context.languageOptions.ecmaVersion` in core rules ([#16458](eslint/eslint#16458)) (Milos Djermanovic) #### Bug Fixes - [`c3ce521`](eslint/eslint@c3ce521) fix: Ensure unmatched glob patterns throw an error ([#16462](eslint/eslint#16462)) (Nicholas C. Zakas) - [`886a038`](eslint/eslint@886a038) fix: handle files with unspecified path in `getRulesMetaForResults` ([#16437](eslint/eslint#16437)) (Francesco Trotta) #### Documentation - [`ce93b42`](eslint/eslint@ce93b42) docs: Stylelint property-no-unknown ([#16497](eslint/eslint#16497)) (Nick Schonning) - [`d2cecb4`](eslint/eslint@d2cecb4) docs: Stylelint declaration-block-no-shorthand-property-overrides ([#16498](eslint/eslint#16498)) (Nick Schonning) - [`0a92805`](eslint/eslint@0a92805) docs: stylelint color-hex-case ([#16496](eslint/eslint#16496)) (Nick Schonning) - [`74a5af4`](eslint/eslint@74a5af4) docs: fix stylelint error ([#16491](eslint/eslint#16491)) (Milos Djermanovic) - [`324db1a`](eslint/eslint@324db1a) docs: explicit stylelint color related rules ([#16465](eslint/eslint#16465)) (Nick Schonning) - [`94dc4f1`](eslint/eslint@94dc4f1) docs: use Stylelint for HTML files ([#16468](eslint/eslint#16468)) (Nick Schonning) - [`cc6128d`](eslint/eslint@cc6128d) docs: enable stylelint declaration-block-no-duplicate-properties ([#16466](eslint/eslint#16466)) (Nick Schonning) - [`d03a8bf`](eslint/eslint@d03a8bf) docs: Add heading to justification explanation ([#16430](eslint/eslint#16430)) (Maritaria) - [`8a15968`](eslint/eslint@8a15968) docs: add Stylelint configuration and cleanup ([#16379](eslint/eslint#16379)) (Nick Schonning) - [`9b0a469`](eslint/eslint@9b0a469) docs: note commit messages don't support scope ([#16435](eslint/eslint#16435)) (Andy Edwards) - [`1581405`](eslint/eslint@1581405) docs: improve context.getScope() docs ([#16417](eslint/eslint#16417)) (Ben Perlmutter) - [`b797149`](eslint/eslint@b797149) docs: update formatters template ([#16454](eslint/eslint#16454)) (Milos Djermanovic) - [`5ac4de9`](eslint/eslint@5ac4de9) docs: fix link to formatters on the Core Concepts page ([#16455](eslint/eslint#16455)) (Vladislav) - [`33313ef`](eslint/eslint@33313ef) docs: core-concepts: fix link to semi rule ([#16453](eslint/eslint#16453)) (coderaiser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1628 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Oct 17, 2022

fasttime marked this pull request as ready for review October 17, 2022 20:10

mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 18, 2022

mdjermanovic approved these changes Oct 18, 2022

View reviewed changes

nzakas requested changes Oct 19, 2022

View reviewed changes

fasttime added 2 commits October 20, 2022 06:59

Add a new unit test, move around an existing one

b756fda

Add a unit test with linted and ignored files

31d8e86

nzakas reviewed Oct 28, 2022

View reviewed changes

Assert that there is an ignored file

90b7df2

nzakas approved these changes Oct 31, 2022

View reviewed changes

nzakas merged commit 886a038 into eslint:main Oct 31, 2022

nzakas added the contributor pool label Oct 31, 2022

eslint-github-bot bot locked and limited conversation to collaborators Apr 30, 2023

eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle files with unspecified path in `getRulesMetaForResults` #16437

fix: handle files with unspecified path in `getRulesMetaForResults` #16437

fasttime commented Oct 17, 2022

netlify bot commented Oct 17, 2022 •

edited

mdjermanovic left a comment

mdjermanovic commented Oct 18, 2022

fasttime commented Oct 18, 2022

nzakas Oct 19, 2022

fasttime Oct 19, 2022

fasttime Oct 20, 2022

nzakas Oct 21, 2022

fasttime Oct 22, 2022

fasttime commented Oct 20, 2022

fasttime commented Oct 24, 2022

nzakas Oct 28, 2022

fasttime Oct 29, 2022

nzakas Oct 31, 2022

nzakas left a comment

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

	assert.deepStrictEqual(rulesMeta, {});

fix: handle files with unspecified path in getRulesMetaForResults #16437

fix: handle files with unspecified path in getRulesMetaForResults #16437

Conversation

fasttime commented Oct 17, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

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

netlify bot commented Oct 17, 2022 • edited

✅ Deploy Preview for docs-eslint canceled.

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic commented Oct 18, 2022

fasttime commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fasttime commented Oct 20, 2022

fasttime commented Oct 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas left a comment

Choose a reason for hiding this comment

fix: handle files with unspecified path in `getRulesMetaForResults` #16437

fix: handle files with unspecified path in `getRulesMetaForResults` #16437

netlify bot commented Oct 17, 2022 •

edited