Potential Optional Chaining Bug

[MercerK]

TypeScript Version: 3.7.x-dev.201xxxxx

Search Terms:

Optional Chaining
Type bug

Code'

We found a bug within our codebase where a specific instance is returning "undefined" within production code. Upon review, we found this instance and recreated it here. Basically, TypeScript knows that "value" is associated to "string[] | undefined", however, it ignores "undefined" when ".includes" is added onto it for most instances and acts like it is "string[]".

interface Model {
    MyField: FieldModel
}

interface FieldModel {
    value: string[]
}

type ValidateFn<Model, Key extends keyof Model> = (
    value: Partial<Model[Key]>,
    attributes: Partial<Model>
) => void


const validator:ValidateFn<Model, 'MyField'> = (v, attributes) => {
    // value is 'string[] | undefined', yet, shows no error for value/includes.
    attributes.MyField?.value.includes('test')

    const MyField = attributes.MyField
    // value is 'string[] | undefined', yet, shows no error for value/includes.
    MyField?.value.includes('test')

    const value = attributes.MyField?.value
    // value is 'string[] | undefined'. This is the only one that shows an error for value/includes.
    value.includes('test')
}

Expected behavior:

Expected an error associated to "value" in each instance as value is a "string[] | undefined".

Actual behavior:

An error is only returned on the last line. The previous lines are not showing an error. Thus, this would create a false positive that "undefined" is handled to the developer (unless they test or check it manually).

Playground Link:

Playground

Related Issues:

[DanielRosenwasser]

I think this is actually an issue with quick info. None of the intermediate property accesses should display with the type | undefined

[RyanCavanaugh]

It looks like we're incorrectly carrying the optional chain undefined marker through the end of the expression. The subtlety is that

({})?.foo?.bar

evaluates to undefined but

({})?.foo?.bar()

does not throw - but we're evaluating this like foo?.bar.baz() won't throw (it will!)

Simplified

interface Model {
    MyField?: { value: string[] }
}

function fn(attributes: Model) {
    // value is 'string[] | undefined', yet, shows no error for value/includes.
    attributes.MyField?.value.includes('test')

    const MyField = attributes.MyField
    // value is 'string[] | undefined', yet, shows no error for value/includes.
    MyField?.value.includes('test')

    const value = attributes.MyField?.value
    // value is 'string[] | undefined'. This is the only one that shows an error for value/includes.
    value.includes('test')
}

[rbuckton]

I concur with @DanielRosenwasser. if we consider this a Bug, then its a bug in QuickInfo, not the type checker. If you were to write attributes.MyField?.value on its own, the result would correctly be string[] | undefined. However, .includes('test') is part of the OptionalChain, and is therefore included in the branch where MyField is defined (and therefore value has type string[]).

@RyanCavanaugh your example does not illustrate a bug, it illustrates the behavior of optional chaining. Any ., [], or () to the right of ?. is part of the OptionalChain and is not a regular property access.

// this is correct behavior. `?.value.includes('test') is the OptionalChain, and 
// is only evaluated if `MyField` is defined.
attributes.MyField?.value.includes('test')
//                  ^^^^^^^^^^^^^^^^^^^^^^ - only evaluated if `MyField` is defined.

// this is also correct behavior, for the same reason above.
const MyField = attributes.MyField
MyField?.value.includes('test')
//       ^^^^^^^^^^^^^^^^^^^^^^ - only evaluated if `MyField` is defined.

// this is *also* correct behavior. Here `?.value` is the OptionalChain.
const value = attributes.MyField?.value
//                                ^^^^^ - only evaluated if `MyField` is defined.

// here, `.includes('test')` is *not* in an OptionalChain, so it should be an error.
value.includes('test')
//    ^^^^^^^^^^^^^^^^ - Always evaluated.

[RyanCavanaugh]

I got myself confused on this; thanks for the correction.

I don't think there's a better way to display this in QuickInfo - we'd have to somehow differentiate "undefined as the final result" and "undefined as an intermediate optional chain result", which is at best going to be confusing.

[DanielRosenwasser]

we'd have to somehow differentiate "undefined as the final result" and "undefined as an intermediate optional chain result", which is at best going to be confusing.

but if you look at the original post, it's totally broken. The intermediate variables are marked possibly undefined, and the method returns something that isn't undefined. I really think this is a bug.

We could definitely have quick info be more contextually sensitive.

[DanielRosenwasser]

Also, what does C# here? @CyrusNajmabadi