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

Don't error when function has an implicit return but its return type is assignable to undefined #53490

Merged
merged 10 commits into from
Mar 27, 2023

Conversation

MariaSolOs
Copy link
Contributor

Fixes #53473

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 24, 2023
@MariaSolOs MariaSolOs marked this pull request as ready for review March 24, 2023 20:29
@MariaSolOs MariaSolOs merged commit c5b2884 into microsoft:main Mar 27, 2023
@MariaSolOs MariaSolOs deleted the undef-return-exceptions branch March 27, 2023 20:21
@ahejlsberg
Copy link
Member

I don't think this is the right fix for the issue. The following now doesn't error:

function foo(x: boolean): string | undefined {
}

This previously would report "A function whose declared type is neither 'void' nor 'any' must return a value."

@ahejlsberg
Copy link
Member

The right fix is to put the assignability check back where it was originally.

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 28, 2023

Specifically, we need to undo the change I mention here. And then also undo the change in this PR.

@MariaSolOs
Copy link
Contributor Author

I'm confused with @jakebailey's comment. Is this a fix or a bug? 😄

@ahejlsberg
Copy link
Member

Is this a fix or a bug?

Is what a fix or a bug?

@MariaSolOs
Copy link
Contributor Author

Is what a fix or a bug?

@ahejlsberg Sorry I wasn't clear, I was referring to this PR. The way I read Jake's comment, this change fixes the issue you brought up here, but you're suggesting we undo it.

@ahejlsberg
Copy link
Member

ahejlsberg commented Mar 28, 2023

Yes, I'm suggesting we undo this PR because, while it fixes the issue here, it causes a new issue that I mention here.

The real fix is to revert the deletion of the assignability check I mention here. Once that's done there is no need for this PR.

@MariaSolOs
Copy link
Contributor Author

Got it, thanks for explaining! I can prepare that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
5 participants