feat(eslint-plugin): [consistent-type-imports] ignore files with decorators, experimentalDecorators, and emitDecoratorMetadata

[bradzacher]

PR Checklist

• Addresses an existing open issue: fixes Bug: [consistent-type-imports] Using with isolatedModules and emitDecoratorMetadata #5468, fixes Docs: consistent-type-imports vs importsNotUsedAsValues #6393
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

Note: I've labelled this as a feat on purpose to mark it for a minor bump. It's technically a "bug fix" but it's also a change in behaviour. One might classify it as a breaking change because a specific usecase will require a config change - but given that usecases is also already broken, I didn't think that it truly qualified as breaking.

---

This PR implements a sledge-hammer approach to fixing the problem in #5468 - I have opted to just completely ignore a file if it passes all three of the following conditions:

• there is a decorator in the file
• experimentalDecorators: true
• emitDecoratorMetadata: true

Originally in #5468 I suggested going with the the approach of specifically ignoring imports that are used in locations that might be passed to decorator metadata. However I ultimately chose not to go down this path because:

• As of TS 5.0 experimentalDecorators is considered legacy
• The stable TC39 decorator metadata proposal does not include type annotations
• verbatimModuleSyntax exists for the above usecase
• The logic required to concretely detect if a specifier is used within decorator metadata is complex and a maintenance burden and only specifically applies to experimentalDecorators

This PR:

• Removes the decorator metadata detection logic from scope-manager
• Adds a new experimentalDecorators parser option that is auto-populated from type info if available
• Exposes both experimentalDecorators and emitDecoratorMetadata on the parser services
  • I did this to make it easy for rules to derive this state without having to manually interact with the program to extract the compiler options.
• Updates consistent-type-imports:
  • so that it bails out of reporting on a file if the aforementioned 3 conditions are met.
  • removes the error messages relating to decorator metatadata
• Refactors the tests to validate the conditions are correctly applied.

NOTE:: I strongly suggest turning on the no-whitespace diff when reviewing this PR.

[typescript-eslint]

Thanks for the PR, @bradzacher!

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	5749a57
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6600b24e92a82200086d6b88
😎 Deploy Preview	https://deploy-preview-8335--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.

[codecov]

Codecov Report

Attention: Patch coverage is 92.38095% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 87.98%. Comparing base (4132374) to head (5749a57).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8335      +/-   ##
==========================================
+ Coverage   87.38%   87.98%   +0.59%     
==========================================
  Files         254      404     +150     
  Lines       12489    14045    +1556     
  Branches     3919     4110     +191     
==========================================
+ Hits        10914    12358    +1444     
- Misses       1304     1382      +78     
- Partials      271      305      +34     

Flag	Coverage Δ
unittest	87.98% <92.38%> (+0.59%)	⬆️

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

Files	Coverage Δ
packages/scope-manager/src/ScopeManager.ts	77.64% <ø> (ø)
packages/scope-manager/src/analyze.ts	61.11% <ø> (ø)
...kages/scope-manager/src/referencer/ClassVisitor.ts	78.78% <100.00%> (ø)
...ackages/scope-manager/src/referencer/Referencer.ts	95.52% <100.00%> (ø)
...ages/typescript-estree/src/createParserServices.ts	60.00% <50.00%> (-11.43%)	⬇️
...eslint-plugin/src/rules/consistent-type-imports.ts	94.77% <93.54%> (-1.21%)	⬇️

... and 146 files with indirect coverage changes

[JoshuaKGoldberg]

Really nice solution, +1 from me on the direction!

Requesting changes mainly on extracting the deeper explanations out of the rule file and into a blog post. Also for at least resolving the breaking change discussion.

But the rule's new implementation looks great to me (thanks for the whitespace suggestion!). ✨

[ZackMFleischman]

@bradzacher Thanks for building this solution! I'm eagerly awaiting it being merged so I can use it in my project where I've been forced to just turn this rule off entirely for the moment.

Also hi @JoshuaKGoldberg! Always fun to see your name pop up. Thanks for fighting the good fight and keeping high-quality open source software available! 😄

[JoshuaKGoldberg]

Haha hey @ZackMFleischman, great to see you pop up too! 😄

[nx-cloud]

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 31 targets

• nx test eslint-plugin
• nx test typescript-estree
• nx test visitor-keys
• nx test type-utils
• nx test typescript-eslint
• nx run types:build
• nx test utils
• nx run repo-tools:postinstall-script
• nx test rule-schema-to-typescript-types
• nx test eslint-plugin --coverage=false
• nx test scope-manager
• nx run-many --target=lint --parallel --exclude eslint-plugin
• nx test parser
• nx test repo-tools
• nx test eslint-plugin-internal
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx test ast-spec
• nx test typescript-estree --coverage=false
• nx run-many --target=typecheck --parallel
• nx test parser --coverage=false
• nx test scope-manager --coverage=false
• nx test type-utils --coverage=false
• nx test visitor-keys --coverage=false
• nx test typescript-eslint --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx test ast-spec --coverage=false
• nx test repo-tools --coverage=false
• nx test rule-schema-to-typescript-types --coverage=false
• nx test utils --coverage=false
• nx run integration-tests:test
• nx run repo-tools:generate-configs

---

Sent with 💌 from NxCloud.

[JoshuaKGoldberg]

Awesome! I'm super happy with the implementation, and think I understand this much better now thanks to the docs & comments. Thanks!

Left some proofreading thoughts on the blog post. That's the only real blocker from me - and there's nothing I likely can't be convinced otherwise on.

🔥

[JoshuaKGoldberg]

Whoop, this is great - useful userland fixing and really solid explanations @bradzacher! 👏

[JoshuaKGoldberg]

Nit: this isn't used later.

Suggested change

var _a;

If TypeScript is producing an _a despite never using it, then that's a bug we can ignore.

[bradzacher]

Oh nah it was there cos the code has no types so this is an extra case I (purposely) didn't mention. In this case when TS can't determine the type but it can see its a value-import - instead TS emits [typeof _a = (typeof Foo !== "undefined" && Foo) === "function" ? _a : Object]. Which is some funky code-golf heh.

Can remove it though yeah as it's not necessary for the examples I'm mentioning.