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

Allow non returning functions to have contextually required undefined return type #36288

Closed
5 tasks done
falsandtru opened this issue Jan 18, 2020 · 13 comments · Fixed by #53092 or #53607
Closed
5 tasks done

Allow non returning functions to have contextually required undefined return type #36288

falsandtru opened this issue Jan 18, 2020 · 13 comments · Fixed by #53092 or #53607
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@falsandtru
Copy link
Contributor

falsandtru commented Jan 18, 2020

Search Terms

#36239

Suggestion

Allow non returning functions to have contextually required undefined return type.

Use Cases

  • Callbacks

Examples

declare function f(a: () => undefined): void;
f(() => { }); // error -> ok
f((): undefined => { }); // error -> ok
const g1: () => undefined = () => { }; // error -> ok
const g2 = (): undefined => { }; // error -> ok if possible
function h1() {
}
f(h1); // error -> error
function h2(): undefined { // error -> ok if possible
}

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript labels Jan 23, 2020
@RyanCavanaugh
Copy link
Member

I like this idea 👍

@wffurr
Copy link

wffurr commented Jul 6, 2020

This would be great.

I'm working with an API that's typed in closure compiler's ES4 type system as () => boolean|undefined and the TypeScript functions passed in are required to explicitly return undefined. If I omit the return, they're typed as void and generate the error Type 'void' is not assignable to type 'boolean | undefined'.(2322)

e.g.

type listener = () => boolean | undefined;
function listen(listener: listener) { }

listen(() => true);  // ok
listen(() => undefined); // ok
listen(() => { });  // error!

What needs to happen to allow the listener callback type to omit return statements? Any chance of this landing in a future release?

@wffurr
Copy link

wffurr commented Jul 28, 2020

I have a lot of code that has functions with return types of undefined or T|undefined.

I'd be happy to try to take this on myself, if a project member could give some pointers as to how such a thing could be implemented.

The idea is that any argument accepting a function parameter whose return type includes undefined could accept a non-returning function instead of requiring an explicit return undefined. Explicitly returning undefined is pretty weird when that's the default behavior of a non-returning function in JavaScript.

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Sep 6, 2020

Do note that an explicit return statement without any value causes TypeScript to allow the undefined return type annotation.

const ok = (): undefined => {
	return; // This explicit return statement is currently required
};

@ilogico
Copy link

ilogico commented Sep 8, 2020

Consider ReactJS's useEffect, the first argument must be a function that returns void | (() => void | undefined).
The problem is the following is valid:

const f: () => void = () => 42;
useEffect(f);

IMO, an API which expects a function that returns void means it will not look at the return value, which isn't the case.
In a sense, void as a return value is more like unknown than undefined.

But if the API had been correctly defined as declare function useEffect(f: () => undefined | (() => void)): void;, the following wouldn't typecheck with noImplicitReturns:

useEffect(() => {});

I believe accepting this suggestion would help defining better APIs.

@ljwagerfield
Copy link

ljwagerfield commented May 26, 2021

I'd like to vote against this -- or at least provide it as a tsconfig option (so that we're not forced to use it).

In fact, I would like to propose making returns more explicit than they are today. Currently the compile error goes away providing your method has at least one terminal branch with an explicit return... it would seem safer IMO if the error only goes away once all terminal branches have an explicit return:

I have many T | undefined in my code: if I forget to explicitly return a value from a terminal branch, there's only a 50% chance that the implicitly returned undefined is the correct value to return (meaning a 50% chance of a bug).

Consider bugs like this, which would be prevented if every terminal branch required a return:

function foo(): number | undefined {
  if (someFunction()) {
    return undefined
  }

  const value: number = someOtherFunction()
  anotherFunction(value)
  // return value <- The code I forgot to write! Today this compiles fine... but it shouldn't IMO.
}

So tbh rather than making the returns more implicit, it would be safer to make them more explicit.

(@falsandtru @RyanCavanaugh sorry for the "thumbs downs": your comments are great, I just wanted the maintainers to see at a glance that someone has an issue with the proposed change, as currently it looks unanimously in favour 😄)

@RyanCavanaugh
Copy link
Member

@ljwagerfield it sounds like you should turn on the noImplicitReturns flag, which does exactly that?

@ljwagerfield
Copy link

ljwagerfield commented May 27, 2021

@ljwagerfield it sounds like you should turn on the noImplicitReturns flag, which does exactly that?

Excellent, so it does!

In that case, assuming noImplicitReturns: true works the same after the proposed changes, then I'm happy to withdraw my reservation.

Thanks again @RyanCavanaugh !

@borgesius
Copy link

Very cool I like it this would have saved me two hours of bugfixing earlier 👍

@bombillazo
Copy link

This would be a great feature to incorporate into TS!

@akouryy
Copy link

akouryy commented Jun 16, 2022

It sounds great. In this example, e and h are already typed when noImplicitReturns: false.

function a(): void { return }
function b(): void { if(0) return }
function c(): void {}

function d(): undefined { return }
function e(): undefined { if(0) return } // error when noImplicitReturns: true
function f(): undefined {}               // always error

function g(): number | undefined { return }
function h(): number | undefined { if(0) return } // error when noImplicitReturns: true
function i(): number | undefined {}               // always error

It would be more consistent to allow f and i under noImplicitReturns: false.

This change seems to have no drawbacks. What blocks approval of it?

@stevenwdv
Copy link

stevenwdv commented Oct 18, 2022

I'm all for this! This would allow typing event handlers as (ev: Event) => undefined | boolean instead of the weird (ev: Event) => any :D

Edit: The current implementation still doesn't allow this as it blocks unions with undefined :(

@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.1.0 milestone Mar 17, 2023
@DanielRosenwasser DanielRosenwasser added Fixed A PR has been merged for this issue Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Mar 17, 2023
@DanielRosenwasser
Copy link
Member

Thank you @saschanaz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet