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-shadow] false positive for generics, callback argument names and more - considered as shadowing since v4.0.0 #2480

Closed
3 tasks done
bengry opened this issue Sep 3, 2020 · 2 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended

Comments

@bengry
Copy link

bengry commented Sep 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

Two issues regarding the same rule no-shadow arose after upgrading to v4.0.2 (and reproduces onn v4.0.2-alpha.2 for the following configuration. v3.10.1 was fine for the same code.

{
  "rules": {
    "no-shadow": "error"
  }
}

I opened 1 issue since it's all on the same rule, but LMK if I should open separate issue for the separate cases:

1. Callback arguments and parameters:

function FilterListItem<T extends string>({ option, onChange }: {
	option: T;
	onChange: (option: T) => void;
}) { ... }

This reports the following error:

error 'option' is already declared in the upper scope no-shadow

2. Generics in static methods

export class Selector<K extends string> {
  public static someStaticMethod<K extends string>(): void;
}

In reality this is a React component, and the static method is getDerivedStateFromProps, but it's the same idea.

This reports the following error:

error 'K' is already declared in the upper scope no-shadow

3. Mapped types

export type DeepReadonly<T> = T extends Primitive
  ? T
  : T extends Array<infer U>
  ? ReadonlyArray<DeepReadonly<U>>
  : T extends Map<infer K, infer V>
  ? ReadonlyMap<DeepReadonly<K>, DeepReadonly<V>>
  : T extends Set<infer M>
  ? ReadonlySet<DeepReadonly<M>>
  : { readonly [K in keyof T]: DeepReadonly<T[K]> };

This reports the following error:

error 'K' is already declared in the upper scope no-shadow

Also related:

export type DeepMutable<T> = T extends Primitive
  ? T
  : T extends ReadonlyArray<infer U>
  ? Array<DeepMutable<U>>
  : T extends ReadonlyMap<infer K, infer V>
  ? Map<DeepMutable<K>, DeepMutable<V>>
  : T extends ReadonlySet<infer U>
  ? Set<DeepMutable<U>>
  : { -readonly [K in keyof T]: DeepMutable<T[K]> };

reports:

317:33 error 'U' is already declared in the upper scope no-shadow
319:18 error 'K' is already declared in the upper scope no-shadow

Expected Result
No ESLint errors for these examples.

Actual Result

Errors are reported, as shown above.
Additional Info

Versions

package version
@typescript-eslint/eslint-plugin 4.0.1
@typescript-eslint/parser 4.0.1
TypeScript 4.0.2
ESLint 7.8.1
node 12.18.3
@bengry bengry added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Sep 3, 2020
@bengry bengry changed the title [no-shadow] false positive for generics and callback argument names are considered as shadowing in 4.0.1 [no-shadow] false positive for generics, callback argument names and more - considered as shadowing since v4.0.0 Sep 3, 2020
@bradzacher
Copy link
Member

bradzacher commented Sep 3, 2020

(1) is already fixed in master.

As per the release notes, you need to use @typescript-eslint/no-shadow, not the base rule.

(2) is correctly reporting.

You have shadowed the generic definition on the class with the generic definition on the function.

It doesn't matter that its a static method, both examples define generics of the same name. The method is a child of the class, hence it shadows.

Example of when this can be confusing

class Foo<T> {
  static method<T>(): T {} // which T is this referring to?
}

const result = Foo<string>.method<number>();
// what's the type of result?

(3)(a) is correctly reporting.

infer K defines a type named K.
K in keyof T defines a type named K

(3)(b) is also correctly reporting.

You have two instances of infer U.
And the same issue as before for K.

@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed triage Waiting for maintainers to take a look labels Sep 3, 2020
@bengry
Copy link
Author

bengry commented Sep 15, 2020

@bradzacher Sorry for the long delay, haven't had the time to look into it again until now.
Re 2. If you remove the generic from the static method you get a TS2302 error ("Static members cannot reference class type parameters"), so I think that this one is incorrect. Sure, I can rename the static method's K to some other name and have the warning gone, but since I can't reference the class' generic K anyway - might as well use the same name and have ESLint not report it.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin working as intended Issues that are closed as they are working as intended
Projects
None yet
Development

No branches or pull requests

2 participants