fix(eslint-plugin): [no-shadow] ignore type-only imports properly

[rafaelss95]

Fixes #3862.

[typescript-eslint]

Thanks for the PR, @rafaelss95!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[nx-cloud]

Nx Cloud Report

CI ran the following commands for commit ebea51a. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status	Command
	nx test @typescript-eslint/eslint-plugin-tslint
	nx test @typescript-eslint/eslint-plugin
	nx test @typescript-eslint/eslint-plugin-tslint
	nx test @typescript-eslint/eslint-plugin
	nx test @typescript-eslint/eslint-plugin-internal
	nx test @typescript-eslint/eslint-plugin-tslint
	nx test @typescript-eslint/eslint-plugin
	nx run-many --target=typecheck --all --parallel
	nx test @typescript-eslint/parser
	nx test @typescript-eslint/experimental-utils
	nx test @typescript-eslint/scope-manager
	nx test @typescript-eslint/visitor-keys

---

Sent with 💌 from NxCloud.

[codecov]

Codecov Report

Merging #3868 (ebea51a) into master (4195919) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3868   +/-   ##
=======================================
  Coverage   93.50%   93.51%           
=======================================
  Files         151      151           
  Lines        8134     8137    +3     
  Branches     2580     2581    +1     
=======================================
+ Hits         7606     7609    +3     
  Misses        167      167           
  Partials      361      361           

Flag	Coverage Δ
unittest	93.51% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/no-shadow.ts	77.16% <100.00%> (+0.55%)	⬆️

[bradzacher]

thanks for this!
almost there

[bradzacher]

this isn't quite correct because it will match all import variables.
We specifically only want to match variables imported via import type.

[rafaelss95]

Good catch! It should be fixed with the fixup.

[bradzacher]

This is off-topic from your current change - could you please also update these two lines:

typescript-eslint/packages/eslint-plugin/src/rules/no-shadow.ts

Line 85 in 955d7a6

return variable.defs[0].type === 'Parameter' && variable.name === 'this';

typescript-eslint/packages/eslint-plugin/src/rules/no-shadow.ts

Line 279 in 955d7a6

((innerDef.type === 'FunctionName' &&

To use DefinitionType like you've imported? It'd be great to ensure we're doing the comparisons the same everywhere :)

[rafaelss95]

This is off-topic from your current change - could you please also update these two lines:

typescript-eslint/packages/eslint-plugin/src/rules/no-shadow.ts

Line 85 in 955d7a6

return variable.defs[0].type === 'Parameter' && variable.name === 'this';

typescript-eslint/packages/eslint-plugin/src/rules/no-shadow.ts

Line 279 in 955d7a6

((innerDef.type === 'FunctionName' &&

To use DefinitionType like you've imported? It'd be great to ensure we're doing the comparisons the same everywhere :)

What about adding DefinitionType to prefer-ast-types-enum rule and fix all possible occurrences once in a separate PR? Would this work for you?

[bradzacher]

What about adding DefinitionType to prefer-ast-types-enum rule and fix all possible occurrences once in a separate PR? Would this work for you?

You can certainly give that a go - I'm not sure if that will false-positive a lot (due to words like Variable and Type being part of the member). It might be noisy - but we can see if it is or not.

There's an open issue to create a generic rule to do that enum check (#308)

[rafaelss95]

I've tested it locally and I think it'll be fine, I'll send it ASAP.

[bradzacher]

LGTM - thanks for your work here!