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: Ensure globbing doesn't include subdirectories #16272

Merged
merged 5 commits into from Sep 12, 2022
Merged

fix: Ensure globbing doesn't include subdirectories #16272

merged 5 commits into from Sep 12, 2022

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Sep 1, 2022

Fixes #16260

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

What changes did you make? (Give an overview)

I changed the resolution mechanism for globbing. Previously, we were automatically looking for every glob pattern inside of the cwd, which could result in more files than necessary being linted. Now, we only look at glob patterns relative to the directory passed in on the command line.

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

Please take a look at the new shallow-glob fixture to make sure it makes sense. I verified that the test using these files failed prior to this change and passed after this change.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Sep 1, 2022
@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 8fc387c
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6318f67e8ce84b0009f6f4b7

// check if the pattern is inside the directory or not
const patternRelativeToFilePath = path.relative(filePath, fullFilePattern);

if (patternRelativeToFilePath.startsWith("..")) {
return false;
}
Copy link
Member

@mdjermanovic mdjermanovic Sep 2, 2022

Choose a reason for hiding this comment

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

This won't work well when ** (or anything else that isn't a literal directory name) is in the middle of filePattern.

For example:

// eslint.config.js
module.exports = {
    files: ["foo/**/*.md"],
    // ...configuration for .md files
};

Command eslint foo/bar, where foo/bar is an existing directory, would not find .md files in foo/bar directory because patternRelativeToFilePath will be calculated as ..\**\*.md.

Copy link
Member

@mdjermanovic mdjermanovic Sep 2, 2022

Choose a reason for hiding this comment

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

It also doesn't work when the pattern starts with a non-literal (except for the special-cased **).

For example, if the config entry has files: ["{foo,bar}/*.md"], command eslint foo won't find .md files in foo because patternRelativeToFilePath is ..\{foo,bar}\*.md, so the pattern gets filtered out here.

Copy link
Member Author

@nzakas nzakas Sep 2, 2022

Choose a reason for hiding this comment

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

Hmmm, yeah, this is the trickiest part of all of this. Do you have any suggestions?

Copy link
Member

@mdjermanovic mdjermanovic Sep 4, 2022

Choose a reason for hiding this comment

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

For this step (filtering out patterns that can't match inside the given directory), I think we could use minimatch with partial: true. But then there's the next step where we should adjust the remaining patterns to match only inside the given directory, which seems overly complex.

Copy link
Member

@mdjermanovic mdjermanovic Sep 4, 2022

Choose a reason for hiding this comment

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

Some alternative approaches:

  1. Use globby to simply get all files in the directory (if the given directory is foo/bar, pass foo/bar/**) and then filter out those that don't match at least one pattern that doesn't end with a *. I think this solution is the least error-prone.
  2. Negative patterns work as logical AND, so it might be possible to get the intersection by appending some form of double negation to the list of patterns.

Copy link
Member Author

@nzakas nzakas Sep 5, 2022

Choose a reason for hiding this comment

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

Thanks for the suggestions. These were super helpful. I went with a minimatch-based approach that seems to work. I didn't need to use partial: true because we actually do have the full path at that point.

@mdjermanovic 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 Sep 2, 2022
@nzakas
Copy link
Member Author

nzakas commented Sep 2, 2022

I can't reproduce the lint error locally, so I have no idea what's going on there.

@nzakas
Copy link
Member Author

nzakas commented Sep 2, 2022

Nevermind, I can reproduce it now. Didn't notice there was a separate command for linting docs JavaScript files.

@nzakas
Copy link
Member Author

nzakas commented Sep 2, 2022

@mdjermanovic I could use your help figuring out why the linting in the docs directory is broken. I'm a little confused as to why this lint step is separate and where exactly it's reading its config from.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 4, 2022

why this lint step is separate and where exactly it's reading its config from.

It's separate from the main lint because linting docs requires docs/node_modules installed. The reasoning is in this comment:

eslint/Makefile.js

Lines 508 to 518 in 42bfbd7

/*
* In order to successfully lint JavaScript files in the `docs` directory, dependencies declared in `docs/package.json`
* would have to be installed in `docs/node_modules`. In particular, eslint-plugin-node rules examine `docs/node_modules`
* when analyzing `require()` calls from CJS modules in the `docs` directory. Since our release process does not run `npm install`
* in the `docs` directory, linting would fail and break the release. Also, working on the main `eslint` package does not require
* installing dependencies declared in `docs/package.json`, so most contributors will not have `docs/node_modules` locally.
* Therefore, we add `--ignore-pattern docs` to exclude linting the `docs` directory from this command.
* There is a separate command `target.lintDocsJS` for linting JavaScript files in the `docs` directory.
*/
echo("Validating JavaScript files");
lastReturn = exec(`${ESLINT}${fix ? "--fix" : ""} . --ignore-pattern docs`);

It uses the same top-level .eslintrc.js/eslint.config.js config file.

The problem will be fixed by #16274

@nzakas
Copy link
Member Author

nzakas commented Sep 5, 2022

Oh nice! I'll rebase on top of that.

jugalthakkar and others added 3 commits Sep 5, 2022
* docs: update docs package ver from main on release

fixes #16212

* docs: read docs package ver instead of main

fixes #16212

* docs: add newline to updated docs package json

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* docs: update docs package json version

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
}

// check if the pattern matches
if (minimatch(filePath, path.dirname(fullFilePattern))) {
Copy link
Member

@mdjermanovic mdjermanovic Sep 7, 2022

Choose a reason for hiding this comment

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

I still think we would need { partial: true } for this approach.

Consider the following example.

Config:

{
    files: ["t*/internal-rules/*.js"],
    // ...
}

Command:

eslint tools

The "t*/internal-rules/*.js" pattern from the config should not be filtered out, because it can match something under the tools directory?

Current behavior:

filePath: D:\projects\eslint\tools
fullFilePattern: D:\projects\eslint\t*\internal-rules\*.js
path.dirname(fullFilePattern): D:\projects\eslint\t*\internal-rules
minimatch(filePath, path.dirname(fullFilePattern)): false

I think what we need is:

if (minimatch(filePath, fullFilePattern, { partial: true })) {
    return true;
}
filePath: D:\projects\eslint\tools
fullFilePattern: D:\projects\eslint\t*\internal-rules\*.js
minimatch(filePath, fullFilePattern, { partial: true }): true

Copy link
Member Author

@nzakas nzakas Sep 7, 2022

Choose a reason for hiding this comment

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

Ah gotcha. I think I misunderstood the purpose of partial: true.

Copy link
Member

@btmills btmills left a comment

Unfortunately mdjermanovic hasn't been able to follow up on the open discussions. Mitigating that, 1) the suggestions from those discussions look to have been implemented as discussed, and 2) this is use-at-your-own-risk code that's easier to change post-release.

@btmills btmills merged commit c6900f8 into main Sep 12, 2022
20 checks passed
@btmills btmills deleted the issue16260 branch Sep 12, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 22, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | patch | [`8.23.0` -> `8.23.1`](https://renovatebot.com/diffs/npm/eslint/8.23.0/8.23.1) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.23.1`](https://github.com/eslint/eslint/releases/tag/v8.23.1)

[Compare Source](eslint/eslint@v8.23.0...v8.23.1)

#### Bug Fixes

-   [`b719893`](eslint/eslint@b719893) fix: Upgrade eslintrc to stop redefining plugins ([#&#8203;16297](eslint/eslint#16297)) (Brandon Mills)
-   [`734b54e`](eslint/eslint@734b54e) fix: improve autofix for the `prefer-const` rule ([#&#8203;16292](eslint/eslint#16292)) (Nitin Kumar)
-   [`6a923ff`](eslint/eslint@6a923ff) fix: Ensure that glob patterns are normalized ([#&#8203;16287](eslint/eslint#16287)) (Nicholas C. Zakas)
-   [`c6900f8`](eslint/eslint@c6900f8) fix: Ensure globbing doesn't include subdirectories ([#&#8203;16272](eslint/eslint#16272)) (Nicholas C. Zakas)

#### Documentation

-   [`16cba3f`](eslint/eslint@16cba3f) docs: fix mobile double tap issue ([#&#8203;16293](eslint/eslint#16293)) (Sam Chen)
-   [`e098b5f`](eslint/eslint@e098b5f) docs: keyboard control to search results ([#&#8203;16222](eslint/eslint#16222)) (Shanmughapriyan S)
-   [`1b5b2a7`](eslint/eslint@1b5b2a7) docs: add Consolas font and prioritize resource loading ([#&#8203;16225](eslint/eslint#16225)) (Amaresh  S M)
-   [`1ae8236`](eslint/eslint@1ae8236) docs: copy & use main package version in docs on release ([#&#8203;16252](eslint/eslint#16252)) (Jugal Thakkar)
-   [`279f0af`](eslint/eslint@279f0af) docs: Improve id-denylist documentation ([#&#8203;16223](eslint/eslint#16223)) (Mert Ciflikli)

#### Chores

-   [`38e8171`](eslint/eslint@38e8171) perf: migrate rbTree to js-sdsl ([#&#8203;16267](eslint/eslint#16267)) (Zilong Yao)
-   [`1c388fb`](eslint/eslint@1c388fb) chore: switch nyc to c8 ([#&#8203;16263](eslint/eslint#16263)) (唯然)
-   [`67db10c`](eslint/eslint@67db10c) chore: enable linting `.eleventy.js` again ([#&#8203;16274](eslint/eslint#16274)) (Milos Djermanovic)
-   [`42bfbd7`](eslint/eslint@42bfbd7) chore: fix `npm run perf` crashes ([#&#8203;16258](eslint/eslint#16258)) (唯然)

</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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xOTQuNSIsInVwZGF0ZWRJblZlciI6IjMyLjE5NC41In0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1543
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion 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.

Bug: [new config system] path to a directory lints files outside that directory
4 participants