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

WIP: Member-type based type guards #6062

Closed
wants to merge 7 commits into from

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Dec 11, 2015

This PR adds type guards of the form:

if (foo.bar.kind === discriminatorType) {
  // foo potentially narrowed
}

Related issues: #186, probably more.

Known issues/TODOs:

  • Comparing against a string doesn't yield a string type specialization, it just gets a string type. I need to get the contextual type of the literal (hence the not-correct changes in stringLiteralTypesInUnionTypes02). It feels awkward that getTypeOfExpression doesn't just return the string type and that I need to special case it, though I would like some advice on how to best do this. @DanielRosenwasser, you should know more about this, what's the best thing to do here?
  • Other type guards don't narrow member types down yet (only property lookup === expr does). I need to rewire the other type guards to allow for nested narrowing.
  • The false branch of a property lookup === expr guard doesn't narrow yet. I'm not 100% sure on what's "safe" to remove in the false branch (probably just nested incompatible union members), though I'm pretty certain it should only do anything when the RHS type is a value literal type (ie, string literal type).
  • I'm pretty sure statements like if (foo["bar"]["kind"] === discriminatorType) don't narrow right now, as I don't handle ElementAccessExpressions yet.

Insights/inputs are welcome - @mhegazy mentioned he wanted me to look into these the other day, and I noticed they've been added to the roadmap for 2.0.

function narrowTypeByValueExpression(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {
assumeTrue = (expr.operatorToken.kind === SyntaxKind.EqualsEqualsEqualsToken) ? assumeTrue : !assumeTrue;
let lhs = skipParenthesizedNodes(expr.left);
const selectors: string[] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Document what a selector is.

@DanielRosenwasser
Copy link
Member

I think we could discuss some issues with string literals with widening semantics. If you'd like, I'll look into what a widening approach would look like based on our earlier conversation.

@weswigham
Copy link
Member Author

@DanielRosenwasser I've looked at it - string literal types just have to be contextually typed. Mostly because (unlike predicate types) they can appear inside arrays and object literals... and in all those cases we want them to widen away... except when we don't because there's a type annotation on the thing the literal is getting assigned to. And thus the contextual type is born - informing the type of the RHS by the type defined on the LHS side.

So, my real issue: I want to run checkExpression but where the RHS can be contextually typed to a string literal type. I guess that means adding a case for EqualsEqualsEquals to getContextualTypeForBinaryOperand, where it returns a string literal type for the RHS side if its a string literal, or its type instead.

@DanielRosenwasser
Copy link
Member

@weswigham I think #6196 is the answer to your first TODO 😃

@mquandalle
Copy link

Is this now fixed by #8010?

@basarat
Copy link
Contributor

basarat commented Apr 24, 2016

Is this now fixed by #8010?

Nope.

type Action = { type: "REQUEST" } 
            | { type: "SUCCESS", data: string } 
            | { type: "FAILURE", error: any }

let action: Action;
if (action.type === "REQUEST"){
  let inside = action; // Still `Action`
}

Tested with alm just to be sure 🌹 :)
image

@weswigham
Copy link
Member Author

I probably need to rework this PR, #8010 heavily modified the type guard code.

@weswigham
Copy link
Member Author

weswigham commented Apr 26, 2016

I think I've mostly reconciled the work in this PR with master and the control-flow based type guards therein. Additionally, the bug I had with the code in the emitter is gone now, so that's nice. It feels like it works more cleanly now that CFA-based guards are in. I've updated the OP to match current progress.

@weswigham
Copy link
Member Author

@DanielRosenwasser There's a change to the compatibility relationship test baselines (which have this kind of comparison in them) - I'm not 100% sure what the desired behavior there is, do you want to look at it?

@Lenne231
Copy link

@weswigham Are there any plans to put this in 2.0? This would be very useful for flux based applications or any application that uses websockets or some kind of messages.

@weswigham
Copy link
Member Author

#9163 succeeds this. Go follow that. 😄

@weswigham weswigham closed this Jun 14, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
@weswigham weswigham deleted the narrow-by-value branch December 18, 2018 00:02
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