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

rafaelss95 · 2021-09-12T00:04:55Z

typescript-eslint · 2021-09-12T00:05:00Z

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 · 2021-09-12T00:09:13Z

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 · 2021-09-12T00:13:47Z

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 · 2021-09-20T21:28:38Z

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

@@ -99,7 +99,9 @@ export default util.createRule<Options, MessageIds>({
      }

      const isShadowedValue =
-        'isValueVariable' in shadowed ? shadowed.isValueVariable : true;
+        !('isValueVariable' in shadowed) ||
+        (shadowed.defs[0]?.type !== DefinitionType.ImportBinding &&


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

Good catch! It should be fixed with the fixup.

bradzacher · 2021-09-20T21:31:24Z

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 · 2021-09-20T23:50:56Z

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 · 2021-09-21T00:16:34Z

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 · 2021-09-21T00:36:14Z

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

bradzacher

LGTM - thanks for your work here!

rafaelss95 force-pushed the fix/3862 branch from c235bc4 to ff24821 Compare September 12, 2021 00:09

bradzacher added the bug Something isn't working label Sep 13, 2021

bradzacher requested changes Sep 20, 2021

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Sep 20, 2021

rafaelss95 force-pushed the fix/3862 branch from ff24821 to cb1634a Compare September 21, 2021 00:34

rafaelss95 added 2 commits September 20, 2021 21:35

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

2ab571e

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

ebea51a

rafaelss95 force-pushed the fix/3862 branch from cb1634a to ebea51a Compare September 21, 2021 00:35

This was referenced Sep 21, 2021

feat(eslint-plugin-internal): [prefer-ast-types-enum] add DefinitionType enum #3916

Merged

fix(eslint-plugin): [no-require-imports] report only global require #3871

Merged

rafaelss95 requested a review from bradzacher September 21, 2021 04:23

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Sep 21, 2021

bradzacher approved these changes Sep 21, 2021

View reviewed changes

bradzacher merged commit dda9cee into typescript-eslint:master Sep 21, 2021

rafaelss95 deleted the fix/3862 branch September 21, 2021 11:38

This was referenced Oct 1, 2021

build(deps-dev): bump @typescript-eslint/eslint-plugin from 4.30.0 to 4.32.0 numbersprotocol/capture-lite#954

Closed

build(deps-dev): bump @typescript-eslint/parser from 4.30.0 to 4.32.0 numbersprotocol/capture-lite#921

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rafaelss95 commented Sep 12, 2021

typescript-eslint bot commented Sep 12, 2021

nx-cloud bot commented Sep 12, 2021 •

edited

Loading

codecov bot commented Sep 12, 2021 •

edited

Loading

bradzacher left a comment

bradzacher Sep 20, 2021

rafaelss95 Sep 21, 2021

bradzacher commented Sep 20, 2021

rafaelss95 commented Sep 20, 2021

bradzacher commented Sep 21, 2021

rafaelss95 commented Sep 21, 2021

bradzacher left a comment

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

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

Conversation

rafaelss95 commented Sep 12, 2021

typescript-eslint bot commented Sep 12, 2021

nx-cloud bot commented Sep 12, 2021 • edited Loading

Nx Cloud Report

codecov bot commented Sep 12, 2021 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Sep 20, 2021

Choose a reason for hiding this comment

rafaelss95 Sep 21, 2021

Choose a reason for hiding this comment

bradzacher commented Sep 20, 2021

rafaelss95 commented Sep 20, 2021

bradzacher commented Sep 21, 2021

rafaelss95 commented Sep 21, 2021

bradzacher left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Sep 12, 2021 •

edited

Loading

codecov bot commented Sep 12, 2021 •

edited

Loading