-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Require optional properties to be present in subtypes #919
Conversation
Remove unused getBestCommonType method (unrelated change)
@@ -3357,7 +3357,7 @@ module ts { | |||
var sourceProp = getPropertyOfApparentType(<ApparentType>source, targetProp.name); | |||
if (sourceProp !== targetProp) { | |||
if (!sourceProp) { | |||
if (!isOptionalProperty(targetProp)) { | |||
if (relation ===subtypeRelation || !isOptionalProperty(targetProp)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space after "==="
👍 |
"In fact, the only remaining features that use subtype checks are union types and best common supertype checks in type argument inference and function return type inference." And overload resolution. |
"This actually shows the flip side of disallowing union types to be created for results of functions with multiple return statements--that would have been the right thing to do here." I would not be surprised if we eventually loosened that rule. I am starting to think we should... |
I love this change! |
Conflicts: tests/baselines/reference/arrayLiteralWithMultipleBestCommonTypes.types
Require optional properties to be present in subtypes
I like this change too. I do want to mention that we should be careful to keep a holistic view of our 1.0 breaking changes. We've been a little loose with that (ie we're allowing some) but the analysis is always in the context of individual changes causing just 1 or 2 little breaks. Since we apply them incrementally as we design each change we can lose sight of the totality of their impact. |
This PR tightens subtyping rules such that optional properties are required to be present in subtypes.
We currently allow subtypes to drop optional properties. This leads to some odd effects:
This currently infers either type
A[]
or typeB[]
forc
in an indeterminate manner. The reason for this behavior dates back to before we had union types--we wanted the above code to produce something better than{}[]
so we engineered the subtyping rules to let this case by. And certainly pickingA[]
orB[]
is better than picking{}[]
. But it's pretty clear by inspection that A and B are different types and that neither should be a subtype of the other, even if they are both assignable to each other.Union types enable us to do the right thing and give
c
type(A|B)[]
. But we need to change the subtyping rules such that A and B aren't both subtypes of each other (because when they are, a union type is reduced to just one of them). With the suggested change a subtype is no longer allowed to drop an optional property. Specifically, if a supertype T has an optional property P, then a subtype S must also have an optional or non-optional property P. For the example above this means that neither A nor B is a subtype of the other, so the type ofc
becomes(A|B)[]
which makes a lot more sense.I'm also changing the relational operators to require one operand be assignable to the other rather than one operand be a subtype of the other. This is pretty much the only place we use a subtype check with operators and it seems unnecessarily strict. In fact, the only remaining features that use subtype checks are union types and best common supertype checks in type argument inference and function return type inference. (Ok, the
instanceof
operator also checks that the right operand is a subtype ofFunction
, but that's pretty immaterial.)The change appears to have very little effect on real world code (one project, Monaco, required a few minor changes).
One change was required in the compiler itself, in a function in emitter.ts. That particular change is required because two anonymous types with properties of types that contain optional properties (subtle!) are no longer subtypes of each other. This actually shows the flip side of disallowing union types to be created for results of functions with multiple return statements--that would have been the right thing to do here. I'm still wondering if we really want to keep that rule (as opposed to just creating union types and relying on those to indicate what the problem was in case it wasn't intended).