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

Fix Diff and Omit #21156

Merged
merged 5 commits into from Jan 13, 2018
Merged

Fix Diff and Omit #21156

merged 5 commits into from Jan 13, 2018

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jan 12, 2018

Fixes #21148

This adds another transformation for indexed access types when computing their constraint. The new transformation removes any mapped type whose template type is never when the object type is an intersection. For example,

type T1<T> = (T & { [P in U]: never })[T]
// simplifies to
type T2<T> = T[T]

This works because this mapped type will only ever remove properties that would otherwise be part of the resulting indexed access type. Specifically, it is useful for Diff:

type Diff<T extends string, U extends string> =
    ({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T]
type DiffConstraint<T extends string, U extends string> =
    ({ [P in T]: P } & { [x: string]: never })[T]

This further simplifies to

type DiffConstraint2<T extends string, U extends string> =
    ({ [P in T]: P })[T]
type DiffConstraint3<T extends string, U extends string> =
    T

After further simplifications.

This change is important because the constraint of indexed access type is used for assignability, and before #17912 the constraint of indexed access types like this was mistakenly any. Now that the constraint is a real type, it's important that it's the right one.

Notes:

  1. This PR also adds Diff and Omit to our tests.
  2. Other transformations might also apply, but this PR does not add them. If you have a new error caused by Fix2 get constraint of indexed access #17912, please create a new issue to track it.

@sandersn sandersn changed the title Fix diff omit Fix Diff and Omit Jan 12, 2018
@sandersn sandersn requested review from ahejlsberg, weswigham, mhegazy and a user January 12, 2018 00:38
}

function getSubstitutedIndexedMappedType(type: IndexedAccessType): Type {
const objectType = type.objectType;

Choose a reason for hiding this comment

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

const { objectType } = type?

//// [indexedAccessRetainsIndexSignature.ts]
type Diff<T extends string, U extends string> =
({ [P in T]: P } & { [P in U]: never } & { [x: string]: never })[T]
type Omit<U, K extends keyof U> = Pick<U, Diff<keyof U, K>>

Choose a reason for hiding this comment

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

(on many people's wishlist): add these to lib.d.ts 💟

Copy link
Member Author

Choose a reason for hiding this comment

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

Diff and Omit might someday be in lib.d.ts once conditional types are in Typescript, but the current implementation is way too hacky.

Choose a reason for hiding this comment

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

In fact, only Diff implementation is way too hacky. But it will be solved if subtraction types make it into typescript (someday).

type Omit<U, K extends keyof U> = Pick<U, Diff<keyof U, K>>


type O = Omit<{ a: number, b: string }, 'a'>

Choose a reason for hiding this comment

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

Maybe better add this to ensure a type error shows up if this breaks

const o: O = { b: '' }

@aczekajski
Copy link

aczekajski commented Jan 12, 2018

Does it also repair 2.6.2 incorrect behaviour aroud Omit? Omit written as

type Omit<U, K extends keyof U> = Pick<U, Diff<keyof U, K>>;

is in fact an equivalent of

type Omit<T, K extends keyof T> = {[P in Diff<keyof T, K>]: T[P]};

and since now the latter works incorrectly, both of them should be incorporated in tests and they should also check if they both maintain optional and readonly fields (refer to #12215 (comment) ).

@sandersn
Copy link
Member Author

@aczekajski This fixes the incorrect error message caused by #17912. It does not fix the weird behaviour with optional properties.

@sandersn
Copy link
Member Author

Looks like one of the Travis instances timed out. I'm going to merge this after getting verbal signoff from Mohamed and Anders.

@sandersn sandersn merged commit 6f28b0a into master Jan 13, 2018
@sandersn sandersn deleted the fix-diff-omit branch January 13, 2018 00:10
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 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.

None yet

3 participants