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] false positive when a type is used in a generic position on a method in a class with emitDecoratorMetadata: true #2972

Closed
3 tasks done
mlc-mlapis opened this issue Jan 25, 2021 · 28 comments · Fixed by #2975
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@mlc-mlapis
Copy link

mlc-mlapis commented Jan 25, 2021

  • I have tried restarting my IDE, and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

The problem reported in #2947 has not been solved by #2943. It remains the same as before.

Repro

If AppState is imported:

import {AppState} from '@m-kbase/shared/general/states';

and later used only in a constructor, like:

constructor(
   private _store: Store<AppState>
) {}

then the warning appears in many places (in relation to the import {AppState} from '@m-kbase/shared/general/states'; line):

warning 'AppState' is defined but never used @typescript-eslint/no-unused-vars

Expected Result

The same result as with the previous version 4.13.0 or earlier, with no warnings.

Versions

package version
@typescript-eslint/eslint-plugin 4.14.1
@typescript-eslint/parser 4.14.1
TypeScript 4.1.3
ESLint 7.18.0
node 12.13.1
@mlc-mlapis mlc-mlapis added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jan 25, 2021
@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party unable to repro issues that a maintainer was not able to reproduce and removed triage Waiting for maintainers to take a look labels Jan 25, 2021
@bradzacher
Copy link
Member

Unable to repro against master.
image

Likely you have an old version installed.

Otherwise - please provide a repro repo and we can look into it.

@Brooooooklyn
Copy link

Same issue here:

"@typescript-eslint/eslint-plugin": "^4.14.1",
"@typescript-eslint/parser": "^4.14.1"

@bradzacher
Copy link
Member

@Brooooooklyn - not to be rude - but that's a pretty useless comment as it doesn't add anything to this issue.
It doesn't provide any additional information - no repro, no code sample. It contains just a copy paste from a package.json.

All it does is create noise.

If you're able to repro it yourself - follow my previous comment:

please provide a repro repo so that we can look into it.

@chunghha
Copy link

@bradzacher here is a repo to reproduce.
chunghha/nest-gql@1978ca7
"yarn lint" with the 4.13.0 throws no warnings/errors but with the 4.14.1 throws two warnings.

nest-gql/src/country/country.resolver.ts
2:10 warning 'Observable' is defined but never used @typescript-eslint/no-unused-vars
4:10 warning 'Country' is defined but never used @typescript-eslint/no-unused-vars

✖ 2 problems (0 errors, 2 warnings)

✨ Done in 4.41s.

@mlc-mlapis
Copy link
Author

mlc-mlapis commented Jan 26, 2021

@bradzacher Maybe some effects of the TS version? I apologize, but when testing version 4.14.1, I mistakenly reported that TS 4.0.5 was used, but in reality, it was 4.1.3. Version 4.1.3 is also used in the repro @chunghha mentioned above.

Likely you have an old version installed.

No, I used version 4.14.1.

@mlc-mlapis
Copy link
Author

mlc-mlapis commented Jan 26, 2021

@chunghha My case is very similar, but it's not actually in the public repo, so I think your repro is enough for the problem reproduction at this moment. If it would be necessary, I can create a simple public demo of it also.

@vladimiry
Copy link

Can confirm that the issue is still there. Use https://github.com/vladimiry/ElectronMail/tree/wip to repro if needed (wip branch, yarn lint runs linting).
"@typescript-eslint/eslint-plugin": "4.14.1",
"@typescript-eslint/parser": "4.14.1",
"typescript": "4.1.3",

@vegerot
Copy link
Contributor

vegerot commented Jan 26, 2021

I can also confirm this.

image

    "@typescript-eslint/eslint-plugin": "^4.14.1",
    "@typescript-eslint/parser": "^4.14.1",
    "@vue/eslint-config-standard": "6.0.0",
    "@vue/eslint-config-typescript": "7.0.0",
    "typescript": "4.1.3",

@bradzacher bradzacher added bug Something isn't working and removed awaiting response Issues waiting for a reply from the OP or another party unable to repro issues that a maintainer was not able to reproduce labels Jan 26, 2021
@bradzacher
Copy link
Member

bradzacher commented Jan 26, 2021

Cool! With @chunghha's repro case I'm able to reproduce this now.

The issue is only occurring for users that are using type-aware linting AND are using emitDecoratorMetadata.

This highlights the value of providing as much detail as possible in the initial post and in follow up comments.
In this instance - I was unable to repro this because what was required was both the ESLint config and TS config.


The more information you can give when you file an issue - the more likely it is that the maintainer can look into it sooner.

The OP contained minimal information - two one-line code fragments, and that was it.
Which meant that I could not reproduce it.

If they had provided their configs (ESLint and TS), then I wouldn't have had to wait for others to provide a repro.

Never be afraid to dump as much info as you have. All of it. Just dump it.
It's much better to sift through information to find what's relevant, rather than get no information and have to burn time on a back-and-forth asking for more info.

@mlc-mlapis
Copy link
Author

@bradzacher You are right on 100%. The same arguments I am using in other projects. 👍 But you know, the one thing is what you know, and the other is to keep it on 100% on each step. So, thank you for your patience to all of you.

@fbaba-nibtravel
Copy link

@bradzacher , thanks for the update. Just tested setting the emitDecoratorMetadata: false and the symptom disappears. Unfortunately I cannot use this solution as our code base does contain code that relies on reflection and without the emitDecoratorMetadata we will likely have some bugs around the app.
Is there anything I can provide to help?

Cheers and thanks for the prompt replies.

@bradzacher bradzacher changed the title A wrong behavior after upgrade to 4.14.1 > PR #2943 didn't solve it [no-unused-vars] false positive when a type is used in a generic position on a method in a class with emitDecoratorMetadata: true Jan 27, 2021
@bradzacher bradzacher pinned this issue Jan 27, 2021
bradzacher added a commit that referenced this issue Jan 27, 2021
@bradzacher
Copy link
Member

This has been fixed in #2975.
This will release to the latest tag on NPM next Monday (Feb 1st) as per our release schedule.


If you need it ASAP, it has also been released to the canary tag on NPM (4.14.2-alpha.1)
To install it:

$ npm i @typescript-eslint/eslint-plugin@canary @typescript-eslint/parser@canary

# or

$ yarn add @typescript-eslint/eslint-plugin@canary @typescript-eslint/parser@canary

@chunghha
Copy link

4.14.2-alpha.1 appears to be working. Thanks.

@mlc-mlapis
Copy link
Author

The final version of 4.14.2 works as expected.

Thanks.

@kmaraz
Copy link

kmaraz commented Feb 2, 2021

We have still the same issue in our Angular project.

constructor(
    public IsMobile: IsMobile,
    private DialogsService: DialogsService,
    private UiState: UiState,
    private WindowRef: WindowRef,
    ....
  ) {

IsMobile, DialogsService, UiState and WindowRef are reported as no-unused-vars.

@vegerot
Copy link
Contributor

vegerot commented Feb 2, 2021

{
    "typescript": "4.1.3",
    "@typescript-eslint/eslint-plugin": "4.14.1",
    "@typescript-eslint/parser": "4.14.1",
    "eslint-import-resolver-typescript": "^2.3.0",
}

I am still having this problem happen in multiple files. Here is one example:

image

@mlc-mlapis
Copy link
Author

mlc-mlapis commented Feb 2, 2021

@kmaraz @vegerot As explained above, there are several scenarios and conditions under which your problem can persist.

So it's crucial to have a simple reproduction project to demonstrate the concrete case 100%. It really helps a lot and also allows eliminating the problem as fast as possible. So invest the necessary time to create it. Thanks.

With such info, this closed issue should be re-open or a new one created.

@vegerot
Copy link
Contributor

vegerot commented Feb 2, 2021

@kmaraz thanks for taking care of this for us ❤️

@kmaraz
Copy link

kmaraz commented Feb 2, 2021

@mlc-mlapis sorry for not having a repo. Here it is: https://github.com/kmaraz/no-unused-vars-issue
I'll show you the issue in the pictures:
image
This works as the class property thisWorks has different name from the Test.
image
This does not work, as the class property Test has the same name as Test import.
Hope it helps! :)

@mlc-mlapis
Copy link
Author

@kmaraz Can you confirm that your case didn't have a problem with version 4.13.0?

@kmaraz
Copy link

kmaraz commented Feb 2, 2021

@mlc-mlapis Yes, I confirm. It worked.

@mlc-mlapis
Copy link
Author

@bradzacher It looks like that there is one special case, where @typescript-eslint/no-unused-vars is falsy positive (and is probably also related to the context, what is the variable name and what is the type/interface/namespace/... name, because in this case, they are the same, but it's allowed in TS in general).

#2972 (comment)

The showed repro has been working correctly in 4.13.0.

@bradzacher
Copy link
Member

Please open a new issue.
That issue is unrelated to this one.

@kmaraz
Copy link

kmaraz commented Feb 2, 2021

New issue: #2994

@bradzacher bradzacher unpinned this issue Feb 13, 2021
@SamiSammour

This comment has been minimized.

@bradzacher
Copy link
Member

no - you just have misconfigured.
make sure you read our FAQs and documentation before commenting or raising an issue.
https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#i-am-using-a-rule-from-eslint-core-and-it-doesnt-work-correctly-with-typescript-code

@vegerot
Copy link
Contributor

vegerot commented Feb 24, 2021

@bradzacher I was wondering if you knew if my issue I describe here (vuejs/vue-cli#3145) is from a bug in vue or a bug in consistent-type-imports. Appreciate the assistance, thanks

@bradzacher
Copy link
Member

please file a new issue, as it is unrelated to this closed issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
9 participants