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

Optional parameters instantiated from generic params add | undefined to the type #58578

Open
ssalbdivad opened this issue May 19, 2024 · 11 comments

Comments

@ssalbdivad
Copy link

πŸ”Ž Search Terms

generic function optional undefined ? modifier return infer inferrence

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

https://tsplay.dev/w199kw

πŸ’» Code

// Since 4.7, TS doesn't innately add `|undefined` to optional params the way it does for props with EOPT off

type Expected = (arg?: number) => void
//   ^? (arg?: number) => void


// But it adds it when instantiating a type from a generic param

type OptionalUnary<t> = (arg?: t) => void

type Fail = OptionalUnary<number>
//    ^? (arg?: number | undefined) => void

declare const foo: <t>(arg?: t) => void  

const instantiated = foo<number>
//    ^? (arg?: number | undefined) => void

declare const bar: <t>(arg: t) => (arg?: t) => void;

const returned = bar(42);
//    ^? (arg?: number | undefined) => void

πŸ™ Actual behavior

(see example)

πŸ™‚ Expected behavior

(see example)

Additional information about the issue

I don't think this is likely to cause type-safety issues. It's primarily just visual clutter and inconsistency with the standard behavior.

@fatcerberus
Copy link

I don't think this is likely to cause type-safety issues.

Yeah, it's purely a display issue. It's legal to pass undefined for an optional parameter either way:

declare const fn: Expected;
fn(undefined);  // fine, no error

@ssalbdivad
Copy link
Author

ssalbdivad commented May 19, 2024

@fatcerberus Right, in terms of the current behavior it has no effect on type safety.

Theoretically though, there are some niche cases where it could lead to semantically different results were it treated the same way as EOPT, particularly related to varargs.

EDIT: Actually I take that back, there are some niche cases where it could cause incorrect inference if the function ends up being used to infer out params like this:

type Arg = ((arg?: number) => void) extends (arg?: infer t) => void ? t : never
//   ^? number

@fatcerberus
Copy link

It looks like inference, at least, is consistent in that case:
Playground link

type ExpectedArg = Expected extends (arg?: infer t) => void ? t : never
//   ^? type ExpectedArg = number
type FailArg = Fail extends (arg?: infer t) => void ? t : never
//   ^? type FailArg = number

@ssalbdivad
Copy link
Author

@fatcerberus Interesting you could use this as a workaround potentially.

That said, it is still a (small) type-safety issue due to the inconsistency if you extract our your parameters like that.

@fatcerberus
Copy link

I believe the type system treats (p?: number) => void equivalently to (p?: number | undefined) => void in all cases - even if you write out the latter explicitly.

@ssalbdivad
Copy link
Author

@fatcerberus Ahh you're right, I see what you mean now.

I'm not a big fan of that either though. If a type has some special casing that results in it being treated equivalently to another type externally, it should always be displayed in the normalized form IMO. It's very confusing to try and infer in a position that's 1:1 with a summary you're looking at and get a different result.

What I meant about EOPT was that if you consider a situation like this:

type NumericOptionalUnary = (arg?: number) => void

const f: NumericOptionalUnary = (...args) => {}

args is inferred as [args?: number | undefined] even with EOPT on. Is it likely to make a huge difference as is? No, but if I have [args?: number] with EOPT, which is equivalent to [number] | [], I might expect to be able to discriminate on length, but suddenly there is a meaningful semantic difference between passing undefined and passing nothing.

I actually have to make this distinction for inline snapshots in attest:

image

Explicit undefined should really not be allowed here, and I'd prefer to be able to write an API that could enforce that.

@fatcerberus
Copy link

fatcerberus commented May 20, 2024

Yeah, optional params are weird, even in JS:

function foo(x = 42) {
    console.log(x);
}
foo(undefined);

You might expect that to print undefined, but it actually prints 42! Which of course is the opposite of how it works with e.g. default values in destructurings, spreads, etc. So I guess TS was attempting to model those semantics in the type system, which is why it doesn’t behave like EOPT does for properties.

@ahrjarrett
Copy link

Not sure if this is helpful or not, but this is a pattern I've been using to tell the difference between a user explicitly passing undefined and the argument being unprovided:

https://tsplay.dev/wE00ZW

@ssalbdivad
Copy link
Author

@fatcerberus Wow thanks, I didn't know that. At least it's still better than how Python handles default arg values πŸ˜†

@ahrjarrett Based on what @fatcerberus mentioned, I don't think that works:

image

You probably are stuck using spread args and checking length.

@ahrjarrett
Copy link

Not sure I follow -- the behavior in your screenshot is the behavior I'd expect @ssalbdivad. Are you saying you want the user to be able to pass undefined?

You can also use an overload for each valid set of arguments the user can provide, and spread the args in the implementation, checking (like you said) for argument length to determine which overload you're handling.

Type inference works great, it's more explicit for the caller, and you still get good type inference in the body of the function.

https://tsplay.dev/mA09RN

@ssalbdivad
Copy link
Author

@ahrjarrett Maybe I misunderstood, but you mentioned

tell the difference between a user explicitly passing undefined and the argument being unprovided

In this example you gave, you can't differentiate as written, no?

That's why I mentioned you'd have to use a variadic parameter and check the length like I do when handling inline snapshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants