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

Function return value constraints #228

Closed
knazeri opened this issue Jul 24, 2014 · 21 comments
Closed

Function return value constraints #228

knazeri opened this issue Jul 24, 2014 · 21 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@knazeri
Copy link

knazeri commented Jul 24, 2014

TypeScript compiler allows to define a function marked explicitly with a return type, with code branches which lead to an inconclusive return value!

I know It is valid in JavaScript function not to return a value on all code path, however in TypeScript being a javascript typed superset, I can imagine having a poor written function [without return value on all code path] could potentially break other codes and I'm talking null-references, type mismatch, NaN, ...
I believe when using TypeScript code in an explicitly typed expression, no-one needs all the ugly plumbings, input or type validation, undefined checking, ... One needs to be sure to write a code that couldn't break the way a dynamic language like javascript could!

The following is a valid JavaScript code and it could compile within TypeScript:

function SomeMethod() { }

When you explicitly mention that the method has a return value, the compiler throw an error:

function SomeMethod(): number { }

However it is not the case when the function does not return in all code path, and I don't understand if this is by design why bother preventing above code from compiling in the first place!

The followings are all wrong in a typed context yet the compiler ignores them as a valid code:

function SomeMethod(): number {
    return;
}

function SomeMethod(): number {
    return null;
}

function SomeMethod(): number {
    if (false)
        return 1;
}

function SomeMethod(): number {
    try {
        return 0;
    }
    catch (ex) {
    }
}
@ahejlsberg
Copy link
Member

This is an issue we have discussed a lot. The reasoning behind the current behavior is that a function with an explicitly declared non-void return type and no return statements is clearly suspicious (and therefore should be an error), but otherwise the function might simply be relying on the fact that JavaScript returns undefined when a function doesn't explicitly return a value. We would turn a lot of existing (and technically correct) JavaScript code into errors if we tightened this, so it would definitely have to be an opt-in similar to noImplicitAny.

@knazeri
Copy link
Author

knazeri commented Jul 24, 2014

Thank you for the response Anders.
Having an option to warn on functions with no return value of all code path by the compiler would be great, I can imagine this should be the default behavior of the compiler given that the problem at hand is with the existing js code.

@DanielRosenwasser
Copy link
Member

Also related is #162, which would also be a breaking change if amended.

@RyanCavanaugh
Copy link
Member

Linking this to #274. I'd like to see some set of nailed-down rules. Specifically:

  • Does this apply to contextually-typed functions as well?
  • Do we enforce that "all or none" return statements have expressions?
  • Is this intermixable with a "must return on all codepaths" rule?

@teobugslayer
Copy link

This check is very important when working with promises, because people sometimes forget to return a promise, thus breaking a promise chain. Consider this sample:

var q = require("q");
var d = q.defer();

function f1() { return q("the first value"); }

function f2() {
    process.nextTick(() => { 
        d.resolve(); 
        console.log("2");
    });
    return d.promise;
}

f1()
.finally(function() {
    return f2();
})
.then(x => console.log("end: " + x));
console.log("1");

If you execute it, the result is:

1
2
end: the first value

Now, lets assume, a programmer makes a mistake and forgets to return the result of f2:

.finally(function() {
    f2();
})

The result is

1
end: the first value
2

This mistake is very unfortunate. It is very easy to be made, especially when coding under pressure; is very easy to be missed on code reviews; tslint and friends does not complain; and its asynchronous nature allows it to slip through the manual and automated testing. The code even mostly works in production, except when it mysteriously does not. Considering how important promises and async/await become, I think it is worthy implementing this check behind an opt-in flag.

@nadyaA
Copy link

nadyaA commented Nov 20, 2015

👍 for @teobugslayer 's suggestion

@vladima
Copy link
Contributor

vladima commented Nov 20, 2015

your wish is my command

declare function foo(s: (x: boolean) => number);

foo((x): number => {
    if (x) return 1;
})

compiling with current master bits

v2m@v2m-ThinkPad-W520:~/sources/TypeScript$ node built/local/tsc.js test.ts --noImplicitReturns
test.ts(3,10): error TS7030: Not all code paths return a value.

@teobugslayer
Copy link

@vladima, I assume this will be released in typescript 1.8, is this correct?

@vladima
Copy link
Contributor

vladima commented Nov 20, 2015

Yes

@teobugslayer
Copy link

Great news, thanks you!

@knazeri
Copy link
Author

knazeri commented Nov 20, 2015

@vladima Great!

@myitcv
Copy link

myitcv commented Nov 22, 2015

@vladima does this page need to be updated to reflect this change?

@vladima
Copy link
Contributor

vladima commented Nov 22, 2015

It will be updated before the release

@teobugslayer
Copy link

I've tested with this code

var q = require("q");
async function f3(): Promise<any> {
}

and it was successfully compiled without errors using this command line

node_modules\.bin\tsc index-async.ts --module commonjs --noImplicitReturns --target ES6

I am using typescript by installing npm i typescript@next.

Is this expected?

@vladima
Copy link
Contributor

vladima commented Nov 25, 2015

currently it is by design, any as promised type (and as return type) suppresses noImplicitReturn check

@teobugslayer
Copy link

I see, thanks for the explanation.

@falsandtru
Copy link
Contributor

I am very glad. Great work.

@alexeagle
Copy link
Contributor

Should this issue be closed now?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 3, 2016

thanks @alexeagle. closing.

@mhegazy mhegazy closed this as completed Mar 3, 2016
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Mar 3, 2016
@mhegazy mhegazy modified the milestones: Community, TypeScript 1.8 Mar 3, 2016
@mhegazy mhegazy removed this from the Community milestone Mar 3, 2016
@perlun
Copy link

perlun commented Apr 22, 2017

Coming in "a bit" late to the party, thanks a lot for adding this option. As someone who is slowly but steadily learning TypeScript but with a strong background in the "implicit return" world of Ruby and CoffeeScript, this seems to be a bug I often make: forgetting to type out the return statement, and without this setting, this is valid (but of course malfunctioning!) TypeScript code. Being able to set this setting in tsconfig.json is great since it helps me avoid these kind of silly errors.

So, great work @vladima and the rest of you, thanks a lot.


A question: If I am declaring a method or a lambda type which must return a value, but it can return "any kind of value", is the correct way then to say that the return type is Object? I.e. like this:

function sql(callback: (h: Handle) => Object): Object {
  // ...
}

Or is there some other, more correct way to do it? (As previously alluded, I willingly admit: I am not a Javascript nor TypeScript expert, yet. 😄)

@mbest
Copy link
Member

mbest commented Apr 17, 2018

In the example above, suppose I want to support an undefined return value, would this be the correct approach?

function SomeMethod(): number | undefined {
    if (false)
        return 1;
}

What if I'm creating an interface and want to indicate that a function can return a value of a certain type or not at all (undefined)? Would I have to include void?

SomeMethod: () => number | undefined | void;

@microsoft microsoft locked and limited conversation to collaborators Jul 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests