Skip to content
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

Type union not matched starting from TS 5.1 #58603

Closed
WGroenestein opened this issue May 21, 2024 · 5 comments Β· Fixed by #58974
Closed

Type union not matched starting from TS 5.1 #58603

WGroenestein opened this issue May 21, 2024 · 5 comments Β· Fixed by #58974
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@WGroenestein
Copy link

πŸ”Ž Search Terms

"5.1", "type union"

πŸ•— Version & Regression Information

  • This changed between versions 5.0 and 5.1 (based on playground version selection)

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.5.0-beta#code/LAKApgdgrgtgBAWQJ4FFrwN6gJAEE4C8cAjADQ4BChcATKAL6igAuSADmHACrtj5FYQ2ANYBLCABMAXIlToAdLgDcOURID8M9ACMwAJxUh6hlr268qAnGMkzkaWPIqHGIAGRwAFDkHZsazTgoSTAAM3EwCUNsV2wAHx8cf2k4HX1o1wBKExBWDlkefKJCvjg48w5nJhBQ4IBjZlEAewg4AGUmmDBmAAtxAHNPAEM7JBLMmQA3JrU4DFdQDq7egc8MOBsU+wV8emzFzu6+iEH1zdGHGCc4PcMlo9Wz8S25R1xSODUZYIkwiIkbvsQPcVic1htnhcdh8viRAXdDqDThDbLJLk4YSkfn8IJF4Qdlsdkec0QoKJiZMR8SBQLUIA1mq0AJKSUR6MANTwk7awTGBNJ6CZwaazXwgolrJKbchCZI4W4Map0hktOAsiRsjnMGhcyGk3mfDRaWC6QVTGYA3wAZwA7qJmHUel5Npk5kk6kMrZweVdcFJ3Z7va8rhR-bLsOLHkk-NLo3Lwwrw9p2UNhNFsL9QkMoAAbZhhvzYZNgVMZRU0mr1Rqq9WahoAZl1qJ9fONMFNQpFlpwtvtjudz1dvmwHq9+t9Bb8kbBw5jzxlhfjhcTheLpeqflHQfRoej09Ocdj4aXfhXfjXaaSmezecnRZTl6ErnoQA

πŸ’» Code

enum MyEnum {
	A = 1,
	B = 2
}

type TypeA = {
	kind: MyEnum.A;
	id?: number;
};

type TypeB = {
	kind: MyEnum.B;
}
& (
	{
		id?: undefined;
	}
	|
	{
		id: number;
	}
);

type MyType = TypeA | TypeB;

function Something(a: MyType): void {}

Something({ kind: MyEnum.A });
Something({ kind: MyEnum.B });
Something({ kind: MyEnum.A, id: undefined });
Something({ kind: MyEnum.A, id: 1 });
Something({ kind: MyEnum.B, id: undefined });
Something({ kind: MyEnum.B, id: 1 });

function Indirect(kind: MyEnum, id?: number): void {
	Something({
		kind,
		id
	});
}

function Indirect2(kind: MyEnum, id?: number): void {
	switch (kind) {
		case MyEnum.A:
		case MyEnum.B:
			Something({
				kind,
				id
			});
			break;
		default:
			break;
	}
}

function Indirect3(kind: MyEnum, id?: number): void {
	switch (kind) {
		case MyEnum.A:
			Something({
				kind,
				id
			});
			break;

		case MyEnum.B:
			Something({
				kind,
				id
			});
			break;
		default:
			break;
	}
}

πŸ™ Actual behavior

Indirect and Indirect2 report an error, while Indirect3 does not even though the call signature is identical (there is just an extra type check)

Argument of type '{ kind: MyEnum; id: number | undefined; }' is not assignable to parameter of type 'MyType'.
  Types of property 'kind' are incompatible.
    Type 'MyEnum' is not assignable to type 'MyEnum.A'.(2345)

πŸ™‚ Expected behavior

All 3 versions of Indirect behave the same and produce no errors

Additional information about the issue

We are currently on 4.9 and are preparing to migrate to 5.5 once it goes GA.
In our codebase TypeB has some additional required fields when id is a numer, hence the union.

I did observe that a simpler call signature (using only the discriminator) also fails in 4.9 and 5.0

Something({ kind });
@RyanCavanaugh
Copy link
Member

Why isn't TypeB written this way?

type TypeB = {
	kind: MyEnum.B;
} & ({ id?: number | undefined; });

@WGroenestein
Copy link
Author

Why isn't TypeB written this way?

type TypeB = {
	kind: MyEnum.B;
} & ({ id?: number | undefined; });

As mentioned above, in the real world example were we encounter this, the "id: number" type branch can have additional fields. While when not providing an id (or undefined), you should not provide these. E.g.

type TypeB = {
	kind: MyEnum.B;
}
& (
	{
		id?: undefined;
	}
	|
	{
		id: number;
		commentId? number;
	}
);

@fatcerberus
Copy link

You can’t actually prevent a commentId from appearing in the id?: undefined case without exact types - see #12936

@RyanCavanaugh
Copy link
Member

Bisects to #53709

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 6, 2024
@ahejlsberg
Copy link
Member

ahejlsberg commented Jun 22, 2024

Yeah, there's an issue here. This is a simplified repro:

type Foo = { kind: "a" | "b", value: number } | { kind: "a", value: undefined } | { kind: "b", value: undefined };

function test(obj: { kind: "a" | "b", value: number | undefined }) {
	let x1: Foo = obj;  // Ok
	let x2: Foo = { kind: obj.kind, value: obj.value };  // Error, but shouldn't be
}

Above, our type discrimination logic reduces type Foo to just { kind: "a" | "b", value: number } because it is the only constituent of the union to which the type of obj.kind is assignable. This unidirectional assignability check is a little too selective. We should really be checking whether there is any overlap between the discriminant type in the object literal and the discriminant type in the target--in other words, the "a" | "b" type from the object literal shouldn't eliminate the "a" and "b" choices in the target. It's a pretty simple fix. I'll put up a PR.

@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Jun 22, 2024
@ahejlsberg ahejlsberg added this to the TypeScript 5.6.0 milestone Jun 22, 2024
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
5 participants