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

No did-you-mean-to-call error on casts #42626

Merged
merged 2 commits into from Feb 12, 2021

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Feb 3, 2021

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 only twice, so I think the best estimate for other ways people have included a cast is zero, given that they've happened zero times so far.
  3. I couldn't think of other places to put the cast, given the restrictions on testedNode just before the new code.

Fixes #41640

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.
@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 3, 2021
Comment on lines 34558 to 34559
&& isParenthesizedExpression(location.expression)
&& isAssertionExpression(location.expression.expression);
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

@DanielRosenwasser
Copy link
Member

Will this need to be cherry-picked into 4.2 stable?

Copy link
Contributor

@elibarzilay elibarzilay left a 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...)

@sandersn
Copy link
Member Author

@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.
@DanielRosenwasser I'd have to read up on the history to decide whether it's important enough to put in 4.2.

@sandersn sandersn merged commit f2bcb21 into master Feb 12, 2021
@sandersn sandersn deleted the no-did-you-mean-to-call-error-on-casts branch February 12, 2021 17:37
@sandersn sandersn restored the no-did-you-mean-to-call-error-on-casts branch February 12, 2021 17:38
@sandersn
Copy link
Member Author

OOPS. I merged this without the followup commit. I opened a new PR here: #42779

@DanielRosenwasser
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Feb 12, 2021

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.2 on this PR at 1d34778. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #42780 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Feb 12, 2021
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
DanielRosenwasser pushed a commit that referenced this pull request Feb 16, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NewErrors] 4.2.0-dev.20201122 vs 4.1.2
4 participants