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

Issue unawaited promise error on symbol-less expressions #44491

Merged
merged 3 commits into from
Jun 28, 2021

Conversation

andrewbranch
Copy link
Member

Makes #43593 less conservative. Previously, since we wanted to check whether the body of the conditional references the same thing that’s being tested for truthiness, we had to get a symbol for the if expression and find references to that same symbol. We can only find symbols on identifiers and access expressions, so we failed to come up with a suitable location on call expressions like the one in the blog post:

async function foo(): Promise<boolean> {
    return false;
}

async function bar(): Promise<string> {
    if (foo()) { // <- no error
        return "true";
    }
    return "false";
}

This PR adds the error back in cases like these where we can’t easily tell whether or not the expression is used again in the function body.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 7, 2021
const testedSymbol = getSymbolAtLocation(testedNode);
if (!testedSymbol) {
const testedSymbol = testedNode && getSymbolAtLocation(testedNode);
if (!testedSymbol && !isPromise) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t know that we actually need have divergent behavior for unawaited promises and uncalled expressions here—what do we expect for something like

const or = x => y => x || y;

if (or(true)) { // today: no error, but maybe should be?
  // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Expanding it to uncalled functions seems like an improvement to me. We just need to know that it won't break lots of existing code. Are you up for checking DT and RWC to see?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks good, although you are right that maybe it should apply to uncalled functions too.

const testedSymbol = getSymbolAtLocation(testedNode);
if (!testedSymbol) {
const testedSymbol = testedNode && getSymbolAtLocation(testedNode);
if (!testedSymbol && !isPromise) {
Copy link
Member

Choose a reason for hiding this comment

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

Expanding it to uncalled functions seems like an improvement to me. We just need to know that it won't break lots of existing code. Are you up for checking DT and RWC to see?

@sandersn sandersn added this to Not started in PR Backlog Jun 18, 2021
@sandersn sandersn moved this from Not started to Needs merge in PR Backlog Jun 18, 2021
@andrewbranch
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

Heya @andrewbranch, I've started to run the extended test suite on this PR at 60d5813. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at 60d5813. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 24, 2021

Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at 60d5813. You can monitor the build here.

@andrewbranch
Copy link
Member Author

Test infrastructure seemed to be having an issue, so I’m not going to worry about aligning the call expression behavior for now, which would be a breaking change that we haven’t validated. We can revisit in the future but I’d probably lean toward waiting to hear user feedback.

@andrewbranch andrewbranch merged commit 54b913c into microsoft:main Jun 28, 2021
PR Backlog automation moved this from Needs merge to Done Jun 28, 2021
@andrewbranch andrewbranch deleted the bug/43593 branch June 28, 2021 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants