FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Infer string literals at comparison locations by DanielRosenwasser · Pull Request #6196 · microsoft/TypeScript · GitHub

Infer string literals at comparison locations - #6196

Closed
Daniel Rosenwasser (DanielRosenwasser) wants to merge 31 commits into
masterfrom
literalTypeLocations
Closed

Infer string literals at comparison locations#6196
Daniel Rosenwasser (DanielRosenwasser) wants to merge 31 commits into
masterfrom
literalTypeLocations

Conversation

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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

Comment thread src/compiler/checker.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That actually might be a better way to go about it.

Copy link
Copy Markdown
Member Author

Ryan Cavanaugh (@RyanCavanaugh) Wesley Wigham (@weswigham) we can discuss that on #6199.

Copy link
Copy Markdown
Member Author

Jason Freeman (@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?

Comment thread src/compiler/checker.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Probably don't need this

Comment thread src/compiler/checker.ts Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not just pass current or parent here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor

I think this change is good.

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

Thanks for the feedback Anders Hejlsberg (@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 commented Mar 9, 2016

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member Author

mrcrowl See #606.

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

closing in favor of #9407

Mohamed Hegazy (mhegazy) deleted the literalTypeLocations branch November 2, 2017 21:02
Microsoft (microsoft) locked and limited conversation to collaborators Jun 19, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL