Bug: [flat config] Global ignores matches differently from non-global ignores

[mxxk]

Environment

Node version: v18.12.1
npm version: 8.19.2
Local ESLint version: v8.41.0
Global ESLint version: none
Operating System: Linux or MacOS

What parser are you using?

Default (Espree)

What did you do?

Configuration

eslint-global-ignore.config.js

export default [
  {
    ignores: ['ignored/'],
  },
  {
    rules: {
      'no-undef': 'error',
    },
  },
];

eslint-files-ignore.config.js

export default [
  {
    files: ['**/*.js'],
    ignores: ['ignored/'],
    rules: {
      'no-undef': 'error',
    },
  },
];

file.js and ignored/file.js

console.log();

What did you expect to happen?

Not sure what is the correct intended behavior here, so raising this issue for the maintainers' consideration. Based on the documentation (Globally ignoring files with ignores), it looks like ignores can match and ignore an entire directory tree:

{
  // Ignores directory tree
  ignores: ["ignored/"],
}

However, if the same ignores pattern is used in a configuration block which also has files, then the pattern is interpreted completely differently, and does not have the same effect:

{
  files: ["**/*.js"],
  // Does *not* ignore directory tree
  ignores: ["ignored/"],
}

Rather, to ignore the entire top-level ignored tree, the ignores pattern needs to be adjusted as follows:

 {
   files: ["**/*.js"],
-  ignores: ["ignored/"],
+  ignores: ["ignored/**"], 
 }

Is this difference in behavior between the two ignores (global and non-global) intentional? And if so, can it be explicitly documented, if not already? It seems like a surprising "gotcha".

What actually happened?

$ ESLINT_USE_FLAT_CONFIG=true eslint -c eslint-global-ignore.config.js .

/repo/file.js
  1:1   error  'console' is not defined  no-undef

✖ 1 problems (1 errors, 0 warnings)

$ ESLINT_USE_FLAT_CONFIG=true eslint -c eslint-files-ignore.config.js .

/repo/ignored/file.js
  1:1   error  'console' is not defined  no-undef

/repo/file.js
  1:1   error  'console' is not defined  no-undef

✖ 2 problems (2 errors, 0 warnings)

Link to Minimal Reproducible Example

https://stackblitz.com/edit/eslint-ignore-pattern-inconsistency

Participation

• I am willing to submit a pull request for this issue.

Additional comments

No response

[Rec0iL99]

Hi @mxxk, thanks for the issue. ignores uses minimatch which is mentioned in the docs.

> minimatch('ignored/file.js', 'ignored/')
false

> minimatch('ignored/file.js', 'ignored/**')
true

You would need to specify ignored/** for any files in the ignored folder to match in the ignores key. This does not look like a bug.

Edit: I now see your point about the ignores globally working without ignored/**. I'm confused.

@mdjermanovic please verify. Thanks.

[mdjermanovic]

Is this difference in behavior between the two ignores (global and non-global) intentional?

It is intentional. Patterns in global ignores can match directories for performance reasons so that ESLint doesn't traverse ignored directories. Patterns in non-global ignores are supposed to match in the same way as patterns in files, that is to match only files.

In my opinion, this is indeed confusing. I think we should update the non-global ignores matching to work the same as global ignores matching. @nzakas what do you think?

[nzakas]

I agree it's confusing, although I'm not sure of the complexity of the fix. If it's an easy fix then let's fix it.

[nzakas]

@Rec0iL99 @mdjermanovic please be sure to take a stab at priority and impact when triaging. :)

[fasttime]

I'm also in favor of changing the behavior of non-global ignores patterns so that they match directories as well. Marking this issue as accepted. @mxxk would you like to submit a pull request?

[mxxk]

@fasttime I would be happy to take a stab at it. Since I'm not familiar with the code, would you mind including some pointers to the following places in the flat config part of the codebase?

1. Where entire directory trees are potentially ignored by global ignores during traversal
2. Where individual files are potentially ignored by non-global ignores

[fasttime]

@fasttime I would be happy to take a stab at it. Since I'm not familiar with the code, would you mind including some pointers to the following places in the flat config part of the codebase?

Thanks for your interest @mxxk! I haven't looked into most of this code before, but I'll try to point out what I think are the relevant parts. I will also do my best to answer any questions that may pop up while you progress.

1. Where entire directory trees are potentially ignored by global ignores during traversal

This is where a directory matched by a global ignore pattern is filtered out:

eslint/lib/eslint/eslint-helpers.js

Line 279 in 4f5440d

return matchesPattern && !configs.isDirectoryIgnored(entry.path);

The implementation of isDirectoryIgnored is in a different repository: https://github.com/humanwhocodes/config-array/blob/v0.11.9/src/config-array.js#L865-L925

The configs object on which the method is called is the resolved FlatConfigArray object that contains information about all global ignores used.

2. Where individual files are potentially ignored by non-global ignores

This is also done in the config-array repo:

https://github.com/humanwhocodes/config-array/blob/v0.11.9/src/config-array.js#L302

The function shouldIgnorePath is very simple, in that it tries to match a given file path against a list of ignore patterns. But what we would actually like to do additionally is to match the parent directories of that file against the list of ignore patterns.
A possible impementation could be similar to what is being done in the core of isDirectoryIgnored, except that in this case the list of ignore patterns would have to come directly from a config element (config.ignores), not from the global ignores of the resolved FlatConfigArray object. Also, I don't think that we can reuse the caching logic of isDirectoryIgnored, because the results would need to be specific to the config element that contains the ignore patterns.

When you feel confident enough to start working on a change, this is what I would do next:

• Fork both the eslint and config-array repos. Make sure you can test and build both projects locally.
• Do any required changes to config-array in a local branch, including tests.
• Test your updated version of config-array in eslint. You can copy your local config-array folder to eslint/node_modules/@humanwhocodes/config-array manually or using npm link.
• Commit your changes to config-array and submit a PR.
• When the PR is merged, bump the version of config-array in eslint, add unit tests and update the docs.
• Commit your changes to eslint and submit a new PR.

[nzakas]

@mxxk @fasttime while I appreciate the enthusiasm, I think this is a pretty hairy change that I'd like to take myself. Definitely not a good first contribution to ESLint due to the complexity.

[nzakas]

I believe this commit should fix it, if anyone wants to take a quick look:
humanwhocodes/config-array@0163f31

[fasttime]

Thanks for the fix @nzakas, that was fast! If we are going to treat a non-universal ignore pattern like "ignored/" as "ignored/**", I would suggest that we apply the same logic also for negated patterns. This way we could selectively unignore subfolders of ignored folders, e.g.

ignores: ["src/", "!src/js/"] // same as ["src/**", "!src/js/**"]

The rest looks fine to me. Maybe @mdjermanovic can verify?

[mdjermanovic]

I don't think ignored/ should be the same as ignored/**.

I'd expect that ignored/ matches only directory ignored/. Files inside are ignored as a consequence of being in an ignored directory, but they're not directly matched by this pattern.

[mdjermanovic]

ignores: ["src/", "!src/js/"] // same as ["src/**", "!src/js/**"]

Negated patterns should have no effect in this case because the parent directory src/ is still ignored.

[mdjermanovic]

Also, the original example should work without the slash too.

{
  // Ignores directory tree ignored/
  ignores: ["ignored"]
}

{
  files: ["**/*.js"],
  // Should ignore directory tree ignored/
  ignores: ["ignored"]
}

[fasttime]

I don't think ignored/ should be the same as ignored/**.

I'd expect that ignored/ matches only directory ignored/. Files inside are ignored as a consequence of being in an ignored directory, but they're not directly matched by this pattern.

I see the point: that's how .gitignore works. If the goal is to make non-global ignores work the same way as global ignores, then I agree that ignored/ should not be the same as ignored/**. Also, it should be possible to ignore a directory with or without a trailing slash in the pattern, for example using ignores: ["dir/subdir"].

[nzakas]

Eh okay, I'm waving the complexity flag here. I don't like the path this is going down.

If we need to make ignored and ignored work like ignored/**, then we're going to be creating a lot more matches for every pattern. The benefit of just checking for trailing slash is that I can effectively ignore all other patterns; without being able to rely on a trailing slash that means I have no choice but to create three patterns for every pattern that doesn't end in a * (because we can't know if foo.js is a file or a directory, we need to match for all cases). This is something I already do in isDirectoryIgnored() because these are intended to be run against directories, but I do not want to do that for all file matches too -- I don't think the tradeoff is worth it.

So my suggestion is that we just update the documentation to reflect the current reality. When ignores is used with files, you need to use file patterns and not directory patterns.

[fasttime]

I've drafted PR #17239 to update the documentation as suggested in the above comment.

[mdjermanovic]

In the 2023-06-01 TSC Meeting, we've agreed to not make any code changes and instead update the documentation to explicitly mention that non-global ignores can only match files and so patterns like foo/ will not ignore anything.