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: Empty glob pattern incorrectly expands to "/**" #11476

Merged
merged 2 commits into from Mar 15, 2019
Merged

Fix: Empty glob pattern incorrectly expands to "/**" #11476

merged 2 commits into from Mar 15, 2019

Conversation

bdchauvette
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to 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:

Tell us about your environment

  • ESLint Version: 5.15.0
  • Node Version: 10.15.2
  • npm Version: 6.4.1

What parser (default, Babel-ESLint, etc.) are you using?

Any; this bug occurs before ESLint parses anything.

Please show your full configuration:

This works without a configuration (e.g. npx eslint "" in any folder)

What did you do? Please include the actual source code causing the issue.

I passed an empty string to ESLint on the command line.

What did you expect to happen?

The empty string would be ignored.

What actually happened? Please include the actual, raw output from ESLint.

The empty string got expanded to /** and ESLint tried to lint my whole filesystem.

See a live example on repl.it (download as zip)

What changes did you make? (Give an overview)

In lib/util/glob-utils.js, empty strings are now filtered out before mapping over the glob patterns.

See a live example on repl.it (download as zip)

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

Nope, thanks for all the hard work on ESLint! 🙇‍♂️

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 5, 2019
Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the PR! I can reproduce this issue. However, I don't think the fix is quite correct.

Unfortunately it seems like this is not the first time we've had this issue: This was originally reported in #8362, and subsequently fixed in #8364 (using an implementation similar to the one in this PR). Later, the processing of empty files was ostensibly moved to lib/cli-engine.js in 15d77bd. (Looking at it now, I see that 15d77bd introduced the bug again because the relevant call to resolveFileGlobPatterns is in glob-utils.js itself, not in cli-engine.js.)

Based on my commit message from 15d77bd, I think it was necessary to move the processing of empty files out of that glob-utils function because the resolveFileGlobPatterns function needs to return an array with the same length as the array it received as input. (This is necessary so that the processed globs can be mapped back to the original globs here, if needed for the purpose of displaying an error message.)

tl;dr: I think it would be better to filter for empty files somewhere in the listFilesToProcess function (maybe right before this line?) rather than in resolveFileGlobPatterns.

@not-an-aardvark not-an-aardvark added bug ESLint is working incorrectly 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 Mar 5, 2019
@bdchauvette
Copy link
Contributor Author

Thanks for the quick feedback! I tried to search for related issues on github, but didn't see anything. My issue-fu is weak 🤦‍♂️

I'll update the PR 👍

It didn't work inside resolveFileGlobPatterns because that function
needs to return an array of the same length as its input (15d77bd).
@bdchauvette
Copy link
Contributor Author

bdchauvette commented Mar 5, 2019

I've moved the ignoring out of resolveFileGlobPatterns, but it was a bit tricky because resolveFileGlobPatterns still needs to handle empty patterns to avoid turning them into /**.

The approach I've gone with is to:

Overall, this has the effect of making the empty pattern a special case of ignored files, which may or may not be a good idea? 👀

Happy to change again if this approach doesn't work 👍

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@btmills btmills merged commit a5dae7c into eslint:master Mar 15, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Sep 12, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants