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

Bug: [new config system] ignores does not work without files #17103

Closed
1 task done
filetvignon opened this issue Apr 21, 2023 · 8 comments · Fixed by #17181
Closed
1 task done

Bug: [new config system] ignores does not work without files #17103

filetvignon opened this issue Apr 21, 2023 · 8 comments · Fixed by #17181
Assignees
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 repro:yes

Comments

@filetvignon
Copy link

Environment

Node version: 18.16.0
npm version: 9.6.4
Local ESLint version: 8.38.0
Global ESLint version: N/A
Operating System: Ubuntu 22.04 (WSL)

What parser are you using?

Default (Espree)

What did you do?

Configuration
[
  {
    rules: {
      "no-unused-vars": "error",
    },
  },
  {
    ignores: ["error.js"],
    rules: {
      "no-unused-vars": "warn",
    },
  },
];

./error.js and ./warn.js:

let unusedVar;

Then run eslint .

What did you expect to happen?

{cwd}/error.js
  1:5  error  'unusedVar' is defined but never used  no-unused-vars

{cwd}/warn.js
  1:5  warning  'unusedVar' is defined but never used  no-unused-vars

What actually happened?

{cwd}/error.js
  1:5  warning  'unusedVar' is defined but never used  no-unused-vars

{cwd}/warn.js
  1:5  warning  'unusedVar' is defined but never used  no-unused-vars

Link to Minimal Reproducible Example

https://github.com/filetvignon/eslint-bug-ignores

Participation

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

Additional comments

I set 2 configuration objects: one with a rule all files and another to change that rule for all files EXCEPT error.js.
I omitted the files property in these objects; although the documentation recommends using ignores with files, it states that this should work and omitting files should be the same as using files: '**/*'.

In reality, it works only if I explicitly set files in the second configuration object:

[
  {
    rules: {
      "no-unused-vars": "error",
    },
  },
  {
    files: ["**/*"],
    ignores: ["error.js"],
    rules: {
      "no-unused-vars": "warn",
    },
  },
];
@filetvignon filetvignon added bug ESLint is working incorrectly repro:needed labels Apr 21, 2023
@fasttime
Copy link
Member

I am able to reproduce this. This is happening because configs without files are considered universal configs, and they are applied to all files regardless of ignores. I also find this behavior surprising, but I believe that it's there intentionally to allow excluding things like node_modules from linting. In this case we should update the documentation to mention that ignores works differently in a config without files.

Maybe @nzakas can advise the best course of action here.

@fasttime fasttime added core Relates to ESLint's core APIs and features repro:yes and removed repro:needed labels Apr 23, 2023
@mdjermanovic
Copy link
Member

This is a bug, the configuration object should not apply to file paths that match its ignores.

In this case we should update the documentation to mention that ignores works differently in a config without files.

There are two cases:

  1. Configuration object has ignores and doesn't have any other keys. In this case, the object specifies which files should be ignored. Mentioned here: https://eslint.org/docs/latest/use/configure/configuration-files-new#globally-ignoring-files-with-ignores.
  2. Configuration object has ignores and one or more other keys. This is a "normal" entry that specifies configuration for matching files. If it doesn't have files, it should work the same as if it had files: ["**/*"]. This is mentioned at the end of https://eslint.org/docs/latest/use/configure/configuration-files-new#excluding-files-with-ignores:

If `ignores` is used without `files` and any other setting, then the configuration object applies to all files except the ones specified in `ignores`, for example:
```js
export default [
{
ignores: ["**/*.config.js"],
rules: {
semi: "error"
}
}
];
```
This configuration object applies to all files except those ending with `.config.js`. Effectively, this is like having `files` set to `**/*`. In general, it's a good idea to always include `files` if you are specifying `ignores`.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 24, 2023
@fasttime
Copy link
Member

Thanks for the clarification @mdjermanovic. Sometimes it's hard to know if it's the implementation that doesn't match the docs or vice-versa. That said, I find the headline of the example mentioned above confusing, as it should actually read (bold is my correction):

If ignores is used without files but with some other settings, then the configuration object applies to all files except the ones specified in ignores, for example:

As for the bug, it needs to be fixed in @humanwhocodes/config-array, although it would be also a good idea to add some unit tests here in eslint.

@filetvignon would you like to submit a pull request?

@mdjermanovic
Copy link
Member

That said, I find the headline of the example mentioned above confusing, as it should actually read (bold is my correction):

If ignores is used without files but with some other settings, then the configuration object applies to all files except the ones specified in ignores, for example

Yes, I think we should fix that in the docs.

@nzakas
Copy link
Member

nzakas commented May 8, 2023

Just catching up on this now. This does look like a bug, and the docs could be clearer, I'd suggest something more along the lines of this:

If ignores is used without files and there are other keys (such as rules), then the configuration object applies to all files except the ones specified in ignores, for example

@nzakas nzakas self-assigned this May 8, 2023
nzakas added a commit to humanwhocodes/config-array that referenced this issue May 9, 2023
When a config has `ignores` and not `files`, plus at least one other
key, it should match all files that don't match `ignores` and the other
keys should be merged into the resulting config.

Prior to this, such a config was always merged in regardless if the
filename matched `ignores`; after this change, it correctly merges in
only when `ignores` matches.

Refs eslint/eslint#17103
nzakas added a commit to humanwhocodes/config-array that referenced this issue May 12, 2023
* fix: Config with just ignores should check ignores

When a config has `ignores` and not `files`, plus at least one other
key, it should match all files that don't match `ignores` and the other
keys should be merged into the resulting config.

Prior to this, such a config was always merged in regardless if the
filename matched `ignores`; after this change, it correctly merges in
only when `ignores` matches.

Refs eslint/eslint#17103

* Update ignores logic

* Update tests/config-array.test.js

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

---------

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@nzakas
Copy link
Member

nzakas commented May 12, 2023

I pushed a new version of @humanwhocodes/config-array, but I won't have time to integrate into ESLint before traveling for next week.

@mdjermanovic
Copy link
Member

I pushed a new version of @humanwhocodes/config-array, but I won't have time to integrate into ESLint before traveling for next week.

I can take this, but it seems that the new version 0.11.9 hasn't been released. The release workflow failed on npm ci:

https://github.com/humanwhocodes/config-array/actions/runs/4961978783/jobs/8879447925

@nzakas nzakas added tsc agenda This issue will be discussed by ESLint's TSC at the next meeting and removed tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jun 1, 2023
@nzakas
Copy link
Member

nzakas commented Jun 1, 2023

Sorry about that, should be published now.

mdjermanovic added a commit that referenced this issue Jun 2, 2023
#17181)

* fix: Config with `ignores` and without `files` should not always apply

Fixes #17103

* use @humanwhocodes/config-array@0.11.9

* use @humanwhocodes/config-array@0.11.10
mattstern31 added a commit to mattstern31/config-array that referenced this issue Nov 11, 2023
* fix: Config with just ignores should check ignores

When a config has `ignores` and not `files`, plus at least one other
key, it should match all files that don't match `ignores` and the other
keys should be merged into the resulting config.

Prior to this, such a config was always merged in regardless if the
filename matched `ignores`; after this change, it correctly merges in
only when `ignores` matches.

Refs eslint/eslint#17103

* Update ignores logic

* Update tests/config-array.test.js

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

---------

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 30, 2023
@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 Nov 30, 2023
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 repro:yes
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants