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

Compiler cannot handle promise.then(null | undefined) #58619

Open
blackdyedsheep opened this issue May 22, 2024 · 8 comments
Open

Compiler cannot handle promise.then(null | undefined) #58619

blackdyedsheep opened this issue May 22, 2024 · 8 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@blackdyedsheep
Copy link

🔎 Search Terms

"promise", "then", "null", "undefined", "nullable"

🕗 Version & Regression Information

  • This changed between versions 3.33.3333 and 3.5.1

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.4.5#code/PTAEBkFMBdQQ1ABwE4HsC2BLAzpU0ALOWAd0wBtzRlJtVyA3PaVUABgG4AoAY1QDtssRKAC8oAAposuAHQ06jSAAo2ASg6guXEKAAqBHKCN9kNHtAA0oPukQVIyY-wBmj7JOk5IAHn4BXdAAjRwA Lhd-fgtMASQ2ZTVQAG8uUHTqGH9kfiRuAF8dMAMjIzhyOhtUM0gLa1t7ckdQJmRMF0xaTwxvP0CQ5FCq-mhkOAsIqJi4xABGRIAubplfAOCwlLSMmmhs3MQCov1DDzKK1lNzaCWGhydIV2qeLqke3B8hNv4AcyG EbGFi0uj0AE9EHgAORrAaQ4wefioWBwbDYTDffhwIJNfCsaDgqGfTA-SGySbRaCxfYAJkWy16RJ Q1SGUyuxyeS4hRBJ1A2AIqH85AAJqBHGgnEF-LBbk0nNg4KDTrAjB1 JBydN9gBmOmvFYfUbE36bVk7PZIWSEB7KAKUDRco4ASRscFyQRRmB45XIoJs5DgmHQ CIsEIqqmlLi5pyHjdoPDP01Uf2ABY9V53gB1YnC1AkUAAH1AABFiHhiwBVABK4CLKXg-AThh SwARDQfaC26B8sytukY-srQQbXbyA7ucVefzBSL4OcxWZqqApTKMI1mgqlcYVR41RrIhSqUgAKwZt6 RkmlnbLIcxAjm1RYWQA-CydAA

💻 Code

// Your code here

🙁 Actual behavior

// Let a promise that will resolve to 0;
const p = Promise.resolve(0); 

// This should error but compiler says it is fine
function p3(): Promise<string> {
    return p.then(null);
}
// I can basically claim that this function returns anything
function p4(): Promise<Window | Date | URL | { anything: "really" }> {
    return p.then(null);
}
// This should also error but compiler says it is fine
function p5(): Promise<string> {
    return p.then(undefined);
}

🙂 Expected behavior

// Let a promise that will resolve to 0;
const p = Promise.resolve(0); 

// This should error with: 
// Type 'number' is not assignable to type 'string'.
function p3(): Promise<string> {
    return p.then(null);
}
// This should error with:
// Type 'number' is not assignable to type 'string'.
function p5(): Promise<string> {
    return p.then(undefined);
}

Additional information about the issue

@blackdyedsheep blackdyedsheep changed the title Compiler cannot handle with promise.then(null | undefined) Compiler cannot handle promise.then(null | undefined) May 22, 2024
@DanielRosenwasser
Copy link
Member

This is the correct behavior - at runtime, .then(null) creates a new Promise with the original underlying value propagated out.

var a = Promise.resolve("hello world").then(null).then(x => console.log(x))

// prints 'hello world'

While you might prefer to never allow null | undefined to be passed into .then(), it's both allowed by the spec and permits patterns that are arguably useful (such as optionally-present handlers to transform values).

@DanielRosenwasser DanielRosenwasser added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 24, 2024
@andrewbranch
Copy link
Member

andrewbranch commented May 24, 2024

I don’t think that’s the issue that’s being demonstrated here. Minimal repro:

const pNumber = Promise.resolve(0);
const pString: Promise<string> = pNumber.then(); // No error

The issue is that without a callback argument to then, the only inference source for its type parameter is the contextual type (in my type annotation here, in the function return types in the repro from @blackdyedsheep).

@DanielRosenwasser
Copy link
Member

Whoops, I misread. Apologies.

@DanielRosenwasser DanielRosenwasser removed the Working as Intended The behavior described is the intended behavior; this is not a bug label May 24, 2024
@andrewbranch
Copy link
Member

The easy fix I thought of was to add an overload for optional undefined/null, but I’m not sure how well that stacks up against the easiest fix, “just don’t write .then(null).” Is there a real use case for writing code like that?

@blackdyedsheep
Copy link
Author

blackdyedsheep commented May 27, 2024

Yes, there is a real use case, that's how I came across this behaviour.

Quoting @DanielRosenwasser :

optionally-present handlers to transform values

ie this code should infer Promise<0 | string> but infers Promise<string>

function maybeTransform(fn?: (v: number) => string) {
	return Promise.resolve(0).then(fn);
}

and this code should not compile

function maybeTransform(fn?: (v: number) => string): Promise<string> {
	return Promise.resolve(0).then(fn);
}

Basically it should behave the same way as

function maybeTransform(fn?: (v: number) => string) {
	return fn ? fn(0) : 0;
}

@andrewbranch
Copy link
Member

I think this can be solved with overloads, but because overload resolution doesn’t happen during inference, I think it’s going to be incredibly breaky. #58678 is up to experiment, but I don’t think it will be viable.

@jcalz
Copy link
Contributor

jcalz commented Jun 9, 2024

Probably nobody calls Promise.resolve(0).then<string>() either except when they do. I'd say it's the same basic issue whether the type is inferred contextually from an unexpected place or the type argument is specified manually: generic defaults don't exactly match the use case, which looks like #56315. The use case in question is something like "when the optional thing is omitted by the caller, it is effectively supplied by the implementer". Generic defaults sort of do this, except when they don't.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 17, 2024
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 5.6.0 milestone Jun 17, 2024
@rbuckton
Copy link
Member

I think this can be solved with overloads, but because overload resolution doesn’t happen during inference, I think it’s going to be incredibly breaky. #58678 is up to experiment, but I don’t think it will be viable.

interface Promise<T> {
  // specific overloads
  then<TResult1, TResult2>(
    onfulfilled: (value: T) => TResult1 | PromiseLike<TResult1>,
    onrejected: (reason: any) => TResult2 | PromiseLike<TResult2>
  ): Promise<TResult1 | TResult2>;
  then<TResult>(onfulfilled: null | undefined, onrejected: (reason: any) => TResult | PromiseLike<TResult>): Promise<T | TResult>;
  then<TResult>(onfulfilled: (value: T) => TResult | PromiseLike<TResult>, onrejected?: null | undefined): Promise<TResult>;
  then(onfulfilled?: null | undefined, onrejected?: null | undefined): Promise<T>;

  // catch all overload
  then<TResult1 = T, TResult2 = never>(
    onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | undefined | null,
    onrejected?: ((reason: any) => TResult2 | PromiseLike<TResult2>) | undefined | null
  ): Promise<TResult1 | TResult2>;
}

This is fairly accurate and works as expected, for the most part. The specific overloads are all mutually exclusive with each other and are designed to avoid introducing a type variable in the return type of then() that might not be otherwise witnessed when a callback is null, undefined, or not supplied. The catch-all overload allows for various combinations of the specific overloads in cases where you might pass a variable of type (() => T) | null | undefined as it would not match any of the specific overloads despite being perfectly legal.

Unfortunately, it causes #36307 to regress, so it's still not completely reliable.

Ideally, we could solve this during inference by relying on some heuristic related to the fact that TResult1 has a default and no other inferences aside from the return type. Unfortunately, I haven't yet found a reliable heuristic that doesn't break some other expectation somewhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants