fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

ronami · 2025-01-02T20:05:56Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-shadow] Doesn't catch shadowing of global types in generics #5935
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR attempts to tackle #5935.

Some thoughts/questions:

I've expanded the existing hoist configuration to include types and functions-and-types. I don't think this is optimal, as permutations need to be manually set (e.g. functions-and-types). It also adds some complexity to this configuration, but I'm not sure if there's a non-breaking alternative.

Since this is an extension rule, this somewhat changes the base rule's option, though I did see other rules like no-empty-function which extend an enum type of their base rule.

That said, I'm not sure if someone will ever want to configure this to just functions or just types. It may be better to have the current functions options include types behavior and rename it in a future major (although this would mean the option would diverge from the base rule).

I'm a bit unsure about this, so I'll be happy to hear other opinions.
I've changed the default from functions to functions-and-types. This seems like the most sensible option, as for both, ordering doesn't matter.
I've included documentation for the added option keys, similar to how I saw this done on other extension rules. I wasn't sure whether to use the correct/incorrect <Tabs /> or not. I have decided not to use <Tabs /> to match the rest of the rule's documentation. Please let me know if it would be better to use <Tabs /> regardless.

typescript-eslint · 2025-01-02T20:06:00Z

Thanks for the PR, @ronami!

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.

netlify · 2025-01-02T20:06:19Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`7dd6b06`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67841f7ae774440008323cc3
😎 Deploy Preview	https://deploy-preview-10593--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 98 (🟢 up 5 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2025-01-02T20:06:49Z

View your CI Pipeline Execution ↗ for commit 7dd6b06.

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 15s	View ↗
`nx test eslint-plugin`	✅ Succeeded	5m 47s	View ↗
`nx test typescript-eslint --coverage=false`	✅ Succeeded	26s	View ↗
`nx test utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test type-utils --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test visitor-keys --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test scope-manager --coverage=false`	✅ Succeeded	<1s	View ↗
`nx test rule-tester --coverage=false`	✅ Succeeded	<1s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-12 20:16:52 UTC

ronami · 2025-01-02T21:11:33Z

packages/eslint-plugin/tests/rules/no-shadow/no-shadow.test.ts

+    {
+      code: `
+let x = foo((x, y) => {});
+let y;
+      `,
+      errors: [
+        {
+          data: {
+            name: 'x',
+            shadowedColumn: 5,
+            shadowedLine: 2,
+          },
+          messageId: 'noShadow',
+          type: AST_NODE_TYPES.Identifier,
+        },
+      ],
+      languageOptions: { parserOptions: { ecmaVersion: 6 } },
+      options: [{ hoist: 'functions' }],
+    },


This isn't directly related to the PR, but it seemed there are very few tests that cover the functions option in hoist (which now has two extra options).

👍👍👍 on adding in missing test coverage! Especially if it's at all related to a rule being changed.

JoshuaKGoldberg

🚀

JoshuaKGoldberg · 2025-01-07T13:30:31Z

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

@@ -63,7 +70,7 @@ export default createRule<Options, MessageIds>({
            type: 'string',
            description:
              'Whether to report shadowing before outer functions or variables are defined.',
-            enum: ['all', 'functions', 'never'],
+            enum: ['all', 'functions', 'functions-and-types', 'never', 'types'],


I've expanded the existing hoist configuration to include types and functions-and-types. I don't think this is optional, as permutations need to be manually set ...

Yeah, I'm thinking this exposes a general learning: don't use enum option types for "when do I X?" questions. Eventually folks will want to do something that could be better expressed as an object. I.e. here:

{ optionName: { functions: true, types: true, variables: true } }

...but, to your point, I think it would be even more disruptive to extend the base rule option to a wholly different format in that way. For now I think the enum is fine. If this rule gets even more complicated over time then we can always switch to the object later.

tl;dr: agreed as-is 👍

JoshuaKGoldberg · 2025-01-07T13:31:55Z

packages/eslint-plugin/tests/rules/no-shadow/no-shadow.test.ts

+    {
+      code: `
+let x = foo((x, y) => {});
+let y;
+      `,
+      errors: [
+        {
+          data: {
+            name: 'x',
+            shadowedColumn: 5,
+            shadowedLine: 2,
+          },
+          messageId: 'noShadow',
+          type: AST_NODE_TYPES.Identifier,
+        },
+      ],
+      languageOptions: { parserOptions: { ecmaVersion: 6 } },
+      options: [{ hoist: 'functions' }],
+    },


👍👍👍 on adding in missing test coverage! Especially if it's at all related to a rule being changed.

ronami · 2025-01-12T20:01:10Z

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

+      if (!outerDef) {
+        return true;
+      }


I went over this again and made one small change. I think I've introduced an unnecessary change to the original implementation (returning false instead of true if outerDef is false, see 7dd6b06).

I don't think this code is reachable (all tests seem to pass regardless of this change), and I couldn't reproduce this in a test. Still, just to be sure, I've changed this to match the original implementation.

Now that it's on its own if statement, it fails codecov (even though this wasn't covered previously but was in a single long binary operation). I'm a bit unsure what the best way to tackle this is. I'd normally use nullThrows or a type assertion, but I'm not entirely sure this really is an unreachable line.

This part of the code seems to originate from the original eslint rule and is there from the very beginning.

Yeah I'd just ignore Codecov here. Its reporter isn't perfect.

codecov · 2025-01-12T20:17:20Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.94%. Comparing base (4dbf48b) to head (7dd6b06).
Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
packages/eslint-plugin/src/rules/no-shadow.ts	85.71%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10593      +/-   ##
==========================================
+ Coverage   86.90%   86.94%   +0.04%     
==========================================
  Files         446      446              
  Lines       15502    15520      +18     
  Branches     4516     4523       +7     
==========================================
+ Hits        13472    13494      +22     
+ Misses       1675     1671       -4     
  Partials      355      355

Flag	Coverage Δ
unittest	`86.94% <85.71%> (+0.04%)`	⬆️

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

Files with missing lines	Coverage Δ
packages/eslint-plugin/src/rules/no-shadow.ts	`80.64% <85.71%> (+0.18%)`	⬆️

... and 8 files with indirect coverage changes

JoshuaKGoldberg

👍 from me. Even if the outerDef issue is a bug, I suspect it'd be an existing one - and probably not as impactful as the root issue this PR is fixing. Good note!

@yeonjuan

##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

##### [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.19.1 | 8.20.0 | | npm | @typescript-eslint/parser | 8.19.1 | 8.20.0 | ## [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.19.1 | 8.20.0 | | npm | @typescript-eslint/parser | 8.19.1 | 8.20.0 | ## [v8.20.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8200-2025-01-13) ##### 🚀 Features - **eslint-plugin:** \[consistent-type-assertions] add arrayLiteralTypeAssertions options ([#10565](typescript-eslint/typescript-eslint#10565)) - **eslint-plugin:** \[no-deprecated] add allow options ([#10585](typescript-eslint/typescript-eslint#10585)) - **eslint-plugin:** \[no-misused-spread] add new rule ([#10551](typescript-eslint/typescript-eslint#10551)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] don't flag optional chaining for union types with an unconstrained type parameters ([#10602](typescript-eslint/typescript-eslint#10602)) - **eslint-plugin:** \[no-shadow] ignore ordering of type declarations ([#10593](typescript-eslint/typescript-eslint#10593)) ##### ❤️ Thank You - Josh Goldberg ✨ - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

ronami added 3 commits January 1, 2025 16:16

initial implementation

f9f48f6

add relevant tests

0eb600b

add docs

0a63dc2

snapshots

b164971

ronami marked this pull request as ready for review January 2, 2025 20:52

ronami commented Jan 2, 2025

View reviewed changes

JoshuaKGoldberg previously approved these changes Jan 7, 2025

View reviewed changes

JoshuaKGoldberg added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 7, 2025

match original implementation

7dd6b06

ronami dismissed JoshuaKGoldberg’s stale review via 7dd6b06 January 12, 2025 20:00

ronami commented Jan 12, 2025

View reviewed changes

JoshuaKGoldberg approved these changes Jan 13, 2025

View reviewed changes

JoshuaKGoldberg merged commit 63135f7 into typescript-eslint:main Jan 13, 2025
60 of 62 checks passed

ronami deleted the no-shadow-hoist-type-declarations branch January 13, 2025 20:44

github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

ronami commented Jan 2, 2025 •

edited

Loading

typescript-eslint bot commented Jan 2, 2025

netlify bot commented Jan 2, 2025 •

edited

Loading

nx-cloud bot commented Jan 2, 2025 •

edited

Loading

ronami Jan 2, 2025

JoshuaKGoldberg Jan 7, 2025

JoshuaKGoldberg left a comment

JoshuaKGoldberg Jan 7, 2025

JoshuaKGoldberg Jan 7, 2025

ronami Jan 12, 2025 •

edited

Loading

ronami Jan 12, 2025 •

edited

Loading

JoshuaKGoldberg Jan 13, 2025

codecov bot commented Jan 12, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

fix(eslint-plugin): [no-shadow] ignore ordering of type declarations #10593

Conversation

ronami commented Jan 2, 2025 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Jan 2, 2025

netlify bot commented Jan 2, 2025 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Jan 2, 2025 • edited Loading

ronami Jan 2, 2025

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 7, 2025

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 7, 2025

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 7, 2025

Choose a reason for hiding this comment

ronami Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

ronami Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Jan 13, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2025 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ronami commented Jan 2, 2025 •

edited

Loading

netlify bot commented Jan 2, 2025 •

edited

Loading

nx-cloud bot commented Jan 2, 2025 •

edited

Loading

ronami Jan 12, 2025 •

edited

Loading

ronami Jan 12, 2025 •

edited

Loading

codecov bot commented Jan 12, 2025 •

edited

Loading