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] Don’t mark declare class expressions as unused #106

Closed
j-f1 opened this issue Jan 21, 2019 · 6 comments · Fixed by #110
Closed

[no-unused-vars] Don’t mark declare class expressions as unused #106

j-f1 opened this issue Jan 21, 2019 · 6 comments · Fixed by #110
Assignees
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@j-f1
Copy link
Contributor

j-f1 commented Jan 21, 2019

Repro

{
  "rules": {
    "@typescript-eslint/no-unused-vars": ["error", { "args": "none" }]
  }
}
declare class ResizeObserver {
  public constructor(cb: (entries: ReadonlyArray<IResizeObserverEntry>) => void)

  public disconnect(): void
  public observe(e: HTMLElement): void
}

Expected Result

No error

Actual Result

.../desktop/app/src/lib/globals.d.ts
  220:15  error  'ResizeObserver' is defined but never used  @typescript-eslint/no-unused-vars

Versions

package version
@typescript-eslint/eslint-plugin 1.0.0
@typescript-eslint/parser 1.0.0
ESLint 5.8.0
node 8.12.0
npm ???
@j-f1 j-f1 added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Jan 21, 2019
@armano2
Copy link
Member

armano2 commented Jan 21, 2019

we have a lot of issues with declarations, i solved some of them in #88

@armano2 armano2 changed the title [no-unused-vars] Don’t mark declare expressions as unused [no-unused-vars] Don’t mark declare class expressions as unused Jan 21, 2019
@j-f1 j-f1 self-assigned this Jan 21, 2019
@j-f1
Copy link
Contributor Author

j-f1 commented Jan 21, 2019

Trying to fix this. The code in #88 is marking Foo as used, but the rule still reports it as unused.

@armano2
Copy link
Member

armano2 commented Jan 21, 2019

i think we should move this to parser as @mysticatea proposed in some old PR in eslint/typescript-eslint-parser#558

with that we could control it better, we can store information that we in "augmentation" like we do with types and override __define to mark is as used when is set

otherwise we should stop overriding eslint rule and write a little more complex logic by ourself (they have something similar for exports)

we have also false positives from #54

@armano2
Copy link
Member

armano2 commented Jan 21, 2019

i just realized that #88 is leading to some false positives

var baz = 2
declare namespace Foo {
  var foo: bar.baz
}

baz is going to be marked as used

@j-f1
Copy link
Contributor Author

j-f1 commented Jan 21, 2019

Not sure how moving the check to the parser would help as the rule is still ignoring markVariableAsUsed.

@armano2
Copy link
Member

armano2 commented Jan 21, 2019

i was thinking a little to far ahead, and didn't explain it (i tend to do so)

  • ResizeObserver is defined in 2 scopes class and module
  • markVariableAsUsed is setting eslintUsed for variable in one scope, in this case for class scope
  • scope of variable is determined by https://github.com/eslint/eslint/blob/master/lib/linter.js#L522
    const currentScope = getScope(scopeManager, currentNode);
    and we already entered class scope because Identifier is a child of ClassDeclaration
  • eslint checks variable in scope module and its not marked as used
    and there is an error

@j-f1 j-f1 closed this as completed in #110 Jan 22, 2019
@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Feb 21, 2020
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
Development

Successfully merging a pull request may close this issue.

2 participants