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-var] false positive when using recursive interface in d.ts #2714

Closed
3 tasks done
lakerswgq opened this issue Oct 27, 2020 · 9 comments · Fixed by #2768
Closed
3 tasks done

[no-unused-var] false positive when using recursive interface in d.ts #2714

lakerswgq opened this issue Oct 27, 2020 · 9 comments · Fixed by #2768
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@lakerswgq
Copy link

  • 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": "error"
  }
}

interface.d.ts

// good
interface IPerson {
  name: string;
  age: number;
}
// got warning
interface IItem {
  title: string;
  url: string;
  children?: IItem[];
}

Expected Result

No warning on interface IItem

Actual Result
Got warning: 'IItem' is defined but never used

Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.6.0
@typescript-eslint/parser 4.6.0
TypeScript 4.0.5
ESLint 6.8.0
node 12.16.2
@lakerswgq lakerswgq added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Oct 27, 2020
@bradzacher
Copy link
Member

are you using IItem elsewhere?
If no - then this is not a false-positive.
Self referencing things are not considered a usage - or else you'll get isolated, unused islands of things.

This is the same behaviour that you get for self-referencing functions

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Oct 27, 2020
@lakerswgq
Copy link
Author

@bradzacher yes,I declare IItem and some other interface as global interface in definition file,so i can access them in other files in my project.But only the IItem interface got warnings.

@bradzacher
Copy link
Member

please provide the full code sample so I can repro it appropriately.

@lakerswgq
Copy link
Author

@bradzacher try this repo, please

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

The bug is that we're doing an explicit report for interfaces that self-reference, but that circumvents the checks we do for ambient declarations.

const isOnlySelfReferenced = variable.references.every(ref => {
if (
ref.identifier.range[0] >= node.range[0] &&
ref.identifier.range[1] <= node.range[1]
) {
return true;
}
return false;
});
if (isOnlySelfReferenced) {

@bradzacher bradzacher added the good first issue Good for newcomers label Oct 27, 2020
@mateusppereira
Copy link

@bradzacher would this solve the issue?

        const isOnlySelfReferenced = variable.references.every(ref => {
          if (
            ref.identifier.range[0] >= node.range[0] &&
-             ref.identifier.range[1] <= node.range[1]
+            ref.identifier.range[1] <= node.range[1] &&
+            !util.isDefinitionFile(filename)
          ) {
            return true;
          }
          return false;
        });

@bradzacher
Copy link
Member

bradzacher commented Oct 28, 2020

that's in the right direction! But we should be able simplify this (I believe)

When the rule runs over a declaration file, it calls markDeclarationChildAsUsed to manually mark the variables (using a standard ESLint property - eslintUsed).

function markDeclarationChildAsUsed(node: DeclarationSelectorNode): void {
const identifiers: TSESTree.Identifier[] = [];
switch (node.type) {
case AST_NODE_TYPES.TSInterfaceDeclaration:
case AST_NODE_TYPES.TSTypeAliasDeclaration:
case AST_NODE_TYPES.ClassDeclaration:
case AST_NODE_TYPES.FunctionDeclaration:
case AST_NODE_TYPES.TSDeclareFunction:
case AST_NODE_TYPES.TSEnumDeclaration:
case AST_NODE_TYPES.TSModuleDeclaration:
if (node.id?.type === AST_NODE_TYPES.Identifier) {
identifiers.push(node.id);
}
break;
case AST_NODE_TYPES.VariableDeclaration:
for (const declaration of node.declarations) {
visitPattern(declaration, pattern => {
identifiers.push(pattern);
});
}
break;
}
let scope = context.getScope();
const shouldUseUpperScope = [
AST_NODE_TYPES.TSModuleDeclaration,
AST_NODE_TYPES.TSDeclareFunction,
].includes(node.type);
if (scope.variableScope !== scope) {
scope = scope.variableScope;
} else if (shouldUseUpperScope && scope.upper) {
scope = scope.upper;
}
for (const id of identifiers) {
const superVar = scope.set.get(id.name);
if (superVar) {
superVar.eslintUsed = true;
}
}
}
function visitPattern(
node: TSESTree.Node,
cb: (node: TSESTree.Identifier) => void,
): void {
const visitor = new PatternVisitor({}, node, cb);
visitor.visit(node);
}
},
});

So instead we could express this clearer as:

 const isOnlySelfReferenced = variable.references.every(ref => { 
   if ( 
     ref.identifier.range[0] >= node.range[0] && 
     ref.identifier.range[1] <= node.range[1] 
   ) { 
     return true; 
   } 
   return false; 
 }); 
-if (isOnlySelfReferenced) {
+if (isOnlySelfReferenced && variable.eslintUsed !== true) {

@ddubrava
Copy link
Contributor

@bradzacher is it possible to create a test case for this? To test variable.eslintUsed we need two files: .d.ts and .ts.

@bradzacher bradzacher added the has pr there is a PR raised to close this label Nov 16, 2020
@bradzacher
Copy link
Member

yes - you can use the filename prop in the test case.

bradzacher added a commit that referenced this issue Nov 16, 2020
I wanted to avoid doing this, but not doing this restricts our logic too much - it causes problems when we want to consider reporting on things that the base rule wouldn't report on.

Fixes #2714
Fixes #2648
Closes #2679
bradzacher added a commit that referenced this issue Nov 22, 2020
I wanted to avoid doing this, but not doing this restricts our logic too much - it causes problems when we want to consider reporting on things that the base rule wouldn't report on.

Fixes #2714
Fixes #2648
Closes #2679
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants