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

apply 'noImplicitReturns' rule for functions that don't have type an… #5733

Merged
merged 2 commits into from
Nov 25, 2015

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Nov 20, 2015

…notations, since user has already opted-in to perform this check. This makes experience mentioned here (##228) slightly better since it will be very cumbersome to ask user to specify type annotations for all callbacks in promises

@@ -9815,13 +9815,19 @@ namespace ts {
// An explicitly typed function whose return type isn't the Void or the Any type
// must have at least one return statement somewhere in its body.
// An exception to this rule is if the function implementation consists of a single 'throw' statement.
// @param returnType - return type of the function, can be undefined if return type is not explicitly specified
Copy link
Member

Choose a reason for hiding this comment

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

This should be switched to JSDoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@DanielRosenwasser
Copy link
Member

Can you add a test for async functions with no return type annotation?

// Functions that return 'void' or 'any' don't need any return expressions.
if (returnType === voidType || isTypeAny(returnType)) {
// Functions with explicitly specified return type which is either 'void' or 'any' don't need any return expressions.
if (returnType && (returnType === voidType || isTypeAny(returnType))) {
Copy link
Member

Choose a reason for hiding this comment

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

What about a Promise<void> in an async function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for async functions returnType is contain promised type so in case of Promise<void> return type will be just void

@DanielRosenwasser
Copy link
Member

I don't know how desirable the behavior is right now. I'm kind of torn on it - on one hand, this is great for people who wrote a void function but just forgot to return something. On the other, this is pretty annoying for people who just want to write a void function with no type annotation.

Would a better approach be to just see if some non-void and non-any type can be inferred for the promised or returned type, and if so, perform the appropriate checking?

@vladima
Copy link
Contributor Author

vladima commented Nov 23, 2015

If this was behavior by default then yes, we definitely should provide a way to opt out of this check. However in our case this is not a default behavior and user has explicitly asked (via compiler option) to do apply this rule. In this case putting another barrier for the rule seems redundant to me.

function checkAllCodePathsInNonVoidFunctionReturnOrThrow(func: FunctionLikeDeclaration, returnType: Type): void {
if (!produceDiagnostics) {
return;
}

// Functions that return 'void' or 'any' don't need any return expressions.
if (returnType === voidType || isTypeAny(returnType)) {
// Functions with with an explicitly specified 'void' or 'any' return type don't need any return expressions.
Copy link
Member

Choose a reason for hiding this comment

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

duplicate 'with with'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mhegazy
Copy link
Contributor

mhegazy commented Nov 25, 2015

👍

vladima added a commit that referenced this pull request Nov 25, 2015
apply 'noImplicitReturns' rule for functions that don't have type an…
@vladima vladima merged commit 937ce71 into master Nov 25, 2015
@vladima vladima deleted the unconditionalNoImplicitReturns branch November 25, 2015 20:25
@alexeagle
Copy link
Contributor

We have started rolling this out inside Google. However, we found a case that isn't caught, which I think it ought to be:

doThing(): string {
  return;
}

Again I wonder whether you have the ability to perform these checks or auto-fix within eg. Azure to know whether the check has the right positive and negative cases.
cc @martine

@alexeagle
Copy link
Contributor

Oops, Evan found the issue for it already: #5916

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants