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: Pass internal config paths in FileEnumerator default (fixes #13789) #13792

Merged
merged 2 commits into from Oct 27, 2020

Conversation

btmills
Copy link
Member

@btmills btmills commented Oct 26, 2020

Prerequisites checklist

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 autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

When CLIEngine instantiates FileEnumerator, it explicitly passes its own CascadingConfigArrayFactory instance, which now includes values for builtInRules, loadRules, eslintRecommendedPath, and eslintAllPath. This is the only place that ESLint core instantiates FileEnumerator, so core does not rely on the constructor's default value for configArrayFactory.

After CascadingConfigArrayFactory was extracted into @eslint/eslintrc, it no longer assumed values for eslintRecommendedPath and eslintAllPath, which CLIEngine now provides. If those values are not passed, as is the case with the default CascadingConfigArrayFactory for configArrayFactory, file enumeration will encounter the exception that was reported in #13789.

From the perspective of ESLint core, the default value for configArrayFactory is dead code. However, even though FileEnumerator is an undocumented API, it's called by eslint-plugin-import's no-unused-modules rule, which hits the exception reproduced by this test.

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

FileEnumerator is an undocumented API, so we probably shouldn't be encouraging people to use it, but eslint-plugin-import/no-unused-modules already does. We don't have a supported API to achieve the same purpose, but the pre-lint step contemplated in RFC42 would probably serve that purpose.

When `CLIEngine` instantiates `FileEnumerator`, it explicitly passes its
own `CascadingConfigArrayFactory` instance, which now includes values
for `builtInRules`, `loadRules`, `eslintRecommendedPath`, and
`eslintAllPath`. This is the only place that ESLint core instantiates
`FileEnumerator`, so core does not rely on the constructor's default
value for `configArrayFactory`.

After `CascadingConfigArrayFactory` was extracted into
`@eslint/eslintrc`, it no longer assumed values for
`eslintRecommendedPath` and `eslintAllPath`, which `CLIEngine` now
provides. If those values are not passed, as is the case with the
default `CascadingConfigArrayFactory` for `configArrayFactory`, file
enumeration will encounter the exception that was reported in #13789.

From the perspective of ESLint core, the default value for
`configArrayFactory` is dead code. However, even though `FileEnumerator`
is an undocumented API, it's called by `eslint-plugin-import`'s
`no-unused-modules` rule, which hits the exception reproduced by this
test.
When `CLIEngine` instantiates `FileEnumerator`, it explicitly passes its
own `CascadingConfigArrayFactory` instance, which now includes values
for `builtInRules`, `loadRules`, `eslintRecommendedPath`, and
`eslintAllPath`. This is the only place that ESLint core instantiates
`FileEnumerator`, so core does not rely on the constructor's default
value for `configArrayFactory`.

After `CascadingConfigArrayFactory` was extracted into
`@eslint/eslintrc`, it no longer assumed values for
`eslintRecommendedPath` and `eslintAllPath`, which `CLIEngine` now
provides. If those values are not passed, as is the case with the
default `CascadingConfigArrayFactory` for `configArrayFactory`, file
enumeration will encounter the exception that was reported in #13789.

From the perspective of ESLint core, the default value for
`configArrayFactory` is dead code. However, even though `FileEnumerator`
is an undocumented API, it's called by `eslint-plugin-import`'s
`no-unused-modules` rule, which hits the exception reproduced by this
test.
@btmills btmills added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features regression Something broke patch candidate This issue may necessitate a patch release in the next few days 3rd party plugin This is an issue related to a 3rd party plugin, config, or parser labels Oct 26, 2020
@eslint-deprecated
Copy link

Hi @btmills!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@btmills btmills changed the title Fix: Pass internal config paths in FileEnumerator default (fixes #13789) Fix: Pass internal config paths in FileEnumerator default (fixes #13789) Oct 26, 2020
@btmills
Copy link
Member Author

btmills commented Oct 26, 2020

Fixed the commit message check by trimming trailing whitespace left over from a copy/paste.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM!

I also checked this with changes from eslint/eslintrc#14 + some built-in rules configured, and it works well (had concerns if the builtInRules argument is also necessary; that's actually how I found out that we were not validating built-in rules).

@btmills
Copy link
Member Author

btmills commented Oct 27, 2020

I also checked this with changes from eslint/eslintrc#14 + some built-in rules configured, and it works well (had concerns if the builtInRules argument is also necessary; that's actually how I found out that we were not validating built-in rules).

Thanks for double checking. It makes sense that file enumeration would need to know paths to internal configs but not need to touch rules. I'm glad you checked and found the validation issue!

@btmills btmills merged commit aeef485 into master Oct 27, 2020
@btmills btmills deleted the issue13789 branch October 27, 2020 03:07
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 26, 2021
@eslint-github-bot 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 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3rd party plugin This is an issue related to a 3rd party plugin, config, or parser archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features patch candidate This issue may necessitate a patch release in the next few days regression Something broke
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants