Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Allow non-unit types in union discriminants#27695

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ahejlsberg merged 5 commits intomasterfrommixedDiscriminantTypes
Oct 15, 2018

Conversation

@ahejlsberg
Copy link
Member

With this PR we permit non-unit types in union type discriminants. Specifically, we now consider a property of a union type to be a discriminant property if it has a union type containing at least one unit type and no instantiable types. For example:

typeResult<T>={error?:undefined,value:T}|{error:Error};functiontest(x:Result<number>){if(!x.error){x.value;// number}else{x.error.message;// string}}test({value:10});test({error:newError("boom")});

Inspired by#27631 which was closed in favor of this PR.

Fixes#24193.

jack-williams, alexrock, yortus, j-oliveras, sean-vieira, dderevjanik, ulrichb, yanchith, KSXGitHub, ondrowan, and 23 more reacted with thumbs up emojicshaa and sergey-shandar reacted with heart emoji
@jack-williams
Copy link
Collaborator

I mentioned it in the old PR, but I think it is worth recording in the PR that gets merged:

This change will have the side-effect of enabling excess property checking for certain union types that were previous not subject to the checks. Unions types are only checked if they have a discriminant property, and this change distinguishes more discriminant types.Technically this is a breaking change.

x.b; // Error
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Could it be worth adding a test that includes constrained generics? My reasoning being that in the future itmight be worth considering discriminants that have constrained parameters because they are comparable to literals in some form. Having a test that would flag any changes might be helpful. Something like:

functionf21<Textendsnumber>(x:{a:undefined;b:{x:number}}|{a:T,b:{y:number}}){if(x.a===undefined){x.b={y:42};}}

@weswigham
Copy link
Member

functionf(x:{kind:true,a:string}|{kind:"b"}|{kind:string,c:string}){if(x.kind===true){x.a;}elseif(x.kind!=="b"){x.c;// Should be `{ kind: string, c: string }`}else{x;// should be `{ kind: "b" } | { kind: string, c: string }`}}

doesn't seem to work yet with this PR.

@jack-williams
Copy link
Collaborator

jack-williams commentedOct 12, 2018
edited
Loading

@weswigham
EDIT: Correction, the reduction is irrelevant in this case. The problem is that the literal is comparable to the string.

Think the issue with your example is that literal reduction loses type"b", so when it comes to narrowing by inequality it does nothing. What would be the impact of disabling literal reduction when getting the property of a union type?

@ahejlsberg
Copy link
MemberAuthor

@jack-williams You're actually right about typestring absorbing type"b" in a union type. All of the narrowing logic in the control flow analyzer uses union type operations so in those operations we really only can discriminate ontrue | string. Wedo have the ability to construct non-reduced union types such as"b" | string, but that capability is used only to make contextual types a bit more precise and isn't actually observable in types that the user might declare or operate on.

I think it is fine to document that discriminants must be truly disjoint types, and thatstring andnumber can't be used to define "default" cases. If we really wanted to support such a default pattern in a meaningful way, we'd need to verify that the specialized cases are subtypes of their corresponding default cases, and we'd need to narrow to the specialized cases consistently. But that's beyond the scope of this PR and I don't even think we want to go there.

c69 reacted with thumbs up emoji

@jack-williams
Copy link
Collaborator

@ahejlsberg Thanks for the explanation. I managed to confuse myself when I experimented with turning off union reduction and finding that it did not have an effect in this example.

But that's beyond the scope of this PR and I don't even think we want to go there.

Yes, I agree with your assessment here. The asymmetry brought about by trying to narrow types like"b" | string that are influenced by inequality, but not equality, leads to code that can be quite subtle.

@weswigham
Copy link
Member

Fair enough. I just wanted to point out that we still don't narrow all the things we look like we could because of how the broader string type and the specific literal type are related.

Copy link
Member

@weswighamweswigham left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

In any case, this definitely looks like an improvement, and we can always adjust it to recognize more types as discriminable in the future if need be.

@bmingles
Copy link

Any chance this will allow using tuples as discriminants?

e.g.

type RouteA = {  tokens: ['a', string]};type RouteB = {  tokens: ['b', string, string]};type Route =  | RouteA  | RouteB;const route: Route = ...if(route.tokens[0] === 'a') {   // compiler knows this is RouteA}

@ksaldana1
Copy link

@bmingles I believe your example will narrow correctly since v3.1.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@weswighamweswighamweswigham approved these changes

@DanielRosenwasserDanielRosenwasserAwaiting requested review from DanielRosenwasser

@RyanCavanaughRyanCavanaughAwaiting requested review from RyanCavanaugh

+1 more reviewer

@jack-williamsjack-williamsjack-williams left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ahejlsbergahejlsberg

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow to use null as discriminator for tagged union

6 participants

@ahejlsberg@jack-williams@weswigham@bmingles@ksaldana1

[8]ページ先頭

©2009-2025 Movatter.jp