Union with non-distinct discriminating property breaks inference somewhat (confusing error messages, invalid intellisense)

[nth-commit]

TypeScript Version: 4.0.2

Search Terms: discriminated union, error messages

Code

type Action = {
    type: 'action1';
    payload: {
        property1: string;
    };
} | {
    type: 'action1';
    payload: {
        property2: number;
    };
} | {
    type: 'action2';
    payload: {
        property3: boolean;
    };
}

const action: Action = { // Actual error is here
    type: 'action1',
    payload: {
        property3: true // Expected error here
    }
}

Expected behavior:

Expected error at action.payload.property3 declaration, perhaps something like:

Object literal may only specify known properties, and 'property3' does not exist in type '{ property1: string; } | { property2: number; }'.

Also, once the object has been refined by the type action1, intellisense should only provide property1 and property2 as a suggestion when declaring the payload.

Actual behavior:

An error message at the first line of the declaration:

Type '{ type: "action1"; payload: { property3: true; }; }' is not assignable to type 'Action'.
  Type '{ type: "action1"; payload: { property3: true; }; }' is not assignable to type '{ type: "action2"; payload: { property3: boolean; }; }'.o
      Type '"action1"' is not assignable to type '"action2"'.(2322)

Also, once the object has been refined by the type action1, intellisense still provides property3 as a suggestion when declaring the payload.

Playground Link: https://www.typescriptlang.org/play?#code/C4TwDgpgBAggxsAlgewHZQLxQN4CgoFSiQBcUA5AIYIqoCM5A3PoWJSADbKUAmZehQVDAAnZJBGg6ZAM7ARiVAHNmggL7M1UAD44WBYhDJUaaBqtbsuvfvqGjxESSABMZVAFcAtgCMnFgg1cLV0BQkNjaiQ0FyY7Nk5uPj0hVjEJUABmMh9kZA4ISlQAqCC1XFw4NDkoKNoyeGj0LDCDcCMKOrNyABp4qyTbVIIHDJBsohEPCDtytSA

[nth-commit]

I expect this would all work out fine if somehow TypeScript could reduce:

type Action = {
    type: 'action1';
    payload: {
        property1: string;
    };
} | {
    type: 'action1';
    payload: {
        property2: number;
    };
} | {
    type: 'action2';
    payload: {
        property3: boolean;
    };
}

To:

type Action = {
    type: 'action1';
    payload: {
        property1: string;
    } | {
        property2: number;
    };
} | {
    type: 'action2';
    payload: {
        property3: boolean;
    };
}

Although, I don't know enough to be sure that is a valid reduction all of the time.

[andrewbranch]

The completions bit does seem like a bug, and I 100% agree with your logic about the error message, though I’m not super optimistic that it can easily be generalized. Worth taking a look, though. (cc @DanielRosenwasser for thoughts on error elaboration)

[a-tarasyuk]

I think it is similar to this #39438 issue related to contextual type in a completion list

[DanielRosenwasser]

I have ideas to improve the current error message, but I don't know how we could turn it into an excess property error. For that, I'd have to ask @sandersn for input.

So right now, the message is

Type '{ type: "action1"; payload: { property3: true; }; }' is not assignable to type 'Action'.
  Type '{ type: "action1"; payload: { property3: true; }; }' is not assignable to type '{ type: "action2"; payload: { property3: boolean; }; }'.
    Types of property 'type' are incompatible.
      Type '"action1"' is not assignable to type '"action2"'.

The most obvious problem to me is that we're trying to match something with a type: "action1" to anything apart from the two cases with type: "action1". This is because discriminateTypeByDiscriminableItems throws away its work if there's any other possible type.

            // make sure exactly 1 matches before returning it
            let nextMatch = discriminable.indexOf(/*searchElement*/ true, match + 1);
            while (nextMatch !== -1) {
                if (!isTypeIdenticalTo(target.types[match], target.types[nextMatch])) {
                    return defaultValue;
                }
                nextMatch = discriminable.indexOf(/*searchElement*/ true, nextMatch + 1);
            }

That doesn't really make sense to me - if you have ties, you're still better off going with one of them than none of them. Maybe @weswigham has more input on the intuition here.

---

Apart from that, I think there's another issue with our heuristics where instead of filtering down unions, they just keep trying to find a "best" type and giving up.

        function getBestMatchingType(source: Type, target: UnionOrIntersectionType, isRelatedTo = compareTypesAssignable) {
            return findMatchingDiscriminantType(source, target, isRelatedTo, /*skipPartial*/ true) ||
                findMatchingTypeReferenceOrTypeAliasReference(source, target) ||
                findBestTypeForObjectLiteral(source, target) ||
                findBestTypeForInvokable(source, target) ||
                findMostOverlappyType(source, target);

That's a little harder to re-orchestrate in a simple way, but it wouldn't be the craziest.

[andrewbranch]

Removing my assignment and milestone because @a-tarasyuk rightly pointed out that the completions part of this report is an exact duplicate of #39438.

[weswigham]

That doesn't really make sense to me - if you have ties, you're still better off going with one of them than none of them. Maybe @weswigham has more input on the intuition here.

Just historical precedent. When we discriminate, we choose to do so to a single item, and not a category of items. Discriminating to a set of applicable elements rather than a single one, in theory, I think would be fine, so long as we were actually set up to handle it everywhere.

[sandersn]

This is because discriminateTypeByDiscriminableItems throws away its work if there's any other possible type.

I think this is because it indicates that the discriminable item might be technically discriminable but not intended as such. Certainly Action is a suspicious discriminated union -- just looking at the OP example with no context, I'd hope that a linter would put a warning on the second type: 'action1'. Something like "did you mean to use a different literal type here, or perhaps combine it with the previous case?"

In other words, I think the specific example isn't a convincing reason to change the single-matching-type rule since it's a possibly-incorrect discriminated union. I'd hope for an union that legitimately needs to have a discriminant property with two or more identical types.

[DanielRosenwasser]

After looking again, I actually believe this is because of contextual typing, where signatures can't be contextually typed by a union. Seems like the easiest thing to do is add a flag for this specific use-case.