Skip to content

Retain literal types in some contextual unions #19322

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

Conversation

weswigham
Copy link
Member

Alternative to #18488. Rather than attempt to find a discriminant from the result of a contextual typing pass, this simply retains literal types in unions manufactured during contextual typing of element properties, as the information is still relevant to us there (since we use it for things other than relationship checking - like determining if literal types should be inferred).

Fixes #16457. Handles both cases listed there.

@sandersn I happened to chance upon this while looking at adding more test cases for #17668, before I realized I was about to include a driveby fix for something that there was already a different issue open for. 🐱

@weswigham weswigham requested a review from sandersn October 19, 2017 03:09
@weswigham weswigham changed the title Retain literal types in contextual unions Retain literal types in some contextual unions Oct 19, 2017
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Seems ... OK, if somewhat ad-hoc. @ahejlsberg should take a look at any changes to getUnionType too.

@@ -0,0 +1,53 @@
interface X {
type: 'x';
value: string;
Copy link
Member

Choose a reason for hiding this comment

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

can you add a method that makes this of the object literal show up in the type baselines? I want to make sure it's not just value: 'done', but value: 'none' | 'done' for the type: 'y' case.

@sandersn sandersn requested a review from ahejlsberg October 19, 2017 17:18
@weswigham
Copy link
Member Author

weswigham commented Oct 19, 2017

@sandersn I've added to the test, and added some code to discriminate a contextual this type; the this types within object literals typed by these unions looks much nicer now.

@ahejlsberg I think flagging getUnionType to not remove literal types is probably correct in this specific case - we don't want to lose the information those types provide (namely their literalness).

@weswigham
Copy link
Member Author

weswigham commented Oct 28, 2017

@ahejlsberg I've implemented the alternative we just spoke about in person (domain-based literal contextual types and the requisite bidirectional comparable relationship to keep casts functional) on this branch; However there is some fallout that I'm not sold on, for example:

class C<T extends { length: number }> {
        constructor(x: T) { }
        foo<U extends T>(x: U) {
            function bar<V extends U>(x: V) {
                return x;
            }
            return bar;
        }
    }
    
    var c = new C({ length: 2 }); // 2 is now a literal type
    var r = c.foo({ length: 3, charAt: (x: number) => { '' } }); // Making this an error, since `length` is the literal `2`.

While correct in the substitutability of T sense, I'm not sure it matches intent. Is this desirable? Does it warrant re-special-casing type parameters as they were before?

@weswigham
Copy link
Member Author

Based on discussions we've had offline, this is likely superseded by #19587, however I can always reopen this pr if we change our mind.

@weswigham weswigham closed this Oct 30, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine unions in contextual typing
2 participants