fix(type-utils): ensure external sources don't match for the FileSpecifier

[RebeccaStevens]

PR Checklist

• Addresses an existing open issue: fixes Bug: v6 typeMatchesSpecifier FileSpecifier checks in node modules #6819
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

This is an alternative to #6860 (closes #6860).

[typescript-eslint]

Thanks for the PR, @RebeccaStevens!

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]

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	6f3bdbf
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/64340e2515ccf900080f4b86
😎 Deploy Preview	https://deploy-preview-6866--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[RebeccaStevens]

@marekdedic How's this approach look to you?

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 6f3bdbf. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 30 targets

• Node 18 - nx test eslint-plugin
• Node 14 - nx test eslint-plugin --coverage=false
• nx run-many --target=build --projects=website-eslint,website
• nx run-many --target=lint --parallel
• Node 18 - nx test scope-manager
• Node 18 - nx test typescript-estree
• Node 18 - nx test utils
• Node 18 - nx test visitor-keys
• Node 18 - nx run types:build
• Node 18 - nx test repo-tools
• Node 18 - nx test type-utils
• nx run types:build
• nx run-many --target=typecheck --parallel
• nx run integration-tests:test
• Node 18 - nx test eslint-plugin-tslint
• Node 14 - nx test typescript-estree --coverage=false
• Node 18 - nx test parser
• Node 14 - nx test utils --coverage=false
• Node 18 - nx test ast-spec
• Node 14 - nx run types:build
• Node 14 - nx test type-utils --coverage=false
• Node 14 - nx test scope-manager --coverage=false
• Node 14 - nx test parser --coverage=false
• Node 18 - nx test eslint-plugin-internal
• Node 14 - nx test ast-spec --coverage=false
• Node 14 - nx test eslint-plugin-internal --coverage=false
• Node 14 - nx test eslint-plugin-tslint --coverage=false
• Node 14 - nx test visitor-keys --coverage=false
• Node 14 - nx test repo-tools --coverage=false
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint

---

Sent with 💌 from NxCloud.

[codecov]

Codecov Report

Merging #6866 (6f3bdbf) into v6 (a35a2bb) will decrease coverage by 0.02%.
The diff coverage is 70.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##               v6    #6866      +/-   ##
==========================================
- Coverage   87.46%   87.45%   -0.02%     
==========================================
  Files         374      374              
  Lines       12879    12892      +13     
  Branches     3811     3813       +2     
==========================================
+ Hits        11265    11275      +10     
- Misses       1229     1231       +2     
- Partials      385      386       +1     

Flag	Coverage Δ
unittest	87.45% <70.00%> (-0.02%)	⬇️

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

Impacted Files	Coverage Δ
packages/type-utils/src/TypeOrValueSpecifier.ts	83.33% <70.00%> (-5.96%)	⬇️

... and 2 files with indirect coverage changes

[marekdedic]

Hi, thanks for the alternative proposal!

Reading quickly through this, I see this mainly builds on the ts.getEffectiveTypeRoots() function, which I didn't know before - quick googling led me here - I understand this description so that this would exclude all types declared in some @types/... package - however, if I understand it correctly, the type roots don't include folders for packages that ship their own types - so for example types from the vue package would still be found. Is that correct or am I missing something?

[marekdedic]

Oh actually, I see isSourceFileFromExternalLibrary there as well (previously thought that was the default library instead of external...) - reading the source code of that, I am confused - wouldn't this function alone do exactly what we want it to do - exclude stuff from node_modules? Or does that not include @types/... packages - then I'd understand the type roots bit :D

Sorry, if this does what I think it does then it's great, but as you see, I'm not that well versed with TS internals :/

[RebeccaStevens]

This approach actually relies more on program.isSourceFileFromExternalLibrary(declaration).

ts.getEffectiveTypeRoots() is a little extra to support custom type root paths. The function will return [/absolute/path/to/project/node_modules/@types] unless typeRoots is specified in the tsconfig.json file.

[marekdedic]

Hmm, yes, that's what I thought in my second comment, sorry for the confusion. I am still not 100% why the type roots are needed - if program.isSourceFileFromExternalLibrary(declaration) supports custom node_modules location, then typeRoots wouldn't be needed, right? And if it does, then this will be a half-measure - @types/ packages will be excluded, regular packages wouldn't, right?

Anyway, apart from this, LGTM.

[marekdedic]

Could you please add a test for this?

[JoshuaKGoldberg]

Test coverage please? I see a missing line 😄

[marekdedic]

Oh, and just for the original question - IMHO, I prefer this to the alternative PR #6860

[JoshuaKGoldberg]

This was unintentionally auto-closed when we merged the v6 branch 🙃 it'll be recreated reopened.

[JoshuaKGoldberg]

👋 Hey @RebeccaStevens! Just checking in, is this still something you have time for? No worries if not - I just don't want to leave it hanging.

[JoshuaKGoldberg]

Closing this PR as it's been stale for a while without activity. Feel free to reopen @RebeccaStevens if you have time - but no worries if not!

If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding a co-author at the end of your PR description. Thanks! 😊