Repository navigation
Unions with overlapping discriminator property do not perform internal excess property checks #51873
Description
Bug Report
Discriminating unions provide a means for type narrowing based on a discriminator property. The first property common to all union constituents is taken as the discriminator property.
If object literals provide a discriminator value, their expected shape gets narrowed to the corresponding union constituent. Excess properties from other constituents (or completely unrelated properties) raise an excess property check (EPC) error.
At present this only works if there is a single matching candidate constituent for the object literal being tested. If two or more types within the union match the provided discriminator value, EPC accepts properties from the entire union - even though only a subset actually matched.
EPC should continue to work with the subset of matching constituents, not the entire union.
The use-case I have in mind for overlapping discriminators is to specify optional properties that may (only) be present in the object literal for specific discriminator values.
interface Common {
type: "A" | "B" | "C" | "D" | [...many];
n: number;
}
interface A {
type: "A";
a?: number;
}
interface B {
type: "B";
b?: number;
}
type CommonWithOptionals = Common | (Common & A) | (Common & B);Because of the above issue, { type: "a", n: 1, a: 1, b: 1 } does not raise an EPC error, even though it is clearly invalid.
The alternative is to specify something along the lines of (Common & { type: "C" | "D" | [...many]> })(?), or the StrictUnion<> approach, but this is quite cumbersome. It may also be the case that Common is a class rather than an interface, or is otherwise an important type of its own, so subtracting from it may not be suitable.
It is worth noting that if (obj.type === "A") obj.a still throws TS(2339): Property 'a' does not exist on type 'Common | (Common & A)'. While this might seem to defeat the purpose of a discriminating union, the goal is merely to ensure correct EPC. We are still explicitly accepting both types, independently of each other. Type guards and in checks can then be used to access A as intended.
🔎 Search Terms
discriminator
union
overlap
excess property check
Similar but subtly different to #20863.
🕗 Version Information
v5.0-next
v4.9.4
- This is the behavior in every version I tried, and I reviewed the FAQ for entries about unions and discriminators.
⏯ Playground Link
Playground link with relevant code
💻 Code
interface Parent {
type: "foo" | "bar" | "baz";
}
interface Bar {
type: "bar";
bar?: number;
}
interface Baz {
type: "baz";
baz?: number;
}
type Union = Parent | Bar | Baz;
function test(union: Union) {}
test({
type: "foo",
bar: 1 // ✅ TS(2345): Object literal may only specify known properties, and 'bar' does not exist in type 'Parent'.
});
test({
type: "bar",
bar: 1,
baz: 1 // ❌ No error?
});🙁 Actual behavior
EPC error raised for single matching discriminated type (Parent), no error raised for combined matching types (Parent | Bar).
🙂 Expected behavior
EPC error raised in both cases, because { baz: number } should only be present for Baz interface, which would have required {type: "baz" }.
Activity
nebkat commentedon Dec 13, 2022
In addition to my example use-case, there is a further debatable use-case:
Because the discriminator should technically resolve to
(Common & Afoo) | (Common & Abar), EPC should accept{ type: "A", foo: 1, bar: 1 }. This opens up the possibility to seamlessly "extend" the available properties in the union for a particular discriminator without having to re-define all of its constituents.Strictly speaking, it should allow
{ a }or{ b }but not both, sinceCommon & Afoo & Abaris not explicitly listed. The question then becomes should this behavior be explicitly documented, or would it be counterintuitive considering normal union behavior? Currently there is no error raised and I think many developers already rely on such "quasioverlapping unions" behavior.In my application this would offer a very elegant solution as my discriminator values are complex union types themselves, so mixing unions and intersections becomes quite tricky.
Consider all union types matching discriminator for excess property c…
Consider all union types matching discriminator for excess property c…
nebkat commentedon Dec 13, 2022
Potential fix: nebkat@f93cc34
Appears to have minimal side effects, but I'm not sure if original behavior is due to a performance constraint.
Will add further tests if some there is some consensus on the issue.
fatcerberus commentedon Dec 13, 2022
That this is explicitly tested for would seem to suggest the behavior is by design.
MartinJohns commentedon Dec 13, 2022
Can possibly considered a duplicate of #20863, because you effectively don't have a discriminated union.
nebkat commentedon Dec 13, 2022
That is the logic I am questioning in this issue I suppose.
It seems to me more like an unintentional byproduct of the restrictive nature of the discriminator EPC check. If that behavior was desired the union should have included an intersection type allowing all properties.
Alternatively the entire union should be marked ambiguous and there should be no EPC even on unambigious discriminators. We are allowing literals to be assigned to an ambiguous
A | Bthat would not be assignable toAalone orBalone, yet we don't allow this with an unambiguousC.Had me scratching my head for a while because in some cases the union acts as an intersection, in others not. The EPC strikes a good balance effectively treating unions constituents as exact types while allowing overlap where there is uncertainty, it is just falling slightly short in this case.
The key difference and the reason I created a separate issue is that, if I have understood it correctly, it becomes prohibitively expensive to discriminate based on arbitrary combinations of properties, whereas in my examples there is always a single discriminator property.
The function to find the reduced discriminator type is already checking each union constituent for a match to the discriminator property, but it is discarding the matches and returning the entire union if there is more than one match.
My proposed change just returns a new subset union of the multiple matches, which could actually be more efficient, unless there is a significant overhead associated with creating the new subset union and calculating its properties.
fatcerberus commentedon Dec 13, 2022
FWIW, per #20863 (comment):
So this was definitely intentional at one point, but that comment was also made almost 5 years ago. So I’m as interested as you are to see what the maintainers will say about it—and your proposed change—now.
RyanCavanaugh commentedon Dec 13, 2022
I think this is a well-thought-out proposal; the comments in those testcases I believe predate the notion of fishing out a discriminated target type. Can you put up a PR that includes a testcase of the OP and we can evaluate the perf/correctness impact?
Consider all union types matching discriminator for excess property c…
Consider all union types matching discriminator for excess property c…
pedro757 commentedon Feb 11, 2023
I believe this could help to illustrate the problem
Typescript is not smart enough to get the type here.
TS Playground here