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

Improve documentation for typescript/no-unused-vars #46

Closed
OliverJAsh opened this issue Aug 3, 2017 · 28 comments
Closed

Improve documentation for typescript/no-unused-vars #46

OliverJAsh opened this issue Aug 3, 2017 · 28 comments
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@OliverJAsh
Copy link
Contributor

There are several use cases typescript/no-unused-vars does not yet support, such as type declarations:

type Foo = string;
// unexpected error
const foo: Foo = 'foo';

Would it be possible to track the status of this somewhere in the documentation? The docs for typescript/no-unused-vars do not make any mention of this.

I would be happy to raise a PR, but I don't know the full details of what is and isn't supported.

@OliverJAsh OliverJAsh changed the title typescript/no-unused-vars only supports decorators Improve documentation for typescript/no-unused-vars Aug 3, 2017
@weirdpattern
Copy link
Contributor

@JamesHenry We can close this issue now 😃

@OliverJAsh
Copy link
Contributor Author

Actually I don't think we can? My PR added this rule to the list, but it doesn't add more detail as per this issue.

@OliverJAsh OliverJAsh reopened this Aug 11, 2017
@weirdpattern
Copy link
Contributor

weirdpattern commented Oct 3, 2017

@JamesHenry can we consider removing this rule in favor of TypeScript own flags --noUnusedParameters and --noUnusedLocals?.

I think this rule is causing a lot of confusion and giving a lot of false/positives.
And more important, if we are already getting this out of box, then I don't see the point in keeping it... thoughts?

@mightyiam
Copy link
Contributor

I think that TypeScript did not include noUnusedParameters and noUnusedLocals in strict because they see them more as style things. I think they might even regret having these options at all, as they are supposed to be linting issues and not type issues.

@lostfictions
Copy link

I appreciate having a no-unused-vars linter rule because I may not necessarily want tsc to return an error (one that might, say, fail a CI build) on something that I might consider more of a warning. (And there's unfortunately no way to configure the severity of the TypeScript flags.)

@weirdpattern
Copy link
Contributor

This is a very good point. I will continue working on this rule for as long as it's needed.

@tenshiemi
Copy link

What is the correct setting for this rule? If I'm understanding correctly, it should prevent ESLint from erroneously throwing these errors, but I can't for the life of me to get it to work and since it's not operating as a rule so much as a workaround it's not clear to me what value I should set it to.

@weirdpattern
Copy link
Contributor

weirdpattern commented Oct 27, 2017

Hi @tenshiemi, this rule extends eslint/no-unused-vars to include TypeScript syntax.

You will need both defined in your eslint config to make it work. What I usually do is extend the eslint:recommended settings, then manually enable typescript/no-unused-vars

Hope this helps.

@tenshiemi
Copy link

tenshiemi commented Oct 27, 2017

@weirdpattern Thanks, this is what I have both it throws a bunch of no-unused-vars and no-undef warnings:

{
    "parser": "typescript-eslint-parser",
    "plugins": [
        "react",
        "standard",
        "typescript",
        "prettier"
    ],
    "extends": [
        "eslint:recommended",
        "plugin:react/recommended",
        "prettier",
        "prettier/react",
        "prettier/standard"
    ],
    "parserOptions": {
        "sourceType": "module",
        "ecmaFeatures": {
            "jsx": true
        }
    },
    "rules": {
        "no-undef": "warn",
        "no-unused-vars": "warn",
        "typescript/no-use-before-define": "warn",
        "typescript/no-unused-vars": "warn",
        "prettier/prettier": ["error", {
            "parser": "typescript",
            "semi": true,
            "singleQuote": true,
            "tabWidth": 4,
            "trailingComma": "all"
        }]
    }

@weirdpattern
Copy link
Contributor

@tenshiemi, we are still working on this rule, we keep finding scenarios that we missed and have to be included.
Could you please provide a snippet of the code that is failing?

@tenshiemi
Copy link

@weirdpattern I just realized it works when I run arc lint, just not when I run eslint directly on a file, so that's a relief!

an example is
10:18 warning 'SearchTableState' is not defined no-undef
when there is export interface SearchTableState { data: object[]; searchText: string; } in the code

@aaronjensen
Copy link

Here's a scenario where it fails:

import { FieldRenderProps } from 'react-final-form'

const Error = ({ meta }: { meta: FieldRenderProps['meta'] }) => null

@milesj
Copy link
Contributor

milesj commented Jun 22, 2018

Instead of creating a new issue, I'll just report some more findings.

  1. "'Foo' is defined but never used". Doesn't catch extends in generics.
import { Foo } from './types';

class Bar<T extends Foo> {}
  1. "'this' is defined but never used". This error comes from the base no-unused-vars rule. This disappears if I set the rule option args to "none".
import webpack from 'webpack';

export default function webpackLoader(this: webpack.loader.LoaderContext) {}
  1. "'ExecaOptions' is defined but never used". Doesn't catch aliased imports.
import execa, { Options as ExecaOptions } from 'execa';

function foo(options: ExecaOptions) {}
  1. "'Bar' is defined but never used". Doesn't catch intersection generics. (Happens a lot with React).
import { Foo, Bar } from './types';

class Baz<Foo & Bar> {}

@macklinu
Copy link
Contributor

As of the latest commit (5e410a36c43d82ab679bed8a8ef68a655008fde9), I'm seeing this snippet pass as a local no-unused-vars test case. Can add it to the codebase in a PR if that would be helpful.

import { FieldRenderProps } from 'react-final-form'

const Error = ({ meta }: { meta: FieldRenderProps['meta'] }) => null

I've confirmed the following as a failing test and will open a PR to address what I believe is the issue:

import { Foo } from './types';

class Bar<T extends Foo> {}

I'm not able to reproduce "'ExecaOptions' is defined by never used" in the following snippet. I see the following errors locally. @milesj do you have a reproducible test case of this issue, perhaps by adding a failing test in a PR? That would be awesome 😄

import execa, { Options as ExecaOptions } from 'execa';

function foo(options: ExecaOptions) {}
AssertionError [ERR_ASSERTION]: Should have no errors but had 3: 
[ { ruleId: 'no-unused-vars',
    severity: 1,
    message: '\'execa\' is defined but never used.',
    line: 2,
    column: 8,
    nodeType: 'Identifier',
    source: 'import execa, { Options as ExecaOptions } from \'execa\';',
    endLine: 2,
    endColumn: 13 },
  { ruleId: 'no-unused-vars',
    severity: 1,
    message: '\'foo\' is defined but never used.',
    line: 4,
    column: 10,
    nodeType: 'Identifier',
    source: 'function foo(options: ExecaOptions) {}',
    endLine: 4,
    endColumn: 13 },
  { ruleId: 'no-unused-vars',
    severity: 1,
    message: '\'options\' is defined but never used.',
    line: 4,
    column: 14,
    nodeType: 'Identifier',
    source: 'function foo(options: ExecaOptions) {}',
    endLine: 4,
    endColumn: 35 } ]

macklinu referenced this issue in macklinu/eslint-plugin-typescript Jun 22, 2018
@milesj
Copy link
Contributor

milesj commented Jun 22, 2018

@macklinu Here's the execa full code: https://github.com/milesj/boost/blob/master/src/Routine.ts#L111 It's probably the wrapping parens + union causing it.

@milesj
Copy link
Contributor

milesj commented Jun 25, 2018

Think I found a few more.

import { Foo, Bar } from './types';

// Doesnt catch either mapped types 
const NAMES: { [name in Foo]: Bar } = {};

@macklinu
Copy link
Contributor

macklinu commented Jul 2, 2018

@milesj
Copy link
Contributor

milesj commented Aug 10, 2018

Any update on this? If you're open to assigning collaborators, I could help go through some of these issues/PRs.

@Radivarig
Copy link

// warning 'B' is defined but never used
const a: Array<{b: B}> = [] 

armano2 referenced this issue in armano2/eslint-plugin-typescript Nov 29, 2018
bradzacher referenced this issue in bradzacher/eslint-plugin-typescript Dec 4, 2018
@JamesHenry JamesHenry transferred this issue from bradzacher/eslint-plugin-typescript Jan 18, 2019
@bradzacher bradzacher added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin documentation Documentation ("docs") that needs adding/updating labels Jan 18, 2019
@ThomasdenH
Copy link
Contributor

ThomasdenH commented Feb 27, 2019

Another false positive:

declare type Checker = (v) => v is number;
const checker: Checker = (w): w is number => true;

Here w is used in the type guard, but according to the lint it's unused.

@ThomasdenH
Copy link
Contributor

Also, since this lint seems to be mostly covered by the Typescript option, maybe it's a good idea to remove it from /recommended?

@bradzacher
Copy link
Member

@ThomasdenH - please raise that error as a separate issue.
This issue is about documentation.

There is discussion about no-unused-vars in the recommended set in #122.

Considering we're working on improving this constantly, I'm going to close this issue.
The team is fixing edge cases and working for a better solution to this.

@kevinmarrec
Copy link

kevinmarrec commented Mar 6, 2019

@bradzacher Does your last comment mean that the issue regarding no-unused-vars will be fixed in next release(s) ?

Cause it causes frustation to have to disable the no-unused-vars rule for the whole project (even JS files) to be able to lint without errors typescript files that are simply using type or interface definitions.

My, minimal reproduction is

import { IOptions } from './types'
const options: IOptions = {}
... // Use `options` later

resulting in

'IOptions' is defined but never used

Where should we follow the status on this ? We should have a unclosed issue pending until it's not fixed. After more than 1 hour figuring what's going on with the severals issues refering to no-unused-vars, this issue seemed the one where people exposed the more use cases which cause the lint issue.

@bradzacher
Copy link
Member

bradzacher commented Mar 6, 2019

@kevinmarrec - which issue? There are a number of edge cases that aren't properly covered by the rule. Some we know about, many we don't.

This issue is specifically about attempting to document the cases that are currently unsupported, but it has accidentally gone off the rails and become a place people are putting bug reports.

A growing thread which has a mixture between discussions, bug reports and examples is not a good way to document issues with the rule. One issue opened per bug, on the other hand, clearly documents what problems are known and open with the rule.

It's very hard to track which bugs are fixed if they are all logged as comments in a single thread: a thread isn't easily searchable, we can't close comments once their issue is fixed, and we can't clearly link to comments in PRs.

This the primary reason I've closed this issue.

Please open a new issue if you find a case that's not properly covered by the rule so that we can properly track the bug.
Please use the search first to double check that your bug isn't already logged.

@kevinmarrec
Copy link

kevinmarrec commented Mar 7, 2019

Alright @bradzacher, I understand.

The issue is that typescript-eslint/no-unused-vars simply tag types and interfaces (that are imported or defined in the file) as unused, even if they are.

Well for my specific case I ended up with this workaround

overrides: [
    {
      files: ['*.ts', '*.tsx'],
      rules: {
        '@typescript-eslint/no-unused-vars': [2, { args: 'none' }]
      }
    }
  ]

which seems to fit my use case, but I think it should work without having to do this workaround.

If I find some time, I'll open an issue with minimal reproduction through a repository.

@IgorMing
Copy link

Thank you @kevinmarrec! It's working fine, now.

@leognmotta
Copy link

@kevinmarrec Thank you!

DesignChips pushed a commit to DesignChips/atomic-design-with-redux that referenced this issue Jun 1, 2019
progfay pushed a commit to progfay/scrapbox-parser that referenced this issue Jun 21, 2019
- occur `no-unused-vars` error and change eslint rule
  - typescript-eslint/typescript-eslint#46 (comment)
@tvvignesh
Copy link

@bradzacher : I had the same problem. The solution which @kevinmarrec provided worked for me as well. using 2.6.1 version of the plugin.

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Nov 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Documentation ("docs") that needs adding/updating package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests