-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Uncalled function checks don't work with negation #43097
Conversation
The TypeScript team hasn't accepted the linked issue #43096. If you can get it accepted, this PR will have a better chance of being reviewed. |
I prefer to wait on a PR until after a bug is accepted because code goes stale so fast. That said, I believe there are a couple of related PRs that are ready to merge, so maybe it's worth deciding whether to accept the source issue for this too. Edit: #42835 |
|
||
if (!isUndefinedFoo) { | ||
// no error | ||
} |
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.
would be nice to have tests for when those negated expressions are part of larger logical expressions (e.g. !isFoo || x
, etc)
|
||
if (!isUndefinedFoo) { | ||
// no error | ||
} |
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.
would also like to see what happens when we issue this error for unawaited promises, like so:
declare function foo(): Promise<string | undefined>;
const awaitedValue = foo();
if (awaitedValue) { // Emits ts2801
// ...
}
if (!awaitedValue) { // What happens here?
// ...
}
At a first glance, I think we should accept the issue. Before a final decision though, we should user/DT test this to see if we get too many unwanted errors. I'll do that once merge conflicts are fixed. |
@jonhue are you still interested in moving forward with this PR? If so, can you fix the conflicts? |
Hey @gabritto, thanks for following up on this! To be completely honest, it's been quite a while and I have a bunch of other projects at hand right now. I will try to squeeze fixing the merge conflicts into my schedule in the next few days, but to be honest, I'm not too sure i'll manage to. So feel free to take over the pr if you think this change is still worth adding :) |
@gabritto Do you mind if I close this until you have a chance to get back to it? |
no, go ahead |
Fixes #43096