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

More prolific literal types #19587

Closed
wants to merge 16 commits into from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Oct 30, 2017

This is an iteration on #19322.

Building on what @ahejlsberg suggested in reviewing #19322, this fixes #16457 by, rather than doing any special-casing of contextual types, causing us to infer literals so long as they are domain-appropriate for a given position. This means that if a number type is contextually typed by a number type, then we retain a literal, and the same each for string and boolean literals.

This necessitated fixing the comparable relationship to be deeply bidirectional (instead of just bidirectional at the top level); as casts between potentially compatible object types would fail thanks to the extra literal types without the deep bidirectionality (this was visible within our own codebase).

Most of the baseline diffs are just new literal types materializing before they are assigned and disappear, however as mentioned in this comment, there are some ramifications where what was previously OK is now an error due to the stricter inference. I think could revert to the old behavior (or similar to it) for type parameters; however this could be an improvement (and is certainly more consistent). Perf-wise, I actually don't see any difference; these types were always there before, we just widened them, whereas now we widen them a step later under most circumstances.

By request, fixes #19632. Also fixes #19837.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 31, 2017

Apart form the test change, do you see any breaks in the RWC baselines?

@weswigham
Copy link
Member Author

weswigham commented Oct 31, 2017

@mhegazy Two fewer errors (one each in two projects) due to the more permissive comparable relationship (desirable - the casts should likely have been allowed, but were prevented due to incompatibilities on inner types, which are now allowed as the inner types are compared bidirectionally just like the top level), and a bunch of altered error messages which now contain literals, but no new breaks.

@weswigham
Copy link
Member Author

@ahejlsberg I've incorporated the changes you suggested in person.

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.

Two questions and one naming suggestion.

@@ -9491,7 +9497,7 @@ namespace ts {
}
// Report constraint errors only if the constraint is not the empty object type
const reportConstraintErrors = reportErrors && constraint !== emptyObjectType;
if (result = isRelatedTo(constraint, target, reportConstraintErrors)) {
if (result = isRelatedToMonodirectional(constraint, target, reportConstraintErrors)) {
Copy link
Member

Choose a reason for hiding this comment

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

why only here and in the union/intersection type handling? why does it become bidirectional everywhere else?

}

function isLiteralContextualType(contextualType: Type) {
function isLiteralContextualType(contextualType: Type, candidateLiteral: Type): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Consider flipping the parameters and changing the name to canBeContextuallyTypedAsLiteral (or something similar; probably the parameter names would need to change too.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@rbuckton 's dynamic names PR already includes both of those things, actually.

Type '() => string | number' is not assignable to type '() => number'.
Type 'string | number' is not assignable to type 'number'.
Type 'string' is not assignable to type 'number'.
tests/cases/conformance/expressions/contextualTyping/arrayLiteralExpressionContextualTyping.ts(8,5): error TS2322: Type '[1, 2, 3, string]' is not assignable to type '[number, number, number]'.
Copy link
Member

Choose a reason for hiding this comment

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

hm, it would be nicer if this change didn't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? 1, 2, and 3 are verbatim what was written and are all contextually typed by number. It's pretty reasonable, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

People expect to see types in a type error, and don't think of 1 as a type. It's not the end of the world; it remains pretty clear because this only happens with contextual typing when the value is sitting right there in plain sight.

@ahejlsberg
Copy link
Member

I think there is some more fundamental thinking we need to do around the comparable relationship before this is ready to go.

The comparable relationship should hold if some value exists that belongs to both types. For object types I think this means we should consider them comparable unless we can prove they're disjoint. I'm not sure that's what we currently do, even with this PR. For example, when a member is present in one object type but not the other, we should consider it a match (because we can't prove that it isn't).

Also, the relationship should be commutative (bidirectional), which it isn't, even with the changes in this PR (e.g. isTypeComparableTo is unidirectional for primitives at the top level).

@weswigham
Copy link
Member Author

weswigham commented Nov 8, 2017

@ahejlsberg

For object types I think this means we should consider them comparable unless we can prove they're disjoint

Any given type variables T and U cannot be proven to be disjoint, so they would be comparable under such a scheme. However, we have a lot of people that assume that we save you from going if (T === U) right now.

Maybe its better to think of this in terms of troubleshooting this specific issue with the comparable relationship:

declare enum Strings { 
  A = "a",
  B = "b"
}

declare let a: { y: 1 | 2, z: "b" | "c" };
declare let b: { y: number, z: Strings.A | Strings.B };

a = b as typeof a;
b = a as typeof b;

Those casts fail - even though { y: 1, z: "b" } should be castable to both types - even though each individual member type is comparable:

declare let az: "b" | "c";
declare let bz: Strings.A | Strings.B;

az = bz as typeof az; // succeeds on Strings.A | Strings.B -> "b | "c", fails on "b" | "c" -> Strings.A | Strings.B  overall success
bz = az as typeof bz;

declare let ay: 1 | 2;
declare let by: number;

ay = by as typeof ay; // fails number -> 1 | 2, succeeds on 1 | 2 -> number, overall success
by = ay as typeof by;

(all of those casts work).

comparableRelation is implemented identically to assignableRelation except:

  • No excess property checks on fresh types
  • Mapped types' modifiers are always comparable
  • Generics are erased when comparing singleton signatures, instead of compared strictly
  • When source is a union, only some element of that union needs to be comparable, rather than each element.
  • Optional properties in both types can have incompatable types (in strict mode, this is also handled by the each => some union flip, since all optional properties have undefined in their domain)

Ergo, { y: 1 | 2, z: "b" | "c" } would be comparable to { y: number, z: Strings.A | Strings.B } if both directions were checked both at the top level and at each member, instead of just at the top.

Right now what we really have is a partiallyComparable relationship that we've branded as comparable, but then check both directions of at many use sites (except narrowing where we only use one direction in a few places, and is potentially an oversight since in others within narrowing we use both directions) - in checkAssertionWorker, checkBinaryLikeExpression, and checkSwitchStatement (all of its non-narrowing uses) we always look at both directions. Our internal usages arguably expect a deeply comparable behavior - nobody would expect the example cast at the top of this comment to fail.

@weswigham
Copy link
Member Author

Superseded by #19966

@weswigham weswigham closed this Nov 13, 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
4 participants