fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation #10616

ronami · 2025-01-05T19:11:06Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-shadow] false positive with augmented modules #5681
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR addresses #5681 and makes the rule not report on some valid ways to use module augmentation.

The rule already has logic that excludes declaration merging in module augmentation from being reported (added in #3959):

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

Lines 260 to 279 in c7154bf

    
           function isExternalDeclarationMerging( 
        
             scope: TSESLint.Scope.Scope, 
        
             variable: TSESLint.Scope.Variable, 
        
             shadowed: TSESLint.Scope.Variable, 
        
           ): boolean { 
        
             const [firstDefinition] = shadowed.defs; 
        
             const [secondDefinition] = variable.defs; 
        
             return ( 
        
               isTypeImport(firstDefinition) && 
        
               isImportDeclaration(firstDefinition.parent) && 
        
               isExternalModuleDeclarationWithName( 
        
                 scope, 
        
                 firstDefinition.parent.source.value, 
        
               ) && 
        
               secondDefinition.node.type === AST_NODE_TYPES.TSInterfaceDeclaration && 
        
               secondDefinition.node.parent.type === 
        
                 AST_NODE_TYPES.ExportNamedDeclaration 
        
             ); 
        
           }

The current logic specifically looks for interface declarations (and not type alias declarations) and requires types to be explicitly exported.

To my understanding, these two restrictions aren't relevant, as these are valid ways to use module augmentation. I'm basing this on the typescript's handbook and testing this locally (I don't think this can be reproduced on the playground, as TypeScript would show a Cannot find module error).

As far as I know, type aliases cannot be used for declaration merging (e.g. defined multiple times) but can be exported from an augmented module (and extend the original module with new type exports).

Testing this locally, it seems that TypeScript is OK with type and interfaces declarations not being explicitly exported, but I couldn't find anything official on this.

I'm far from having a thorough understanding of declaration merging, so I'd be happy to hear opinions on this.

… various ways to have augmented modules

typescript-eslint · 2025-01-05T19:11:09Z

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.

nx-cloud · 2025-01-05T19:11:55Z

View your CI Pipeline Execution ↗ for commit c889be9.

Command	Status	Duration	Result
`nx test eslint-plugin`	✅ Succeeded	5m 53s	View ↗
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 47s	View ↗
`nx test visitor-keys`	✅ Succeeded	<1s	View ↗
`nx run types:build`	✅ Succeeded	<1s	View ↗
`nx test utils`	✅ Succeeded	<1s	View ↗
`nx test type-utils`	✅ Succeeded	<1s	View ↗
`nx run-many --target=build --exclude website --...`	✅ Succeeded	25s	View ↗
`nx test typescript-eslint`	✅ Succeeded	4s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-23 20:26:49 UTC

netlify · 2025-01-05T19:12:19Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`c889be9`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6792a121cb9d390009b32c26
😎 Deploy Preview	https://deploy-preview-10616--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 (no change 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.

JoshuaKGoldberg

I was procrastinating reviewing this pretty intensely because of it touching declaration merging and shadowing... but it's actually really not that much code I think. 👍 from me.

Also ccing @bradzacher in case they have time to take a look too, as Brad's seen some things I tend to not recall.

JoshuaKGoldberg · 2025-01-23T13:18:12Z

@ronami the merge conflicts are in no-shadow from your other PR #10593, could you resolve conflicts? I started trying but they look nontrivial.

ronami · 2025-01-23T18:44:55Z

@ronami the merge conflicts are in no-shadow from your other PR #10593, could you resolve conflicts? I started trying but they look nontrivial.

Oh, I didn't noticed this had conflicts. Sure, I've resolved them 👍

codecov · 2025-01-23T18:45:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.19%. Comparing base (9e8828b) to head (c889be9).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10616   +/-   ##
=======================================
  Coverage   87.19%   87.19%           
=======================================
  Files         450      450           
  Lines       15632    15632           
  Branches     4570     4570           
=======================================
  Hits        13630    13630           
  Misses       1645     1645           
  Partials      357      357

Flag	Coverage Δ
unittest	`87.19% <ø> (ø)`

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% <ø> (ø)`

ronami · 2025-01-23T19:21:56Z

~~I'm not sure why CI failed, the error I see is GitHub Actions has encountered an internal error when running your job.. Should I try re-running CI?~~

~~Tests seem to pass locally.~~

Edit: Green, thanks! 😄

JoshuaKGoldberg

🔥

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - 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.

@OlivierZal

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.21.0 | 8.22.0 | | npm | @typescript-eslint/parser | 8.21.0 | 8.22.0 | ## [v8.22.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8220-2025-01-27) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-template-expression] handle template literal type ([#10612](typescript-eslint/typescript-eslint#10612)) - **eslint-plugin:** \[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor ([#10552](typescript-eslint/typescript-eslint#10552)) - **eslint-plugin:** \[no-extraneous-class] handle accessor keyword ([#10678](typescript-eslint/typescript-eslint#10678)) - **eslint-plugin:** \[no-shadow] don't report unnecessarily on valid ways of using module augmentation ([#10616](typescript-eslint/typescript-eslint#10616)) - **eslint-plugin:** \[no-duplicate-type-constituents] handle nested types ([#10638](typescript-eslint/typescript-eslint#10638)) - **eslint-plugin:** \[prefer-nullish-coalescing] doesn't report on ternary but on equivalent || ([#10517](typescript-eslint/typescript-eslint#10517)) ##### ❤️ Thank You - mdm317 - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - 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.

remove unnecessary checks so the rule correctly doesn't report on the…

a4ebde3

… various ways to have augmented modules

ronami marked this pull request as ready for review January 5, 2025 19:32

JoshuaKGoldberg previously approved these changes Jan 19, 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 19, 2025

bradzacher previously approved these changes Jan 22, 2025

View reviewed changes

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

ronami added 2 commits January 23, 2025 20:41

merge main branch

e1ea882

add tests for exported interface and non-exported type alias declaration

a3e2979

ronami dismissed stale reviews from bradzacher and JoshuaKGoldberg via a3e2979 January 23, 2025 18:44

Merge branch 'main' into no-shadow-fix-module-augmentation

c889be9

kirkwaiblinger requested review from JoshuaKGoldberg and bradzacher January 24, 2025 14:43

bradzacher approved these changes Jan 24, 2025

View reviewed changes

JoshuaKGoldberg approved these changes Jan 24, 2025

View reviewed changes

JoshuaKGoldberg merged commit 586e7eb into typescript-eslint:main Jan 24, 2025
64 checks passed

ronami deleted the no-shadow-fix-module-augmentation branch January 25, 2025 09:52

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation #10616

fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation #10616

ronami commented Jan 5, 2025

typescript-eslint bot commented Jan 5, 2025

nx-cloud bot commented Jan 5, 2025 •

edited

Loading

netlify bot commented Jan 5, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Jan 23, 2025

ronami commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

ronami commented Jan 23, 2025 •

edited

Loading

JoshuaKGoldberg left a comment

	function isExternalDeclarationMerging(
	scope: TSESLint.Scope.Scope,
	variable: TSESLint.Scope.Variable,
	shadowed: TSESLint.Scope.Variable,
	): boolean {
	const [firstDefinition] = shadowed.defs;
	const [secondDefinition] = variable.defs;

	return (
	isTypeImport(firstDefinition) &&
	isImportDeclaration(firstDefinition.parent) &&
	isExternalModuleDeclarationWithName(
	scope,
	firstDefinition.parent.source.value,
	) &&
	secondDefinition.node.type === AST_NODE_TYPES.TSInterfaceDeclaration &&
	secondDefinition.node.parent.type ===
	AST_NODE_TYPES.ExportNamedDeclaration
	);
	}

fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation #10616

fix(eslint-plugin): [no-shadow] don't report unnecessarily on valid ways of using module augmentation #10616

Conversation

ronami commented Jan 5, 2025

PR Checklist

Overview

typescript-eslint bot commented Jan 5, 2025

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

netlify bot commented Jan 5, 2025 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jan 23, 2025

ronami commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

ronami commented Jan 23, 2025 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

nx-cloud bot commented Jan 5, 2025 •

edited

Loading

netlify bot commented Jan 5, 2025 •

edited

Loading

codecov bot commented Jan 23, 2025 •

edited

Loading

ronami commented Jan 23, 2025 •

edited

Loading