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

Narrowing discriminated unions with union discriminant properties #31404

Closed
JakeTunaley opened this issue May 15, 2019 · 4 comments
Closed

Narrowing discriminated unions with union discriminant properties #31404

JakeTunaley opened this issue May 15, 2019 · 4 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@JakeTunaley
Copy link

TypeScript Version: 3.5.0-dev.20190514

Search Terms: undefined discriminated union discriminant discriminator unions type guard control flow narrowing

Code

// --strictNullChecks

interface Option1 {
	x: true;
}

interface Option2 {
	x: false | undefined;
}

type OptionUnion = Option1 | Option2;

function acceptOption1(x: Option1): void {}

function acceptOption2(x: Option2): void {}

function acceptNever(x: never): void {}

function test(anOption: OptionUnion): void {
	if (anOption.x === true) {
		acceptOption1(anOption);
	} else if (anOption.x === false || anOption.x === undefined) {
		acceptOption2(anOption);
	} else {
		acceptNever(anOption);
	}
}

Expected behavior:

No errors.

Actual behavior:

acceptNever(anOption); has an error: Argument of type 'Option2' is not assignable to parameter of type 'never'. ts(2345)

Note that the code works correctly if the discriminator is not a property:

// --strictNullChecks

type Option1 = true;
type Option2 = false | undefined;
type OptionUnion = Option1 | Option2;

function acceptOption1(x: Option1): void {}

function acceptOption2(x: Option2): void {}

function acceptNever(x: never): void {}

function test(anOption: OptionUnion): void {
	if (anOption === true) {
		acceptOption1(anOption);
	} else if (anOption === false || anOption === undefined) {
		acceptOption2(anOption);
	} else {
		acceptNever(anOption); // Not an error - correctly narrows to `never`
	}
}

Playground Link:

http://www.typescriptlang.org/play/index.html#src=%2F%2F%20--strictNullChecks%0D%0A%0D%0Ainterface%20Option1%20%7B%0D%0A%09x%3A%20true%3B%0D%0A%7D%0D%0A%0D%0Ainterface%20Option2%20%7B%0D%0A%09x%3A%20false%20%7C%20undefined%3B%0D%0A%7D%0D%0A%0D%0Atype%20OptionUnion%20%3D%20Option1%20%7C%20Option2%3B%0D%0A%0D%0Afunction%20acceptOption1(x%3A%20Option1)%3A%20void%20%7B%7D%0D%0A%0D%0Afunction%20acceptOption2(x%3A%20Option2)%3A%20void%20%7B%7D%0D%0A%0D%0Afunction%20acceptNever(x%3A%20never)%3A%20void%20%7B%7D%0D%0A%0D%0Afunction%20test(anOption%3A%20OptionUnion)%3A%20void%20%7B%0D%0A%09if%20(anOption.x%20%3D%3D%3D%20true)%20%7B%0D%0A%09%09acceptOption1(anOption)%3B%0D%0A%09%7D%20else%20if%20(anOption.x%20%3D%3D%3D%20false%20%7C%7C%20anOption.x%20%3D%3D%3D%20undefined)%20%7B%0D%0A%09%09acceptOption2(anOption)%3B%0D%0A%09%7D%20else%20%7B%0D%0A%09%09acceptNever(anOption)%3B%0D%0A%09%7D%0D%0A%7D%0D%0A

Related Issues:

#14471 is also using undefined as a discriminant, although the linked issue is restricting itself to talking about mapped types

@jack-williams
Copy link
Collaborator

jack-williams commented May 15, 2019

I think this is a design limitation in discriminant narrowing which is effectively a top-down process, rather than bottom up.

When narrowing for the negation of a discriminant check - so when something like anOption.x === ${THING} is assumed to be false - the checker will try and filter out branches in the declared union (here OptionUnion) that are definitely incompatible at the point of that individual check.

Knowing that anOption.x !== true is sufficient to categorically rule out Option1 from OptionUnion.

Knowing that anOption.x !== false on its own is not sufficient to rule out Option2 from OptionUnion.
Knowing that anOption.x !== undefined on its own is not sufficient to rule out Option2 from OptionUnion.

Only using the composition of the two is it possible to rule out Option2, however the checker will not compose multiple property narrowings when discriminant pruning. So by top-down I mean that it will not collect state from composite narrowings of the same property and use them.

For example, the checker knows that x is never in the else branch because it composes the two checks in the ||, but the composed checks are not used for discriminant narrowing.

function test(anOption: OptionUnion): void {
	if (anOption.x === true) {
		acceptOption1(anOption);
	} else if (anOption.x === false || anOption.x === undefined) {
		acceptOption2(anOption);
	} else {
		anOption.x // x is never
		acceptNever(anOption);
	}
}

Ideally you want to identify that any object that has a property of type never must itself be never. Though, as I've learned the hard way, it is easy to introduce massive performance regressions by changing discriminant narrowing.

There may be some changes that could be done in the checker to support this; someone on the team would need to look into it. Two workarounds for now:

  1. Separate out the union into three parts.
interface Option1 { x: true; }
interface Option2 { x: false; }
interface Option3 { x: undefined; }
type OptionUnion = Option1 | Option2 | Option3;
  1. Use the property itself as proof of never.
acceptNever(anOption.x); // ok

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label May 17, 2019
@j-morone
Copy link

j-morone commented Mar 10, 2020

I have a case that may be more related to #20863 from a technical standpoint, but since it is specific to undefined, I thought I'd add here.

I'd like to have an interface that conditionally has a set of additional optional properties based on the truthiness of an optional property.

interface CommonProps {
  a: number;
  b: string;
  c: boolean
}

interface HighPriorityProps {
  neededBy?: DateTime;
  priorityLevel?: number;
}

interface HighPriorityIssueProps  extends HighPriorityProps{
  isHighPriority: true
}

interface RegularPriorityIssueProps {
  isHighPriority?: false
}

type Props = CommonProps & (HighPriorityIssueProps | RegularPriorityIssueProps)

The use case is for a React component where for ~80% of the use cases I don't want to have to pass in isHighPriority={false}. I'd like to be able to use the optional isHighPriority as a discriminant and allow the additional props iff it is present and truthy.

Instead

<Issue priorityLevel={1}/>

does not throw an error, when I expect it to. I can not use the first workaround, because then I would manually have to still include the optional prop as <Issue isHighPriority={undefined} > instead of excluding it, which is what I want users of the API to be able to do. I cannot use the second as this is in a React component and not a function.

Let me know if I should open a separate issue. Since this is close to many open issues, I didn't want to add another to the list!

EDIT: By the way, I am using the StrictUnion helper from the linked issue to get the behavior I want currently. But I would expect this as default for optional discriminants

@andrewbranch
Copy link
Member

@j-morone that is a separate issue; I filed it at #41759.

@andrewbranch andrewbranch changed the title Using undefined as a property discriminated union doesn't work correctly with control flow narrowing Narrowing discriminated unions with union discriminant properties Oct 12, 2021
@Peeja
Copy link
Contributor

Peeja commented May 24, 2023

@jack-williams I'm not sure I understand why this is a design limitation. The checker does know how to narrow the discriminant property itself. Couldn't the object's type be narrowed at the same time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

6 participants