fix(eslint-plugin): [no-unnecessary-type-assertion] fix false negative for const variable declarations

[abrahamguo]

PR Checklist

• Addresses an existing open issue: fixes [no-unnecessary-type-assertion] only checks non-null-assertion #4485 and fixes Docs: [no-unnecessary-type-assertion] incorrect examples don't report errors #8458
• That issue was marked as accepting prs
• Steps in Contributing were taken

Overview

The no-unnecessary-type-assertions rule needs a bit more detail about when type assertions are truly useful or useless on literals.

[typescript-eslint]

Thanks for the PR, @abrahamguo!

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	69f2cca
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/65f752740382d70008132bf6
😎 Deploy Preview	https://deploy-preview-8558--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 97 (🟢 up 4 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]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 69f2cca. 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 type-utils
• nx run types:build
• nx test typescript-estree
• nx run repo-tools:postinstall-script
• nx test typescript-eslint
• nx test scope-manager
• nx test eslint-plugin --coverage=false
• nx test rule-schema-to-typescript-types
• nx test utils
• nx test parser
• nx run-many --target=build --parallel --exclude=website --exclude=website-eslint
• nx test visitor-keys
• nx test repo-tools
• nx test eslint-plugin-internal
• nx test ast-spec
• nx run-many --target=lint --parallel --exclude eslint-plugin
• nx test visitor-keys --coverage=false
• nx test typescript-estree --coverage=false
• nx test typescript-eslint --coverage=false
• nx test utils --coverage=false
• nx test type-utils --coverage=false
• nx test repo-tools --coverage=false
• nx run-many --target=typecheck --parallel
• nx test rule-schema-to-typescript-types --coverage=false
• nx test scope-manager --coverage=false
• nx test ast-spec --coverage=false
• nx test eslint-plugin-internal --coverage=false
• nx run repo-tools:generate-configs
• nx test parser --coverage=false
• nx run integration-tests:test

---

Sent with 💌 from NxCloud.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.09%. Comparing base (3856c67) to head (69f2cca).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8558      +/-   ##
==========================================
- Coverage   88.10%   88.09%   -0.02%     
==========================================
  Files         402      402              
  Lines       14029    14015      -14     
  Branches     4117     4112       -5     
==========================================
- Hits        12360    12346      -14     
  Misses       1370     1370              
  Partials      299      299              

Flag	Coverage Δ
unittest	88.09% <100.00%> (-0.02%)	⬇️

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

Files	Coverage Δ
...-plugin/src/rules/no-unnecessary-type-assertion.ts	96.96% <100.00%> (-0.54%)	⬇️

[bradzacher]

See the end of this comment #4485 (comment) for details of the changes we would accept to the rule.

Furthermore, the TSLint PR above also introduced some special handling for tuple types, which appears to be unnecessary, so it has been removed.

playground is this not the case that this assertion is required?

Or are you saying that the logic isn't required because it's never hit?

[abrahamguo]

is this not the case that this assertion is required?
Or are you saying that the logic isn't required because it's never hit?

@bradzacher this assertion is indeed required; however, no special handling is required for it (or, for any tuple types, I believe) in this lint rule because services.getTypeAtLocation(node.expression) === castType returns false.

[abrahamguo]

Alrighty — I still need to clean up the tests a little bit, but I've reviewed (and added tests for):

• the examples (some of which are currently broken) from the current docs, discussed further in #8458
• the examples from the original TSLint PR
• and all examples both invalid and valid, from the open issue [no-unnecessary-type-assertion] only checks non-null-assertion #4485

and I am confident that my logic is correct for detecting useful vs useless literal type assertions:

• First, as discussed above, any special logic for tuple type assertions is unnecessary. The base condition of castType === uncastType always returns false for any non-primitive type (in other words, this rule seems to do nothing at all for non-primitive types. Something to investigate for the future?)
• The new logic is in the wouldSameTypeBeInferred variable. There are two cases to consider, depending on whether castType.isLiteral() returns true or false.
• Note that castType.isLiteral() only returns true if the type assertion is a single literal. If it is a union of literals, for example, then castType.isLiteral() will return false.
• if castType.isLiteral() returns true, then we know that the same type will be inferred, and therefore we are OK to report, if and only if we are within a const variable declaration. This includes reporting on an as const of a primitive within a const variable declaration.
• On the other hand, if castType.isLiteral() returns false, then we must be looking at a more complex type, and we should report (assuming that typeIsUnchanged is also true) unless this is a const assertion, in which case, do not report.

Things to investigate for the future:

• Make this rule work for contextual typing (already noted as a TODO)
• Make this rule work for non-primitives (did it ever in the past? unsure)

[auvred]

Overall looks good to me! Thanks for working on this! 🚀

Just requesting splitting the test cases

[auvred]

[non-actionable] Hmmm, looks like these cases are not reported (playground)

let a: unknown
const b = (a = 1 as 1)
const c = (console.log(1), 1 as 1)

However, I'm not sure we can easily detect all such cases. This function might become too complex...

[JoshuaKGoldberg]

Yeah I think we can leave these for now. If someone files an issue showing them being buggy in isolation, then that'd indicate there's enough user appetite to start thinking about whether the complexity is worth it.

[auvred]

Getting closer! 🙌

I think the main blocker in my mind is updating correct example in the docs

[auvred]

LGTM 🔥

Thanks for working on this!

[JoshuaKGoldberg]

🙌 swell, thanks!