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

no-unused-vars with ignoreRestSiblings:true reports false-positives #14163

Closed
bdurrer opened this issue Mar 2, 2021 · 7 comments · Fixed by #14264
Closed

no-unused-vars with ignoreRestSiblings:true reports false-positives #14163

bdurrer opened this issue Mar 2, 2021 · 7 comments · Fixed by #14264
Labels
accepted archived due to age bug repro:yes rule
Projects

Comments

@bdurrer
Copy link

bdurrer commented Mar 2, 2021

Tell us about your environment

  • ESLint Version: 7.21.0
  • Node Version: 12.16.2
  • npm Version: 6.14.4
  • Operating System: Windows

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?
babel-eslint

Please show your full configuration:

Configuration
module.exports = {
    extends: 'airbnb',
    parser: 'babel-eslint',
    plugins: [
        'jasmine',
        'webdriverio',
        'react-redux',
    ],
    env: {
        'browser': true,
        'node': true,
        'jasmine': true,
        'webdriverio/wdio': true,
    },
    rules: {
        'import/no-cycle': 'off',
        'react/jsx-props-no-spreading': 'off',
        'react/no-access-state-in-setstate': 'off',
        'arrow-parens': ['error', "as-needed", { "requireForBlockBody": true }],
        'function-paren-newline': 'off',
        'indent': ['error', 4, { 'SwitchCase': 1 }],
        'no-undef': 'error',
        'no-underscore-dangle': 'off',
        'no-multiple-empty-lines': ['warn', { "max": 2 } ],
        'no-else-return': ['error', { allowElseIf: true }],
        'no-restricted-globals': [
            "error",
            { name: 'Set', message: 'Use window.Set or add an eslint-ignore if native Set is what you want' },
            { name: 'Map', message: 'Use window.Map or add an eslint-ignore if native Map is what you want' }
        ],
        'max-len': ['error', 120],
        'implicit-arrow-linebreak': 'off',
        'jsx-a11y/click-events-have-key-events': 'off',
        'jsx-a11y/interactive-supports-focus': 'off',
        'jsx-a11y/label-has-for': 'off',
        'jsx-a11y/label-has-associated-control': 'off',
        'jsx-a11y/no-noninteractive-element-interactions': 'off',
        'jsx-a11y/no-static-element-interactions': 'off',
        'jsx-a11y/anchor-is-valid': 'off',
        'jsx-a11y/control-has-associated-label': 'off',
        'class-methods-use-this': 'off',
        'no-plusplus': ['error', { 'allowForLoopAfterthoughts': true }],
        'no-mixed-operators': ['error', { 'allowSamePrecedence': true }],
        'no-multi-spaces': ['error', { ignoreEOLComments: true }],
        'no-console': 'warn',
        'no-continue': 'off',
        'no-unused-vars': ['error', { 'ignoreRestSiblings': true }],
        'object-curly-newline': 'off',
        'operator-linebreak': ["warn", "after"],
        'prefer-destructuring': 'off',
        'prefer-promise-reject-errors': 'warn',
        'react/sort-comp': ['warn', { order: ['static-methods',  'lifecycle', 'everything-else', 'render'] }],
        'react/forbid-prop-types': 'off',
        'react/jsx-fragments': 'off',
        'react/jsx-filename-extension': 'off',
        'react/jsx-indent': ['error', 4],
        'react/jsx-indent-props': ['error', 4],
        'react/no-danger': 'off',
        'react/require-default-props': 'off',
        'react/jsx-boolean-value': 'off',
        'react/no-did-update-set-state': 'off', // recommended way since 16.3.
        'react/destructuring-assignment': 'off',
        'react/static-property-placement': 'off',
        'react-redux/prefer-separate-component-file': 'off',
        'react-redux/connect-prefer-named-arguments': 'off',
        'react/no-unused-prop-types': 'off', // superseded by react-redux/no-unused-prop-types
        'react/jsx-props-no-multi-spaces': 'off',
        // disallow certain syntax forms
        // http://eslint.org/docs/rules/no-restricted-syntax
        'no-restricted-syntax': [
            'error',
            {
                selector: 'ForInStatement',
                message: 'for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array.',
            },
            {
                selector: 'LabeledStatement',
                message: 'Labels are a form of GOTO; using them makes code confusing and hard to maintain and understand.',
            },
            {
                selector: 'WithStatement',
                message: '`with` is disallowed in strict mode because it makes code impossible to predict and optimize.',
            },
        ],
        camelcase: 'off'
    },
    globals: {
        '__BUILD_NR__': true,
        'requestIdleCallback': true,
        'any': true
    },
    settings: {
        'import/core-modules': [
            'native-api',
        ],
        'react': {
            'pragma': 'React',
            'version': '16.3'
        },
    }
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

function buildLibraryStructure(folders, entries) {
    const structureMap = {
        root: {
            id: 'root',
            title: 'Library',
            type: navItemTypes.folder,
            parent: null,
            children: [],
            weight: 1,
        },
    };

    // reused dummy variables used to spread data out of entries/folders
    /* eslint-disable no-unused-vars */
    let folder;
    let validFrom;
    let validUntil;
    let parent;
    /* eslint-enable no-unused-vars */
    let rest;

    // Add folders to map of items
    folders.forEach((folderItem) => {
        ({ folder, validFrom, validUntil, ...rest } = folderItem);

        structureMap[folderItem.id] = {
            ...rest,
            oid: folderItem.id,
            isFolder: true,
        };
    });
    return structureMap;
}

What did you expect to happen?
I believe the rule no-unused-vars should not be triggered.

The rule is configured as 'no-unused-vars': ['error', { 'ignoreRestSiblings': true }],.
The above code splits definition and spreading of the two variables folder and rest. The definition of the variables are even enclosed in a disable no-unused-vars

What actually happened? Please copy-paste the actual, raw output from ESLint.

38:16 error 'folder' is assigned a value but never used no-unused-vars
38:20 error 'validFrom' is assigned a value but never used no-unused-vars
38:31 error 'validUntil' is assigned a value but never used no-unused-vars

Steps to reproduce this issue:

  1. split definition and usage of variables
  2. use the rest operator with a throw-away variables on an other line
  3. no-unsed-vars is triggered

Are you willing to submit a pull request to fix this bug?
No, I don't think I could

@bdurrer bdurrer added bug triage labels Mar 2, 2021
@nzakas nzakas added this to Needs Triage in Triage via automation Mar 2, 2021
@nzakas nzakas moved this from Needs Triage to Ready to Implement in Triage Mar 2, 2021
@nzakas nzakas added accepted and removed triage labels Mar 2, 2021
@nzakas
Copy link
Member

nzakas commented Mar 2, 2021

Confirmed, I was able to reproduce this. The problem seems to be related to using an assignment statement vs. a variable declaration. So this works as expected, with no warnings for the first three variables:

// working correctly
var { folder, validFrom, validUntil, ...rest } = folderItem;

However, this causes a warning for the first three variables:

// working incorrectly
({ folder, validFrom, validUntil, ...rest } = folderItem);

This is definitely a bug, thanks for reporting it.

@nzakas nzakas added the rule label Mar 2, 2021
@LulaV14
Copy link

LulaV14 commented Mar 2, 2021

Is this a good ‘first-time’ issue?
I’ve never contributed to this project but I will love to do that 🙂

@mdjermanovic
Copy link
Member

mdjermanovic commented Mar 2, 2021

Hi @LulaV14! Yes, this looks like a good first issue.

In this rule, restSibling logic currently checks only defs, but it should also check references. Let us know if you need any further info, and thanks for the help!

@nzakas
Copy link
Member

nzakas commented Mar 6, 2021

Welcome @LulaV14! Let us know if we can help you along the way.

@LulaV14
Copy link

LulaV14 commented Mar 31, 2021

My apologies @nzakas I've been very busy lately and haven't put a PR for this.
I see there's an open PR created by @yeonjuan that should fix the issue.

I'll try checking other good first-time issues.

@mdjermanovic mdjermanovic moved this from Ready to Implement to Pull Request Opened in Triage Mar 31, 2021
@nzakas
Copy link
Member

nzakas commented Apr 1, 2021

No worries @LulaV14, we are all working in our spare time.

@yeonjuan
Copy link
Member

yeonjuan commented Apr 1, 2021

Hi @LulaV14. Sorry for doing it without checking your progress. I should have checked but I missed it..😭

Triage automation moved this from Pull Request Opened to Complete Apr 3, 2021
mdjermanovic pushed a commit that referenced this issue Apr 3, 2021
… (#14264)

* Fix: Check assignment reference in no-unused-vars (fixes #14163)

* rename variables

* add example
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 1, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age label Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted archived due to age bug repro:yes rule
Projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

5 participants