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

This type predicates for type guards #5906

Merged
merged 17 commits into from
Dec 10, 2015

Conversation

weswigham
Copy link
Member

Closes #5764.

This type predicates are allowed anywhere both a type predicate and a this type are allowed, and additionally are allowed on class and interface members, allowing for this-predicate getters and this-predicate members.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 3, 2015

nice 🏆

@@ -1632,6 +1632,10 @@
"category": "Error",
"code": 2517
},
"A this based type guard is not assignable to a parameter based type guard": {
Copy link
Member

Choose a reason for hiding this comment

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

'this'-based

parameter-based

@DanielRosenwasser
Copy link
Member

Can you add an example where you orphan your type guard method to a function?

}

// @kind (TypePredicateKind.Identifier)
export interface IdentifierTypePredicate extends TypePredicate {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like overkill to have two different kinds of TypePredicate types and a dedicated enum. Could you just use parameterIndex = -1 to indicate this?

targetParamText);
}
else if (hasDifferentTypes) {
reportError(Diagnostics.A_this_based_type_guard_is_not_assignable_to_a_parameter_based_type_guard);
Copy link
Member

Choose a reason for hiding this comment

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

This special casing of this based type guards doesn't really seem necessary to me. I would just treat it like a parameter named 'this' and use the existing error messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

That wouldn't work well at all. The existing error messages look for matching parameter names when comparing two type guards - Parameter "this" is not in the same position as parameter "x" - is a non-intuitive error, given that there is no parameter named "this" (or I would have reused it). They also don't check that the this-based type guard is in a location with a valid this-type.

Plus, when comparing this-based type guards or a this-based guard with a parameter-based guard you don't even have to do most of these checks.

@DanielRosenwasser
Copy link
Member

@JsonFreeman I realized that the way that @weswigham has implemented this, we could discriminate on generators being finished or not.

@JsonFreeman
Copy link
Contributor

@DanielRosenwasser Is that the case? If I understand you correctly, you mean that to check if an iterator/generator is done, you would do

if (iterResult.done) {
    ...
}

Is it possible to use a type predicate on a property declaration? If not, it looks like you'd have to define the done property as a getter. I'm not sure if this is a problem, but it's worth noting. I guess accessors are treated the same as properties, so in theory it might be okay, it just looks weird to define it that way.

The real issue is that a type predicate can only tell you whether this is of a certain type, given its done value. It cannot tell discriminate between two types based on the done value. So would the definition of IteratorResult be something like this (given a type SuspendedType for done: false, and CompletedType for done: true)

{
    value: SuspendedType;
    done: this is { value: CompletedType };
}

Is this the correct definition? I guess this is how you would discriminate between the two types. But if CompletedType is not assignable to SuspendedType, then will the type guard even apply? I think it will still give you SuspendedType.

@DanielRosenwasser
Copy link
Member

If I understand you correctly, you mean that to check if an iterator/generator is done, you would do

if (iterResult.done) {
    ...
}

@JsonFreeman yes, that's exactly correct.

@weswigham has allowed these predicate types on properties because it is necessary to support them on getters as well. It is slightly surprising (at least it was for me), but I don't necessarily see anything wrong with it[1]. It's no more than a boolean that is used as an indicator for the type, and that's being encoded into the type.

You might be correct in that weakness though @weswigham can weigh in. We should have some samples on this.

I think the interface we'd end up with is

interface IteratorResult<CompletedType, SuspendedType> {
    value: CompletedType | SuspendedType;
    done: this is { value: CompletedType };
}

[1] There's an issue regarding "orphaning" the property, whereby we don't orphan the type to boolean:

let x = {
    p: this is Whargarbl,
}
let y = x.p; // 'y' has type 'this is Whargarbl'

@JsonFreeman
Copy link
Contributor

Oh... why is the type of y in your last example a type predicate? In the initial design of type predicates, they denoted type boolean, and the narrowing semantics did not flow around along with the value. I'm curious why this has changed.

Barring that, I think supporting type predicates on properties is fine.

As for IteratorResult, the type you wrote is technically accurate (more so than mine), but it spoils the element type of an iterator, which is supposed to be SuspendedType in the example. This was one of the major challenges of typing generators, and may have to be revisited if generators become more fully supported in the type system.

@DanielRosenwasser
Copy link
Member

Is it spoiled? I think we could still appropriately negate the guard on the union of value and end up with SuspendedType like we would want - not CompletedType | SuspendedType.

@JsonFreeman
Copy link
Contributor

That would involve type subtraction semantics. You would have to have way in the type system of seeing { value: CompletedType | SuspendedType }, and then subtracting from it, { value: CompletedType }, and getting the result { value: SuspendedType }. In order to do this, you would need to make a lot of very specific assumptions about the structure of IteratorResult. That might be okay if you check that all these assumptions hold, when you process lib.d.ts. That way you could ensure up front that this type subtraction would be well defined when you needed it. Also, is the expectation that this subtraction would apply in the false branch of a type guard on done, in addition to iteration constructs?

@DanielRosenwasser
Copy link
Member

But I believe we actually do "the right thing" with user defined type guards:

function f(x): x is { p: string } {
    return true;
}

var v: {
    p: string | number;
}

if (f(v)) {
    v.p // 'p' has type 'string'!
}

@JsonFreeman
Copy link
Contributor

I mean in the else block, does v.p have type number? That's the sort of thing you would need for iteration. Because in iteration, done is false.

@DanielRosenwasser
Copy link
Member

Ah, that wouldn't be covered unfortunately, and you're right.

@JsonFreeman
Copy link
Contributor

Why is a type predicate still effective after a property is orphaned?

And can you do generators with my proposed type, or does the type guard only take effect if the guard type is assignable to the original type?

{
    value: SuspendedType;
    done: this is { value: CompletedType };
}

@DanielRosenwasser
Copy link
Member

Assignable

Why is a type predicate still effective after a property is orphaned?

Because that's the way it just currently works. We were looking into how to shed it properly following an assignment. You can't do it as part of widening because you don't want to "dive in" to the type you're widening.

@@ -4973,6 +5031,7 @@ namespace ts {
if (relation === assignableRelation) {
if (isTypeAny(source)) return Ternary.True;
if (source === numberType && target.flags & TypeFlags.Enum) return Ternary.True;
if (source.flags & TypeFlags.Boolean && target.flags & TypeFlags.Boolean && (source.flags & TypeFlags.PredicateType || target.flags & TypeFlags.PredicateType)) return Ternary.True;
Copy link
Member

Choose a reason for hiding this comment

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

Consider

if (source.flags & target.flags & TypeFlags.Boolean && (source.flags | target.flags) & TypeFlags.Predicate)

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think the current way is clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why do you require at least one of the types to be a predicate type? Surely two plain old booleans are mutually assignable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Previously, all boolean compatibility was covered via reference comparison of the types. Now that there can be multiple boolean types, I needed to actually check for their assignability. However, while claiming that all boolean based types are assignable is correct for now, it would be incorrect once boolean value types are added.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, so after adding the value types, if you have a set containing the types boolean, true, false, and type predicates, which pairs would you want to exclude from the assignability relation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably just true and false, from each other, honestly? Meh, you're probably right, it probably doesn't need to check for type-guardiness.

Copy link
Contributor

Choose a reason for hiding this comment

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

One interesting question is whether boolean or a type predicate should be assignable to a value type like true. Intuitively, it should not be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I imagine not, since their truthiness is unknown.

@JsonFreeman
Copy link
Contributor

Why not just have the property be of type boolean to begin with? The type predicate only needs to take effect on a property access type guard. And that can be achieved by fetching the type predicate associated with the property symbol. The type predicate's narrowing semantics don't need to be trafficked through the property's type.

@@ -2089,6 +2108,7 @@ namespace ts {
ESSymbol = 0x01000000, // Type of symbol primitive introduced in ES6
ThisType = 0x02000000, // This type
ObjectLiteralPatternWithComputedProperties = 0x04000000, // Object literal type implied by binding pattern has computed properties
PredicateType = 0x08000000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does a type predicate act as a type? I think this is the source of the orphaning issue.

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 special case type predicates everywhere with extra slots for them when they're mutually exclusive with a return type (if there is a type predicate, the return type must be boolean)? They even affect type equality (differently than booleans do) when they're nested within a type. Why not unify them all as predicate types? It would be fairly easy to unify them in this way, IMO.

Yes, it being a type is why it follows assignments at present (like all types), but it shouldn't be hard to fix - it's just a matter of making the right branches of getWidenedType return boolean for type predicates.

The more interesting issue I've noted with them right now is that this types aren't properly reinstantiated when narrowing. I can't nest two this type guards, like so:

class FSO {
  isFile: this is File;
  isNetworked: this is (this & Networked);
}
class File {
  path: string;
}
interface Networked {
  host: string;
}
let f: FSO = new File();
if (f.isFile) {
  // f should be File
  if (f.isNetworked) {
    // f should be File & Networked, but is just File 
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that is an interesting issue with the nested 'this' type predicates. It does seem like it should work.

The original reason why type predicates were special cased, was to prevent them from flowing around. That could be taken care of with widening, but it still requires type relations for type predicates. The main thing is that the a type predicate is in intimately tied to one of the parameters in its associated signature (or this), so once it departs from the signature or property, it is really just a boolean. Is widening really enough to make sure it doesn't travel around?

Copy link
Member Author

Choose a reason for hiding this comment

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

It certainly seems like it, given that we widen on assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I am thinking about it. You may be right actually. It's possible we did not think of widening at the time. @mhegazy or @tinganho might have thoughts on this point of whether to treat type predicates as types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed the issue - it was simply that this type guards weren't re-instantiated by instantiateType. 👍

@weswigham
Copy link
Member Author

@JsonFreeman I've been able to unify all type predicates into predicate types with minimal changes to baselines (our type baseline writer doesn't get widened types, so when it prints out the values of predicate returning functions it now prints the predicate rather than boolean, which is a mostly cosmetic change as far as our baselines are concerned) - the typePredicate field on Signature is no more. At @DanielRosenwasser's request, I've preserved some old behavior regarding the assignability of boolean-returning functions to type-predicate returning functions (ie, they're not assignable) which was likely to change so that allowing this can be saved for a later discussion.

@JsonFreeman
Copy link
Contributor

@weswigham Thanks for the update. I have a few thoughts:

  1. In terms of displaying the type, will quick info widen the type to boolean, or will it behave like type writer?
  2. In terms of assignability of boolean-returning functions to type predicate-returning functions, I can see arguments for both, but I agree with @DanielRosenwasser's intuition about keeping it the same for now. It seems they probably should not be assignable, but then again, a boolean is assignable to a type predicate when it's not in return position.
  3. You said "I propose not checking subtype assignability to the this type of this type guards (however, narrowing will still check if the type if a subtype to actually narrow)." What is the difference between the two checks you're referring to, in terms of the source type and target type for each?
  4. This issue of vacuous truth and falsity does not seem unique to this type predicates, or even type predicates. It seems to apply for type guards across the board. I don't think it's a serious issue, but it's probably best to have a consistent policy for all type guards.
  5. The widening approach we discussed seems like it would have a strange effect for function expressions and object literals. Would the following variables have different types?:
var v1: (x: any) => x is T;
var v2 = (x: any): x is T => true; // Does this widen?

var v3: { isT(): this is T; };
var v4 = { isT(): this is T { return true } }; // Does this widen?

It would seems unintuitive to me if v1 and v2 are different, or if v3 and v4 are different.

@weswigham
Copy link
Member Author

@JsonFreeman

  1. Since it widens to boolean, in all meaningful situations (assignments, returns), it gets reinterpreted as a boolean.
  2. And so it is, for now.
  3. The check we do on type guards is that the RHS is a subtype of the LHS, meaning, in this case, that the RHS should be a subtype of the polymorphic this type - which is rather constraining, since this could mean that you can't guard from a base type to a sub-type. When narrowing, we check that they type guarded against is a subtype of the specific type being guarded on, and only narrow if this is the case (so you never narrow "across the lattice" as @DanielRosenwasser once put it).
  4. We check, for parameter type guards, that the type of the parameter is a supertype of the type guarded against in the return type, hence why I stated that we should have not perform the equivalent for this guards, as it would be extremely limiting.
  5. v1 and v2 are the same type, since we only widen on assignments/declarations - we never widen when the user has actually specified a predicate as the function's return type. v3 and v4 are both invalid because there is no polymorphic this available at those locations, but even if they were, there would be no widening in this location because, again, we don't widen return types within the signature.

@weswigham
Copy link
Member Author

By the way, as @DanielRosenwasser pointed out, the unification also makes this possible:

interface Array<T> {
  filter<U extends T>(f: (item: T, index: number, array: T[]) => item is U): U[];
}

var x: (string | number)[]

function isString(x: any): x is string {
    return typeof x === "string";
}

var y = x.filter(isString); // y is inferred as string[]

It's awesome.

@@ -1275,6 +1276,16 @@ namespace ts {
}
}

function checkTypePredicate(node: TypePredicateNode) {
Copy link
Member

Choose a reason for hiding this comment

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

Consider just destructuring parameterName and type

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

if (detached()) {
a.follow();
~~~~~~
!!! error TS2339: Property 'follow' does not exist on type 'RoyalGuard'.
Copy link
Member

Choose a reason for hiding this comment

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

Awesome.

@DanielRosenwasser
Copy link
Member

👍

weswigham added a commit that referenced this pull request Dec 10, 2015
This type predicates for type guards
@weswigham weswigham merged commit 58400ed into microsoft:master Dec 10, 2015
@JsonFreeman
Copy link
Contributor

Oh, so when you widen a function, you don't widen its declared return type.

I think my confusion about the "narrow across the lattice" point, is that I don't see the difference between the type being guarded, and the polymorphic this type (once instantiated). Are these in fact two different types? Or does "polymorphic this type" mean the uninstantiated / generic type?

@JsonFreeman
Copy link
Contributor

And I agree that the type argument inference for type predicates is pretty nifty. Though I am not sure that the return type of a generic function at run time can realistically be related to the type predicate of a callback that's passed in.

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