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

feat: add destructuredArrayIgnorePattern option in no-unused-vars #15649

Merged
merged 15 commits into from Mar 11, 2022

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Feb 26, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] 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)

Fix #15611

Added a new option destructuredArrayIgnorePattern to the no-unused-vars rule.

// With the following option set within eslintrc rules
// "no-unused-vars": ["error", { "destructuredArrayIgnorePattern": "^_" }],

const array = ['a', 'b', 'c'];

const [a, _b, c] = array;  // No "no-unused-vars" error on _b

const newArray = [a, c];

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

No

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Feb 26, 2022
@eslint-github-bot
Copy link

Hi @snitin315!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@snitin315 snitin315 changed the title feat: add destructuredArrayIgnorePattern option in no-unused-vars rule feat: add destructuredArrayIgnorePattern option in no-unused-vars Feb 26, 2022
@snitin315 snitin315 marked this pull request as ready for review February 26, 2022 11:54
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Mar 1, 2022
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

Should the option also allow this:

/*eslint no-unused-vars: ["error", { "destructuredArrayIgnorePattern": "^_" }]*/

let _a, b;

foo.forEach(item => {
    [_a, b] = item;
    doSomething(b);
});

That might be consistent with the change we recently accepted (#14163) for option ignoreRestSiblings:

/*eslint no-unused-vars: ["error", { "ignoreRestSiblings": true }]*/

let a, b; // 'a' is allowed because it is used as a rest sibling

foo.forEach(item => {
    ({ a, ...b } = item);
    doSomething(b);
});

docs/rules/no-unused-vars.md Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@snitin315
Copy link
Contributor Author

Should the option also allow this:

/*eslint no-unused-vars: ["error", { "destructuredArrayIgnorePattern": "^_" }]*/

let _a, b;

foo.forEach(item => {
    [_a, b] = item;
    doSomething(b);
});

I agree, it makes sense to allow this.

@snitin315
Copy link
Contributor Author

That might be consistent with the change we recently accepted (#14163) for option ignoreRestSiblings:

@mdjermanovic It seems ignoreRestSiblingsworks only with object and not with arrays, should we track it in a separate issue?

/*eslint no-unused-vars: ["error", { "ignoreRestSiblings": true }]*/

let a, b; // reports a

foo.forEach(item => {
    ([ a, ...b ] = item);
    console.log(b);
});

const [c, ...d] = []; // reports c
console.log(d)

const {e, ...f} = {}; // ok, doesn't report e
console.log(f);

let x, y; // ok, doesn't report x

foo.forEach(item => {
    ({x, ...y} = item);
    console.log(y);
});

Online demo

@mdjermanovic
Copy link
Member

@mdjermanovic It seems ignoreRestSiblingsworks only with object and not with arrays, should we track it in a separate issue?

I think it works as intended because documentation for ignoreRestSiblings and the original proposal (#7968) explicitly refer to objects and rest properties. Rest elements would need a separate option.

lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
lib/rules/no-unused-vars.js Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Comment on lines +250 to +260
const [a, _b, c] = ["a", "b", "c"];
console.log(a+c);

const { x: [_a, foo] } = bar;
console.log(foo);

function baz([_c, x]) {
x;
}
baz();

Copy link
Sponsor Contributor

@ljharb ljharb Mar 10, 2022

Choose a reason for hiding this comment

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

i'm confused. in an array pattern, you never need an unused variable, you just omit the binding - ie, [, x] etc. Why would you ever want an exception here?

It's only in object destructuring that omitting the binding has a use case.

Copy link
Member

Choose a reason for hiding this comment

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

The rationale for this option is in Additional Comments of the original post in #15611.

This is optional behavior, unused variables in array patterns are not ignored by default. Users who prefer elisions can simply not enable this option.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

So, this is a style option, added after the prohibition on such things?

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 merged commit 800bd25 into main Mar 11, 2022
@mdjermanovic mdjermanovic deleted the feat/add-destructuredArrayIgnorePattern branch March 11, 2022 12:59
@pstachula-dev
Copy link

I have error with this rule on Eslint 8.11.0

My config:

"@typescript-eslint/no-unused-vars": [
      "error",
      {
        destructuredArrayIgnorePattern: "^_",
      },
    ],

Error:

Oops! Something went wrong! :(

ESLint: 8.11.0

Error: .eslintrc.js:
	Configuration for rule "@typescript-eslint/no-unused-vars" is invalid:
	Value {"destructuredArrayIgnorePattern":"^_"} should be equal to one of the allowed values.
	Value {"destructuredArrayIgnorePattern":"^_"} should NOT have additional properties.
	Value {"destructuredArrayIgnorePattern":"^_"} should match exactly one schema in oneOf.

@snitin315
Copy link
Contributor Author

Can you once try the following config? Maybe typescript-eslint has not updated it yet.

"no-unused-vars": [
      "error",
      {
        destructuredArrayIgnorePattern: "^_",
      },
    ],

@pstachula-dev
Copy link

pstachula-dev commented Mar 15, 2022

I have installed: typescript-eslint 5.12.1

With this configuration, I have an error

Variable name `_open` must match one of the following formats: camelCase, PascalCase, UPPER_CASEeslint[@typescript-eslint/naming-convention](https://typescript-eslint.io/rules/naming-convention)
'_open' is assigned a value but never used.eslint[@typescript-eslint/no-unused-vars](https://typescript-eslint.io/rules/no-unused-vars)

But there is one strange thing.
With this:

"@typescript-eslint/no-unused-vars": [
      "error",
      {
        destructuredArrayIgnorePattern: "^_",
      },
    ],

In VSC and Eslint plugin - everything is correct, but I have problem only in CLI with eslint

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 18, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.10.0...v8.11.0)

#### Features

-   [`800bd25`](eslint/eslint@800bd25) feat: add `destructuredArrayIgnorePattern` option in `no-unused-vars` ([#&#8203;15649](eslint/eslint#15649)) (Nitin Kumar)
-   [`8933fe7`](eslint/eslint@8933fe7) feat: Catch `undefined` and `Boolean()` in no-constant-condition ([#&#8203;15613](eslint/eslint#15613)) (Jordan Eldredge)
-   [`f90fd9d`](eslint/eslint@f90fd9d) feat: Add ESLint favicon to the HTML report document ([#&#8203;15671](eslint/eslint#15671)) (Mahdi Hosseinzadeh)
-   [`57b8a57`](eslint/eslint@57b8a57) feat: `valid-typeof` always ban `undefined` ([#&#8203;15635](eslint/eslint#15635)) (Zzzen)

#### Bug Fixes

-   [`6814922`](eslint/eslint@6814922) fix: escaping for square brackets in ignore patterns ([#&#8203;15666](eslint/eslint#15666)) (Milos Djermanovic)
-   [`c178ce7`](eslint/eslint@c178ce7) fix: extend the autofix range in comma-dangle to ensure the last element ([#&#8203;15669](eslint/eslint#15669)) (Milos Djermanovic)

#### Documentation

-   [`c481cec`](eslint/eslint@c481cec) docs: add fast-eslint-8 to atom integrations (userguide) ([#&#8203;15695](eslint/eslint#15695)) (db developer)
-   [`d2255db`](eslint/eslint@d2255db) docs: Add clarification about `eslint-enable` ([#&#8203;15680](eslint/eslint#15680)) (dosisod)
-   [`8b9433c`](eslint/eslint@8b9433c) docs: add object pattern to first section of computed-property-spacing ([#&#8203;15679](eslint/eslint#15679)) (Milos Djermanovic)
-   [`de800c3`](eslint/eslint@de800c3) docs: link to minimatch docs added.  ([#&#8203;15688](eslint/eslint#15688)) (Gaurav Tewari)
-   [`8f675b1`](eslint/eslint@8f675b1) docs: sort-imports add single named import example ([#&#8203;15675](eslint/eslint#15675)) (Arye Eidelman)

#### Chores

-   [`385c9ad`](eslint/eslint@385c9ad) chore: rm trailing space in docs ([#&#8203;15689](eslint/eslint#15689)) (唯然)

</details>

---

### Configuration

📅 **Schedule**: 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).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1217
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>
@esetnik
Copy link

esetnik commented Apr 20, 2022

I have error with this rule on Eslint 8.11.0

My config:

"@typescript-eslint/no-unused-vars": [
      "error",
      {
        destructuredArrayIgnorePattern: "^_",
      },
    ],

Error:

Oops! Something went wrong! :(

ESLint: 8.11.0

Error: .eslintrc.js:
	Configuration for rule "@typescript-eslint/no-unused-vars" is invalid:
	Value {"destructuredArrayIgnorePattern":"^_"} should be equal to one of the allowed values.
	Value {"destructuredArrayIgnorePattern":"^_"} should NOT have additional properties.
	Value {"destructuredArrayIgnorePattern":"^_"} should match exactly one schema in oneOf.

I have the same issue as @pstachula-dev after this PR change

srijan-deepsource pushed a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
…eslint#15649)

* feat: add `destructuredArrayIgnorePattern` option in `no-unused-vars`

Fixes eslint#15611

* docs: add `destructuredArrayIgnorePattern` option in `no-unused-vars`

* fix: remove false positives and false negatives

* docs: update

* test: add more cases

* docs: update

* fix: remove false positives

* docs: update

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

* feat: improve error message

* feat: cover more cases

* docs: add more example

* refactor: code

* fix: check for all references

* docs: add more examples

* chore: apply suggestions from code review

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

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
srijan-deepsource added a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 8, 2022
@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 Sep 8, 2022
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 enhancement This change enhances an existing feature of ESLint feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule Change: (Add destructuredArrayIgnorePattern option to no-unused-vars)
5 participants