Double errors reported with control flow based checking in a loop

[ivogabe]

In a loop, multiple errors are sometimes reported on the same location, but with slightly different types.

@ahejlsberg Any idea what's going on?

TypeScript Version:

nightly (1.9.0-dev.20160430)

Code

function test<U>(xs: U[], combine: (a: U, b: U) => U) {
    let hasValue = false;
    let value: U | undefined;
    for (const x of xs) {
        if (hasValue) {
            value = combine(value, x);
        } else {
            hasValue = true;
            value = x;
        }
    }
}

Expected behavior:
Only one error should be reported:

value = combine(value, x);
                ~~~~~ Argument of type 'U | undefined' is not assignable to parameter of type 'U'.
                         Type 'undefined' is not assignable to type 'U'.

By adding an exclamation mark to that line (value = combine(value!, x);), no error should be reported.

Actual behavior:
Two errors are reported:

value = combine(value, x);
                ~~~~~ Argument of type 'U | undefined' is not assignable to parameter of type 'U'.
                         Type 'undefined' is not assignable to type 'U'.
                ~~~~~ Argument of type 'undefined' is not assignable to parameter of type 'U'.

Adding an exclamation mark gives:

value = combine(value!, x);
                ~~~~~~ Argument of type 'nothing' is not assignable to parameter of type 'U'.

[ahejlsberg]

The assignments in the loop body are analyzed twice because of the back branch. Both times we have to evaluate the combine call to find the type of the assignment (it is beyond the capabilities of the analysis to deduce that combine isn't called the first time). The first time through the type of the value argument is undefined. The second time through it is U | undefined. Errors are reported from both passes, so without the ! operator the two errors are actually expected.

Now, the problem we run into here is that when we apply the ! operator to the type undefined in the first pass we end up with the type nothing. That doesn't seem quite right. I think we have two options we can consider here:

1. In the operation x!, if the narrowed type of x is undefined, null, or null | undefined, revert to the declared type of x before removing undefined and null.
2. In the operation x!, always return the declared type of x with undefined and null removed, regardless of any preceding type guards and assignments.

Not quite sure which is better. (1) better preserves facts we have already deduced, but it seems a bit arbitrary. (2) is more predictable but possibly irritating with more complex types such as string | number | undefined because it would "forget" any preceding operations that removed string or number.

[ahejlsberg]

@mhegazy Opinions?

[ahejlsberg]

BTW, to avoid the error you'd currently have to write:

function test<U>(xs: U[], combine: (a: U, b: U) => U) {
    let hasValue = false;
    let value = <U | undefined>undefined;  // Force initial type to be U | undefined
    for (const x of xs) {
        if (hasValue) {
            value = combine(value!, x);
        } else {
            hasValue = true;
            value = x;
        }
    }
}

Or, alternatively:

function test<U>(xs: U[], combine: (a: U, b: U) => U) {
    let hasValue = false;
    let value = <U><any>undefined;  // Declared and initial are both U
    for (const x of xs) {
        if (hasValue) {
            value = combine(value, x);  // Don't need !
        } else {
            hasValue = true;
            value = x;
        }
    }
}

Neither of these are great, but they illustrate the issue.

[ivogabe]

Errors are reported from both passes, so without the ! operator the two errors are actually expected.

Wouldn't it be better to only show errors from the last pass? I think it can be very confusing to get multiple errors on the same location, with different types mentioned in those errors.

[mhegazy]

Option 2 seems too heavy handed. I would think that most o the time narrowing works. it is these cases where we narrow to nothing for uninitialized variables, and for these reverting to the declared type when we hit nothing seems like a good solution.

[ahejlsberg]

With #8429 you can now write your example as:

function test<U>(xs: U[], combine: (a: U, b: U) => U) {
    let hasValue = false;
    let value: U | undefined;
    for (const x of xs) {
        if (hasValue) {
            value = combine(value!, x);  // No error here
        } else {
            hasValue = true;
            value = x;
        }
    }
}

The double errors also go away, but that's mostly a side effect of the declared and initial types being the same.

[mhegazy]

I believe this should not be an issue now with #8429. please reopen if not the case.

[ivogabe]

It did solve my simplified example, but when I tested in on the real code it still fails 😒. So here is the full code that I use:

export interface Storage<TKey, TValue> {
    createStore(values: Map<TKey, TValue>, parents?: Store<TKey, TValue>[]): Store<TKey, TValue>;
    get(store: Store<TKey, TValue>, key: TKey): TValue;
}
export interface Store<TKey, TValue> {
    values: Map<TKey, TValue>;
    parents: Store<TKey, TValue>[];
}
export function createStorage<UKey, UValue>(defaultValue: (key: UKey) => UValue, union: (a: UValue, b: UValue) => UValue): Storage<UKey, UValue> {
    type UStore = Store<UKey, UValue>;

    return { createStore, get };

    function createStore(values: Map<UKey, UValue>, parents: Store<UKey, UValue>[] = []): UStore {
        return { values, parents };
    }
    function get(store: UStore, key: UKey) {
        const stack = [store];
        let hasValue = false;
        let currentValue: UValue | undefined = undefined;
        while (stack.length !== 0) {
            const s = stack.pop()!;
            if (s.values.has(key)) {
                const value = s.values.get(key)!;
                if (hasValue) {
                    currentValue = union(currentValue!, value);
                } else {
                    hasValue = true;
                    currentValue = value;
                }
            } else {
                stack.push(...s.parents);
            }
        }
        if (!hasValue) {
            currentValue = defaultValue(key);
        }
        store.values.set(key, currentValue);
        return currentValue!;
    }
}

This shows an error on the line that calls union, saying Argument of type 'nothing' is not assignable to parameter of type 'UValue'. Tested with 1.9.0-dev.20160504.

[mhegazy]

the issue is let currentValue: UValue | undefined = undefined;. if you change this to let currentValue: UValue | undefined it should work as expected.

the issue here, is let currentValue: UValue | undefined = undefined; behaves as let currentValue: UValue | undefined; currentValue = undefined; which means that currentValue's type is undefined, then narrowing this further results in nothing, once you get to nothing bang does not help.

[ivogabe]

Thanks, I got it working now. Still I think it's counterintuitive. let currentValue: UValue | undefined; works, but let currentValue: UValue; and let currentValue: UValue | undefined = undefined; not.

This reminds me of the idea to separate type resolving from error reporting. It was mentioned in the control flow checking PR, because getting the type of <string> expression now checks expression too, which can break narrowing with control flow analysis sometimes. Currently expression needs to be checked, because errors need to be reported there. If resolving and error reporting would be separated, this wouldn't be necessary.

For this issue, that would mean that the value is resolved to UValue | nothing, which is reduced to UValue. Do you think of this idea?