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

Allow partial unions for indexed type access #22094

Closed

Conversation

nomcopter
Copy link

@nomcopter nomcopter commented Feb 21, 2018

Solves #21975 and #14366 by allowing partial union types for indexed type access as opposed to creating a null assertion type operator.
@mhegazy @DanielRosenwasser

Fixes #21975
Fixes #14366
Related #16108

@msftclas
Copy link

msftclas commented Feb 21, 2018

CLA assistant check
All CLA requirements met.

@RyanCavanaugh
Copy link
Member

@ahejlsberg take a look?

Copy link
Member

@ahejlsberg ahejlsberg left a comment

Choose a reason for hiding this comment

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

Looks good, but needs the suggested change. Also, please add a test to ensure the expected error is reported.

@@ -7997,7 +8003,7 @@ namespace ts {
getPropertyNameForKnownSymbolName(idText((<PropertyAccessExpression>accessExpression.argumentExpression).name)) :
undefined;
if (propName !== undefined) {
const prop = getPropertyOfType(objectType, propName);
const prop = getPropertyOfType(objectType, propName, /*allowPartialUnions*/ true);
Copy link
Member

Choose a reason for hiding this comment

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

This is a too permissive. For example, it will allow the following to compile with no error:

interface Foo {
    bar: { baz: string } | { qux: number }
}

function f(x: Foo) {
    return x['bar']['baz'];  // Should be an error
}

It's pretty easy to fix through:

const prop = getPropertyOfType(objectType, propName, !accessExpression);

This will allow partial unions only when the indexed access doesn't originate in an expression.

Copy link
Author

@nomcopter nomcopter Feb 27, 2018

Choose a reason for hiding this comment

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

Ah good catch. Fixed! Thanks for the review @ahejlsberg

@mhegazy
Copy link
Contributor

mhegazy commented Mar 1, 2018

@ahejlsberg any more comments?

@weswigham
Copy link
Member

We actually have a NonNullable operator now, though.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 1, 2018

We actually have a NonNullable operator now, though.

u mean generic type? the idea is not to force users to write the type reference if all what they want is to get to the type of the property.

@weswigham
Copy link
Member

I don't think it's a good idea to implicitly filter the union given to only the applicable elements, since now conditional types exist and allow users to explicitly perform that filtering if need be themselves.

@weswigham
Copy link
Member

Like, index has the nice property that that's the inverse of a mapped type right now, this breaks that - a mapped type will never produce multiple "possible" object types.

@weswigham
Copy link
Member

weswigham commented Mar 1, 2018

I'm going to insist that this does not feel like a primitive operation anymore when you do this. You can easily write an Index<T, U> now that has this only-chooses-applicable-members behavior if you need to, but the reverse isn't true. If we take this, there's no way to gain back the type safety.

For posterity: type Index<T, U extends string> = T extends {[K in U]: any} ? T[U] : never

Note how I had to use our current index operator in the definition. If index becomes loose by default, writing a strict version becomes impossible because there's no way to write a type that can trigger the same errors...

@nomcopter
Copy link
Author

Sorry I don't entirely follow. Can conditional types be used to do the following?

interface Foo {
    bar: {
        baz: string;
    } | {
        qux: number;
    }
}

type ShouldBeString = Foo['bar']['baz'];

What favorable errors would be impossible after this change? Could you give an example?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2018

OK, I have added this issue to the agenda of the next design meeting; let's see if we can reach consensuses about this.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 9, 2018

Discussed this again in #22445. Conclusion, with NonNullable available now with conditional types, the original scenario should be unblocked, no reason to add new rules.

@mhegazy mhegazy closed this Mar 9, 2018
@nomcopter nomcopter deleted the indexed-access-partial-unions branch March 9, 2018 21:36
@nomcopter nomcopter restored the indexed-access-partial-unions branch March 12, 2018 04:45
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 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.

6 participants