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

[@typescript-eslint/no-shadow] false positives with static methods #2592

Closed
dinofx opened this issue Sep 22, 2020 · 7 comments · Fixed by #3393
Closed

[@typescript-eslint/no-shadow] false positives with static methods #2592

dinofx opened this issue Sep 22, 2020 · 7 comments · Fixed by #3393
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@dinofx
Copy link
Contributor

dinofx commented Sep 22, 2020

False positive reported for static methods with a type parameter with the same name as the containing class' type parameter.

export class Wrapper<Wrapped> {
  private constructor(private readonly wrapped: Wrapped) {}

  unwrap(): Wrapped {
    return this.wrapped;
  }

  static create<Wrapped>(wrapped: Wrapped) {
    return new Wrapper<Wrapped>(wrapped);
  }
}

In a static method, the type parameters for the class are not in scope because there's no instance.

@dinofx dinofx added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Sep 22, 2020
@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for maintainers to take a look labels Sep 22, 2020
@bradzacher
Copy link
Member

Yeah this is a difficult one.
The scope analysis tooling assumes that each scope follows a tree-like structure. In this instance here the static scope is a child of the class scope.

To make this work "properly", we'd have to restructure the class analysis to handle this. It'd be super weird to setup.

It's probably better to just handle this in the rule - check the shadow and ignore it if the method is static.

@dinofx
Copy link
Contributor Author

dinofx commented Sep 23, 2020

It looks like the problem is nestFunctionScope is always setting the upper scope to this.currentScope. If you're in a static method, the upper scope should be the class' parent scope, not the class, I think?

Conditional types had the same problem. You had to visit some of the children under one scope, and the "else" child under the original scope. Maybe it would be easier to have the upper score be a parameter instead of something that is pushed and popped.

@bradzacher
Copy link
Member

The class creates its own scope to contain both the generic type parameter decls, and for the class name (also the class heritage is referenced from within this scope).

If you reference the class name from inside of the class, it references a variable defined inside of the class scope, not the variable defined in the parent scope.

If we were to use the parent scope of the class scope, that would break that relationship, as the static method would reference the class variable defined in the parent scope.

The true fix for this would be to nest an "instance" scope within the class scope, and then define the class generics and instance members within that child scope, and then define static members in the class scope itself.

However this would be a large deviation from the core eslint-scope algorithm, and it's hard to say what the ramifications would be. Rules make assumptions based on what eslint-scope does, so we unfortunately have to adhere to it.

@Ionaru

This comment has been minimized.

@bradzacher

This comment has been minimized.

@OliverJAsh

This comment was marked as off-topic.

@bradzacher

This comment was marked as resolved.

bradzacher added a commit that referenced this issue May 15, 2021
…owing class generics

Fixes #2592

Unfortunately I think this is impossible to solve from the scope analysis side.. So I opted instead to just ignore it from the lint rule.
@bradzacher bradzacher added the has pr there is a PR raised to close this label May 15, 2021
bradzacher added a commit that referenced this issue May 15, 2021
…owing class generics (#3393)

Fixes #2592

Unfortunately I think this is impossible to solve from the scope analysis side.. So I opted instead to just ignore it from the lint rule.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working has pr there is a PR raised to close this package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
4 participants