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.

Sign up for GitHub

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 unignore statements don't break file filtering #109

Closed
wants to merge 7 commits into from
Closed

fix: ensure unignore statements don't break file filtering #109

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ffe2a7d
Fix unignore statements breaking file filtering
comp615 Dec 5, 2024
aa9b5b7
cleanup a bit
comp615 Dec 5, 2024
eec6938
Different approach
comp615 Dec 6, 2024
fa89eb6
Add testing
comp615 Dec 27, 2024
e231178
Revert config
comp615 Dec 27, 2024
4c316ed
little clean
comp615 Dec 27, 2024
64efb17
more clear
comp615 Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions shared/configs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { FlatConfigItem, MatchedFile } from './types'
import { Minimatch } from 'minimatch'

const minimatchOpts = { dot: true }
const minimatchOpts = { dot: true, flipNegate: true }
const _matchInstances = new Map<string, Minimatch>()

function minimatch(file: string, pattern: string) {
Expand All @@ -13,7 +13,7 @@ function minimatch(file: string, pattern: string) {
return m.match(file)
}

export function getMatchedGlobs(file: string, glob: (string | string[])[]) {
function getMatchedGlobs(file: string, glob: (string | string[])[]) {
const globs = (Array.isArray(glob) ? glob : [glob]).flat()
return globs.filter(glob => minimatch(file, glob)).flat()
}
Expand All @@ -35,12 +35,21 @@ export function isGeneralConfig(config: FlatConfigItem) {
return (!config.files && !config.ignores) || isIgnoreOnlyConfig(config)
}

/**
* Given a list of matched globs, if an unignore (leading !) is the last one, then the file no longer matches the glob set
*/
function filterUnignoreGlobs(globs: string[]) {
if (!globs.length || globs[globs.length - 1].startsWith('!'))
return []
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if I follow, why would the unignore being the last makes the glob empty?

Copy link
Contributor Author

@comp615 comp615 Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was primarily trying to make the change as small as possible within the existing code. Basically for the globalIgnore case...they are equivalent because if it's unignored, then we don't return globs anyways (i.e. the code carries on). It's by definition impossible to be ignored if the globs match as an unignore.

image

However, I put up a different take I was thinking about that might be better for the per config case. It might be a little more/less of a brain twister. In the new code, we disconnect the pushing of the config from the pushing of the globs. This new case will then return the unignored globs, but not push the config, which may be more useful for debugging or more clear as shown below (i.e. it does match technically)

Old approach:
image

Revised approach:
image

In terms of why cueing on the last item works. Globs are evaluated in order, so you can walk through the list of globs for a config. Given **/foo/special.js:
**/*.js. - File is ignored
!**/special.js. - File is unignored (this negates the previous matches)
**/foo/special.js - File is reignored (this negates the negation, or rather readds it from an unmatched state)

So using that logic, since we walk them in order, if the last matching glob is a positive matcher, than effectively all the other globs don't matter, it matches; the result is the file is ignored. If the last matching glob is an unignore (!), than no matter what the order of the things preceding it is, the final result is that it undoes all the ignores and it doesn't match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But won't there be the case of mixing of different types?

**/*.js
**/foo/*.js
!**/foo/special/*
**/foo/special/**/*.ts
...

I think we might better run a proper test for each glob with their priority?

Copy link
Contributor Author

@comp615 comp615 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The globs we are cuing on have already been matched on line 69. So it's impossible to generate a file name that would cause both of these to match since they have mutually exclusive file extensions:
**/foo/*.js
**/foo/special/**/*.ts

Don't get me wrong, this is still not totally accurate with how ESLint actually works. For instance:

The pattern directory/** ignores the entire directory and its contents, so traversal will skip over the directory completely and you cannot unignore anything inside.

So it's very easy to make something that minimatch in the inspector will match but isn't in line with how ESLint works. But the issue is that we're fundamentally reimplementing the logic inside ESLint, so it will never be perfectly in sync and accurate. However, this change makes it more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antfu I think we're on slightly different schedules, but let me know if you want to hop on discord or something and take this off async delay!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still not sure if I understand the logic here. Could we have some test cases to at least guard the behavior here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! There was no testing in this package previously so I also had to setup and configure all that, so it's expanded the PR a bit. I used Mocha and followed the general style from other ESLint packages so that hopefully it's familiar/consistent

return globs
}

export function matchFile(
filepath: string,
configs: FlatConfigItem[],
ignoreOnlyConfigs: FlatConfigItem[],
): MatchedFile {
const globalIgnored = ignoreOnlyConfigs.flatMap(config => getMatchedGlobs(filepath, config.ignores!))
const globalIgnored = ignoreOnlyConfigs.flatMap(config => filterUnignoreGlobs(getMatchedGlobs(filepath, config.ignores!)))
if (globalIgnored.length) {
return {
filepath,
Expand All @@ -56,7 +65,7 @@ export function matchFile(
}
configs.forEach((config, index) => {
const positive = getMatchedGlobs(filepath, config.files || [])
const negative = getMatchedGlobs(filepath, config.ignores || [])
const negative = filterUnignoreGlobs(getMatchedGlobs(filepath, config.ignores || []))
if (!negative.length && positive.length)
result.configs.push(index)
result.globs.push(
Expand Down