-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
No did-you-mean-to-call error on casts #42626
Conversation
I chose to do the ad-hoc check rather than yet another tree walk. 1. It's faster to run and easier to read. 2. This error came from looking at real code. It happened twice, so I think the best estimate for other uses that happened zero times is in fact zero. 3. I couldn't think of other places to put the cast, given the restrictions on `testedNode` just before the new code.
src/compiler/checker.ts
Outdated
&& isParenthesizedExpression(location.expression) | ||
&& isAssertionExpression(location.expression.expression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is weird that adding a layer of parens will throw this off. Maybe use skipParentheses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea
Will this need to be cherry-picked into 4.2 stable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to @DanielRosenwasser's comment, I noticed that this:
function f(result: unknown) {
if (((result as I).always)) {
return result
}
}
doesn't throw the error. (So if it's just a rare case, it could be used
to avoid it?)
(Just a comment, I don't have an opinion on these errors...)
@elibarzilay Casts are a usual way for me to try to silence an error I know is bogus, so I think it's important that this works. |
OOPS. I merged this without the followup commit. I opened a new PR here: #42779 |
I think it's probably safe to error less. Pinging @RyanCavanaugh for thoughts. In the meantime... @typescript-bot cherry-pick this to release-4.2 |
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
Hey @DanielRosenwasser, I've opened #42780 for you. |
Component commits: 214ef0c No did-you-mean-to-call error on casts I chose to do the ad-hoc check rather than yet another tree walk. 1. It's faster to run and easier to read. 2. This error came from looking at real code. It happened twice, so I think the best estimate for other uses that happened zero times is in fact zero. 3. I couldn't think of other places to put the cast, given the restrictions on `testedNode` just before the new code. 1d34778 Merge branch 'master' into no-did-you-mean-to-call-error-on-casts
Component commits: 214ef0c No did-you-mean-to-call error on casts I chose to do the ad-hoc check rather than yet another tree walk. 1. It's faster to run and easier to read. 2. This error came from looking at real code. It happened twice, so I think the best estimate for other uses that happened zero times is in fact zero. 3. I couldn't think of other places to put the cast, given the restrictions on `testedNode` just before the new code. 1d34778 Merge branch 'master' into no-did-you-mean-to-call-error-on-casts Co-authored-by: Nathan Shively-Sanders <nathansa@microsoft.com>
I chose to do the ad-hoc check rather than yet another tree walk.
testedNode
just before the new code.Fixes #41640