Skip to content

Fix partial type relations#17382

Merged
ahejlsberg merged 4 commits into
masterfrom
fixPartialTypeRelations
Jul 28, 2017
Merged

Fix partial type relations#17382
ahejlsberg merged 4 commits into
masterfrom
fixPartialTypeRelations

Conversation

@ahejlsberg

@ahejlsberg ahejlsberg commented Jul 25, 2017

Copy link
Copy Markdown
Member

With this PR the following is no longer an error:

interface Foo { a: string, b: string }

function f<T extends Foo>(a: Partial<Foo>, b: Partial<T>) {
    a = b;  // No error here
}

Fixes #16985.

Comment thread src/compiler/checker.ts

function isPartialMappedType(type: Type) {
return getObjectFlags(type) & ObjectFlags.Mapped && !!(<MappedType>type).declaration.questionToken;
}

@DanielRosenwasser DanielRosenwasser Jul 26, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems like an awfully specific check - does it work on homomorphic mapped types in which all properties of the type being mapped are already optional? For example:

type Shapeify<T> = {
    [K in keyof T]: any;
}

interface MyInterface {
  something?: number;
}

class MyClass<T extends MyInterface> {
  doIt(data: Shapeify<T>) {}
}

class MySubClass extends MyClass<MyInterface> {}

function fn(arg: typeof MyClass) {};

fn(MySubClass);

The same question applies to if you used Readonly<T> or others.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you're getting at. The only way we can be certain that all properties of the type that results from a mapping operation are optional is if the mapped type includes a ? in the template specification. That's what we're checking here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As we discussed offline, that example doesn't apply because a constraint being all-optional (e.g. MyInterface) doesn't guarantee that T itself will be all-optional.

Comment thread src/compiler/checker.ts
result = Ternary.True;
}
else if (isGenericMappedType(target)) {
result = isGenericMappedType(source) ? mappedTypeRelatedTo(<MappedType>source, <MappedType>target, reportStructuralErrors) : Ternary.False;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you made isGenericMappedType a type predicate, you could avoid the type assertions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, because something for which isGenericMappedType returns false could still be a MappedType. We would need to introduce a new unique type representing generic mapped types, but it isn't worth the hassle.

@ahejlsberg

Copy link
Copy Markdown
Member Author

@DanielRosenwasser If you're otherwise done with the review, please close it out and I will get this PR merged.

@ahejlsberg

Copy link
Copy Markdown
Member Author

@DanielRosenwasser Ping!

@ahejlsberg ahejlsberg merged commit e7e6475 into master Jul 28, 2017
@ahejlsberg ahejlsberg deleted the fixPartialTypeRelations branch July 28, 2017 00:32
@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.

3 participants