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

Infer string literals at comparison locations #6196

Closed
wants to merge 31 commits into from

Conversation

DanielRosenwasser
Copy link
Member

This PR removes ad-hoc checks for string-like types by adding new contextual types for literal types at select locations. It addresses some of the major issues brought up in #6167 by adding strictness where users are most interested.

A literal type is inferred for any string literal in a literal match location, defined as such:

  • The expression of a switch statement is a literal match location.
  • The expression of a case clause is a literal match location.
  • Either operand of a ===, !==, ==, or != expression is a literal match location.
  • The expression within a parenthesized expression is a literal match location if the parenthesized expression itself is a literal match location.
  • The expression within a type assertion (of either form) is a literal match location if the assertion expression itself is a match location.
  • Either branch of a conditional expression is a literal match location if the conditional expression itself is a literal match location.
  • Either operand of a || expression is a literal match location if the || expression itself is a literal match location.
  • The right-side operand of a && or , expression is a literal match location if the && or , expression itself is a literal match location.

String literals can still get string literal types through contextual typing, but a check for whether a string is in a literal match location will be apply first.

This means that the following will now error:

if ("foo" === "bar") {
    // ...
}

because types "foo" and "bar" are not assignable to one another.

@weswigham
Copy link
Member

If || gets a contextual type, should && follow similar rules?

@RyanCavanaugh
Copy link
Member

If we do apply a contextual type to &&, it should only be on the right operand

@@ -10866,7 +10885,7 @@ namespace ts {
}

function checkStringLiteralExpression(node: StringLiteral): Type {
const contextualType = getContextualType(node);
const contextualType = getLiteralContextualType(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this even need to be a type? I mean, do you actually use the type for anything, or just check if it exists? I feel like a boolean is more appropriate.

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 actually might be a better way to go about it.

@DanielRosenwasser
Copy link
Member Author

@RyanCavanaugh @weswigham we can discuss that on #6199.

@DanielRosenwasser
Copy link
Member Author

@JsonFreeman I've separated the logic out. This means we can't do both in the same pass, but ¯_(ツ)_/¯

Does anyone have any thoughts on the current approach to things?

// In an assignment expression, the right operand is contextually typed by the type of the left operand.
if (node === binaryExpression.right) {
return checkExpression(binaryExpression.left);
}
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't need this

// a node that isn't a match location, and then get the contextual type of that.
// That would save a few steps but the checks in 'isExpression' seem so involved
// that it would probably be better to simply grab the contextual type if we didn't.
const contextualType = getContextualType(literalNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just pass current or parent here?

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 guess I assumed too much when I wrote the above comment. getContextualType expects an Expression, so we'd need to check if current is an expression using isExpression. But that function is so roundabout, I figured we'd be better off just performing the walk from literalNode again.

Additionally, you need to because we just skipped ||, where the RHS gets contextually typed by the LHS if a parent contextual type doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see what you mean now about isExpression. And good point about ||.

@JsonFreeman
Copy link
Contributor

I think this change is good.

@DanielRosenwasser
Copy link
Member Author

This change thinks you're good too Jason. ❤️

@DanielRosenwasser
Copy link
Member Author

Thanks for the feedback @ahejlsberg, that really cleaned things up. One issue is that in order to truly state that an expression is an operand in a comparison location, I have to include type assertions. For the most part, this isn't a problem. You shouldn't be able to assert that the type of "foo" is something like number. But as you can see here in this test, you can run into issues with intersection types.

This might not be a huge deal, but I'd like to hear your thoughts on whether we should make an exception there.

@mrcrowl
Copy link

mrcrowl commented Mar 9, 2016

Will this include intellisense support for string literals (both for assignment and comparison)? Or should I open that as a separate issue.

@DanielRosenwasser
Copy link
Member Author

@mrcrowl See #606.

@ahejlsberg
Copy link
Member

@DanielRosenwasser #9407 has now been merged so I think we can close this one.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

closing in favor of #9407

@mhegazy mhegazy closed this Sep 13, 2016
@mhegazy mhegazy deleted the literalTypeLocations branch November 2, 2017 21:02
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants