Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

abrahamguo
Copy link
Contributor

@abrahamguo abrahamguo commented Feb 27, 2024

PR Checklist

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
Copy link
Contributor

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.

Copy link

netlify bot commented Feb 27, 2024

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...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
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.

Copy link

codecov bot commented Feb 27, 2024

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%) ⬇️

@auvred auvred added the awaiting response Issues waiting for a reply from the OP or another party label Feb 27, 2024
@auvred auvred removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 27, 2024
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Feb 27, 2024
@abrahamguo
Copy link
Contributor Author

abrahamguo commented Feb 27, 2024

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
Copy link
Contributor Author

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

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)

@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 29, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Just requesting splitting the test cases

Comment on lines +109 to +115
function isConstVariableDeclaration(node: TSESTree.Node): boolean {
return (
node.type === AST_NODE_TYPES.VariableDeclaration &&
node.kind === 'const'
);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 auvred changed the title fix: fix false negative for const variable declarations in no-unnecessary-type-assertion fix(eslint-plugin): [no-unnecessary-type-assertion] fix false negative for const variable declarations Mar 4, 2024
@auvred auvred added the awaiting response Issues waiting for a reply from the OP or another party label Mar 4, 2024
@abrahamguo abrahamguo requested a review from auvred March 4, 2024 13:03
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 4, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer! 🙌

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

@auvred auvred added the awaiting response Issues waiting for a reply from the OP or another party label Mar 6, 2024
abrahamguo and others added 3 commits March 7, 2024 06:58
Co-authored-by: auvred <61150013+auvred@users.noreply.github.com>
@abrahamguo abrahamguo requested a review from auvred March 7, 2024 13:06
@github-actions github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Mar 7, 2024
auvred
auvred previously approved these changes Mar 7, 2024
Copy link
Member

@auvred auvred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

Thanks for working on this!

guy applauds

@auvred auvred added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Mar 13, 2024
Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌 swell, thanks!

@JoshuaKGoldberg JoshuaKGoldberg merged commit d2995df into typescript-eslint:main Mar 18, 2024
58 of 59 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 26, 2024
@abrahamguo abrahamguo deleted the no-unnecessary-type-assertion branch April 2, 2024 01:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge
Projects
None yet
4 participants