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!: no-unused-vars varsIgnorePattern behavior with catch arguments #17932

Merged
merged 5 commits into from Jan 4, 2024

Conversation

Tanujkanti4441
Copy link
Contributor

@Tanujkanti4441 Tanujkanti4441 commented Dec 30, 2023

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:

Tell us about your environment (npx eslint --env-info):

  • Node version: 21.1.0
  • npm version: 10.2.0
  • Local ESLint version: latest
  • Global ESLint version: latest
  • Operating System: windows

What parser are you using (place an "X" next to just one item)?

[x] Default (Espree)
[ ] @typescript-eslint/parser
[ ] @babel/eslint-parser
[ ] vue-eslint-parser
[ ] @angular-eslint/template-parser
[ ] Other

Please show your full configuration:

Configuration
'@typescript-eslint/no-unused-vars': [
      'error',
      {
        varsIgnorePattern: '^err',
        caughtErrors: 'all'
      }
    ]

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

try {
  // ...
} catch (err) { // <== unused variable
  console.error(process.env)
}

What did you expect to happen?
eslint won't ignore the err argument.
What actually happened? Please include the actual, raw output from ESLint.
eslint ignored the err argument by following the varsIgnorePattern.

Fixes: #17540

@Tanujkanti4441 Tanujkanti4441 requested a review from a team as a code owner December 30, 2023 13:29
@eslint-github-bot eslint-github-bot bot added breaking This change is backwards-incompatible feature This change adds a new feature to ESLint labels Dec 30, 2023
Copy link

netlify bot commented Dec 30, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 741cfb6
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/65962b496825de00093001ed
😎 Deploy Preview https://deploy-preview-17932--docs-eslint.netlify.app/use/migrate-to-9.0.0
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Tanujkanti4441 Tanujkanti4441 changed the title feat!: fix varsIgnorePattern behavior fix!: varsIgnorePattern behavior with catch arguments Dec 30, 2023
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Dec 30, 2023
tests/lib/rules/no-unused-vars.js Show resolved Hide resolved
docs/src/rules/no-unused-vars.md Outdated Show resolved Hide resolved
docs/src/rules/no-unused-vars.md Outdated Show resolved Hide resolved
docs/src/rules/no-unused-vars.md Outdated Show resolved Hide resolved
docs/src/rules/no-unused-vars.md Show resolved Hide resolved
@snitin315 snitin315 added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jan 1, 2024
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @mdjermanovic to review before merging.

@nzakas
Copy link
Member

nzakas commented Jan 2, 2024

Oops, one more request: because this is a breaking change, can you please add an entry into migrate-to-9.0.0.md under the "Breaking Changes for Users" section?

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Looking good. Just cleaned up the wording on the migration guide with a few suggestions.

@@ -28,6 +28,7 @@ The lists below are ordered roughly by the number of users each change is expect
* [Case-sensitive flags in `no-invalid-regexp`](#no-invalid-regexp)
* [Stricter `/* exported */` parsing](#exported-parsing)
* [`"eslint:recommended"` and `"eslint:all"` strings no longer accepted in flat config](#string-config)
* [Behavior of `varsIgnorePattern` option of `no-unused-vars` rule with catch arguments](#vars-ignore-pattern)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* [Behavior of `varsIgnorePattern` option of `no-unused-vars` rule with catch arguments](#vars-ignore-pattern)
* [`varsIgnorePattern` option of `no-unused-vars` no longer applies to catch arguments](#vars-ignore-pattern)

@@ -236,6 +237,25 @@ export default [

**Related issue(s):** [#17488](https://github.com/eslint/eslint/issues/17488)

## <a name="vars-ignore-pattern"></a> Behavior of `varsIgnorePattern` option of `no-unused-vars` rule with catch arguments
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## <a name="vars-ignore-pattern"></a> Behavior of `varsIgnorePattern` option of `no-unused-vars` rule with catch arguments
## <a name="vars-ignore-pattern"></a> `varsIgnorePattern` option of `no-unused-vars` no longer applies to catch arguments

Comment on lines 242 to 244
In previous versions of ESLint, `varsIgnorePattern` option of `no-unused-vars`, that is used to restrict some specified variable to be reported, used to ignore all variables that machtes the specified pattern except `function` arguments because there is a seperate option to ignore them called `argsIgnorePattern` and same was expected for `caughtErrorsIgnorePattern` option but `varsIgnorePattern` was also ignoring `catch` block arguments.

This behavior has been fixed in ESLint v9.0.0, that means `varsIgnorePattern` option don't get applied to both `function` and `catch` block arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In previous versions of ESLint, `varsIgnorePattern` option of `no-unused-vars`, that is used to restrict some specified variable to be reported, used to ignore all variables that machtes the specified pattern except `function` arguments because there is a seperate option to ignore them called `argsIgnorePattern` and same was expected for `caughtErrorsIgnorePattern` option but `varsIgnorePattern` was also ignoring `catch` block arguments.
This behavior has been fixed in ESLint v9.0.0, that means `varsIgnorePattern` option don't get applied to both `function` and `catch` block arguments.
In previous versions of ESLint, the `varsIgnorePattern` option of `no-unused-vars` incorrectly ignored errors specified in a `catch` clause. In ESLint v9.0.0, `varsIgnorePattern` no longer applies to errors in `catch` clauses. For example:


```

**Related issue(s):** [#17540](https://github.com/eslint/eslint/issues/17540)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**Related issue(s):** [#17540](https://github.com/eslint/eslint/issues/17540)
**To address:** If you want to specify ignore patterns for `catch` clause variable names, use the `caughtErrorsIgnorePattern` option in addition to `varsIgnorePattern`.
**Related issue(s):** [#17540](https://github.com/eslint/eslint/issues/17540)

Copy link
Contributor

@snitin315 snitin315 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!

we could also add "none" value to vars setting, just like args and catchErrors

The original issue also mentions adding "none" for vars, should we support this?

@mdjermanovic
Copy link
Member

The original issue also mentions adding "none" for vars, should we support this?

I think that should be a separate issue.

Copy link
Member

@mdjermanovic mdjermanovic 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!

@mdjermanovic mdjermanovic changed the title fix!: varsIgnorePattern behavior with catch arguments fix!: no-unused-vars varsIgnorePattern behavior with catch arguments Jan 4, 2024
@mdjermanovic mdjermanovic merged commit 3ee0f6c into eslint:main Jan 4, 2024
17 checks passed
@Tanujkanti4441 Tanujkanti4441 deleted the ignore-pattern branch January 5, 2024 09:01
michael-siek added a commit to dequelabs/axe-core that referenced this pull request May 17, 2024
Upgrade to eslint v9 flat config. Custom eslint rules have been tested
and work in the new config file. Removed unused vars in try catch blocks
due to[ breaking change in eslint
v9](eslint/eslint#17932). Use
[globals](https://www.npmjs.com/package/globals) to include [globals
env](https://eslint.org/docs/latest/use/configure/language-options#predefined-global-variables)
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 breaking This change is backwards-incompatible bug ESLint is working incorrectly feature This change adds a new feature to ESLint
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Bug: varsIgnorePattern also applies to catch arguments
4 participants