Skip to content

[no-unused-vars] false positives for code inside declare blocks #2844

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.

Sign up for GitHub

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

Jump to bottom
Closed
3 tasks done
kripod opened this issue Dec 3, 2020 · 6 comments · Fixed by #2848
Closed
3 tasks done

[no-unused-vars] false positives for code inside declare blocks #2844

kripod opened this issue Dec 3, 2020 · 6 comments · Fixed by #2848
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@kripod
Copy link
Contributor

kripod commented Dec 3, 2020

  • 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.

Repro

{
  "rules": {
    "@typescript-eslint/no-unused-vars": "warn"
  }
}
import "next-auth";

declare module "next-auth" {
  interface User {
    id: string;
    givenName: string;
    familyName: string;
  }
}
import { PrismaClient } from "@prisma/client";

// Namespace declarations are also problematic
declare namespace globalThis {
  let prisma: PrismaClient; // Value can be anything else
}

Expected Result

No warnings should be emitted.

Actual Result

Invalid warnings are emitted inside declare blocks, unlike in v4.8.

Versions

package version
@typescript-eslint/eslint-plugin 4.9.0
@typescript-eslint/parser 4.9.0
TypeScript 4.1.2
ESLint 7.14.0
node 14.15.0
@kripod kripod added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Dec 3, 2020
@vidaritos
Copy link

Already fixed (I think?), pending release: #2833

@kripod
Copy link
Contributor Author

kripod commented Dec 3, 2020 •
edited
Loading

@vidaritos I saw that PR but don’t think it has anything to do with declare blocks, unfortunately. Thank you for pointing out that fixes are already being made.

@bradzacher
Copy link
Member

I am unable to reproduce this against master

image

Could you please create a repro repo to help me track down the issue?

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look labels Dec 3, 2020
@whitetrefoil
Copy link

I've met this issue in my proj:
https://github.com/whitetrefoil/whitetrefoil-checkin-www/tree/temp/typescript-eslint-2844
It's not a minimal repro, but only 2 files are related:

/src/store/types.ts
Here declares RootState.

/src/hooks/use-root-selector.ts
And it's used here.

@bradzacher bradzacher added bug Something isn't working and removed awaiting response Issues waiting for a reply from the OP or another party labels Dec 4, 2020
@bradzacher
Copy link
Member

Thanks for that @whitetrefoil

I've figured it out.
This is a race condition between naming-convention and no-unused-vars.
If no-unused-vars runs first - it works as expected.
But if naming-convention runs first - it breaks.

Why?

Because naming-convention uses the same utility as no-unused-vars does to detect unused variables, and this utility caches the results for each AST (to save duplicate work!)

typescript-eslint/packages/eslint-plugin/src/util/collectUnusedVariables.ts

Lines 39 to 43 in aadb39f

const program = context.getSourceCode().ast;
const cached = this.RESULTS_CACHE.get(program);
if (cached) {
return cached;
}

However - no-unused-vars has a few additional selectors it runs in order for it to not report on ambient declarations. naming-convention doesn't run these because it doesn't care.

typescript-eslint/packages/eslint-plugin/src/rules/no-unused-vars.ts

Lines 250 to 288 in a8227a6

// declaration file handling
[ambientDeclarationSelector(AST_NODE_TYPES.Program, true)](
node: DeclarationSelectorNode,
): void {
if (!util.isDefinitionFile(filename)) {
return;
}
markDeclarationChildAsUsed(node);
},
// children of a namespace that is a child of a declared namespace are auto-exported
[ambientDeclarationSelector(
'TSModuleDeclaration[declare = true] > TSModuleBlock TSModuleDeclaration > TSModuleBlock',
false,
)](node: DeclarationSelectorNode): void {
markDeclarationChildAsUsed(node);
},
// declared namespace handling
[ambientDeclarationSelector(
'TSModuleDeclaration[declare = true] > TSModuleBlock',
false,
)](node: DeclarationSelectorNode): void {
const moduleDecl = util.nullThrows(
node.parent?.parent,
util.NullThrowsReasons.MissingParent,
) as TSESTree.TSModuleDeclaration;
// declared ambient modules with an `export =` statement will only export that one thing
// all other statements are not automatically exported in this case
if (
moduleDecl.id.type === AST_NODE_TYPES.Literal &&
checkModuleDeclForExportEquals(moduleDecl)
) {
return;
}
markDeclarationChildAsUsed(node);
},

So if naming-convention runs first, it'll generate list X, which includes any ambient variables, then no-unused-vars will run, and it'll mark the ambient variables as used, but the utility will still generate list X because of caching.

This is an easy fix.

bradzacher added a commit that referenced this issue Dec 4, 2020
@bradzacher
fix(eslint-plugin): [no-unused-vars] fix race condition between namin…
a37c058 
…g-convention and no-unused-vars

Fixes #2844
@bradzacher bradzacher mentioned this issue Dec 4, 2020
Merged
bradzacher added a commit that referenced this issue Dec 4, 2020
@bradzacher
fix(eslint-plugin): [no-unused-vars] fix race condition between namin…
ccb6b94 
…g-convention and no-unused-vars (#2848)

Fixes #2844
@kripod
Copy link
Contributor Author

kripod commented Dec 4, 2020

Hooray, thank you for figuring this out and fixing it so fast! 🙌

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
4 participants
@whitetrefoil @bradzacher @kripod @vidaritos