Skip to content

Default values in combination with object destructuring are not type checked within function declarations. #19127

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

Closed
bverbist opened this issue Oct 12, 2017 · 4 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@bverbist
Copy link

bverbist commented Oct 12, 2017

TypeScript Version: 2.5.3

Code

// ===== string literal default value =====

// TS does not give an error for the default value 'z'
function defaultUsingStringLiteralWithinObject({someInput = 'z'}: {someInput?: 'x' | 'y'}) {
    console.log(someInput)
}

// but when calling the function this correctly gives TS2345 (Type 'z' is not assignable to type 'x' | 'y')
defaultUsingStringLiteralWithinObject({someInput: 'z'})

// p.s. TS correctly gives TS2322 when invalid default without object destructuring
function defaultUsingStringLiteral(someInput?: 'x' | 'y' = 'z') {
    console.log(someInput)
}

// ===== enum default value =====

enum IOtherInput {
    x = 'x',
    y = 'y'
}

// TS does not give an error for the default value 'z'
function defaultUsingEnumWithinObject({otherInput = 'z'}: {otherInput?: IOtherInput}) {
    console.log(otherInput)
}

// but when calling the function this correctly gives TS2345 (Type 'z' is not assignable to type 'IOtherInput')
defaultUsingEnumWithinObject({otherInput: 'z'})

// p.s. TS correctly gives TS2322 when invalid default without object destructuring
function defaultUsingEnum(otherInput?: IOtherInput = 'z') {
    console.log(otherInput)
}

Expected behavior:
Default values - in combination with object destructuring - should also be type checked in the function declaration.

Actual behavior:
Default values - in combination with object destructuring - are not type checked in the function declaration. Noticed this behaviour for both string literals and enums.

@ahejlsberg
Copy link
Member

Your example is actually working as intended. In a construct such as

function foo({ someInput = 'z' }: { someInput?: 'x' | 'y' }) {
    // Type of someInput is 'x' | 'y' | 'z'
}

the type of someInput within the function is inferred to be 'x' | 'y' | 'z', i.e. a union of the destructuring source type and the default value type. However, the type of someInput required when you're calling the function is 'x' | 'y' | undefined. Effectively, this allows you to use a default value that isn't in the domain of the values you're allowed to pass to the function.

@ahejlsberg ahejlsberg added the Working as Intended The behavior described is the intended behavior; this is not a bug label Oct 12, 2017
@bverbist
Copy link
Author

Thanks for the quick response!

Shouldn't the same inferred behaviour then also apply to the examples without destructuring? e.g.

type TSomeInput = 'x' | 'y'

function defaultUsingStringLiteral(someInput: TSomeInput = 'z') {
    // whatever
}

But this now gives a TS2322 : 'z' is not assignable to type 'TSomeInput'. Doesn't feel consistent.

Personally I don't immediately see a strong use case for "allows you to use a default value that isn't in the domain of the values you're allowed to pass to the function". There's functionally no difference if the user would pass the default value or not pass the variable at all.
Instead this inferred additional-allowed-value could cause unwanted situations where an old 'wrong' default value (that wasn't refactored because of no type warning and missing unit test) is still being used.

@ahejlsberg
Copy link
Member

A scenario motivating the design is described in #10577. Arguably, in your example the type annotation is perhaps a stronger indication of intent, but I also think it is entirely reasonable for the default value to contribute to the eventual type. Either way, we can't break the scenario in #10577, so any change would have to accommodate both that and your example. Not sure if that is possible.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 27, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Oct 27, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants