GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Infer string literals at comparison locations by DanielRosenwasser · Pull Request #6196 · microsoft/TypeScript

Conversation

@DanielRosenwasser

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) commented Dec 22, 2015

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

olmobrutall, mquandalle, RobertoUa, tinganho, AJamesPhillips, and SlurpTheo reacted with thumbs up emoji mquandalle reacted with hooray emoji
All reactions
  • 👍 6 reactions
  • 🎉 1 reaction
Daniel Rosenwasser (DanielRosenwasser) added 8 commits December 21, 2015 15:57
@DanielRosenwasser
Added tests.
0d2fb26
@DanielRosenwasser
Accepted baselines.
639d9bf
@DanielRosenwasser
Infer string literal types at comparison locations.
13ec5d7
@DanielRosenwasser
Accepted baselines.
e109b17
@DanielRosenwasser
Removed ad-hoc string-like checking for type assertions, switch/cases…
de9789a
…, and equality comparisons.
@DanielRosenwasser
Accepted baselines.
d0a8573
@DanielRosenwasser
Amended tests.
e452955
@DanielRosenwasser
Accepted baselines.
ced65ac
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Dec 22, 2015
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Dec 22, 2015
WIP: Member-type based type guards #6062
Closed
4 tasks
@weswigham

Wesley Wigham (weswigham) commented Dec 22, 2015

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Dec 22, 2015

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JsonFreeman
JsonFreeman reviewed Dec 22, 2015
View reviewed changes
Comment thread src/compiler/checker.ts Outdated

@JsonFreeman Jason Freeman (JsonFreeman) Dec 22, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 22, 2015

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Dec 22, 2015
Contextually type the rightmost operand of comma operators and logical 'AND's #6199
Closed
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Dec 22, 2015

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser
Separated logic out to 'shouldAcquireLiteralType'.
881b52d
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Dec 22, 2015

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JsonFreeman
JsonFreeman reviewed Dec 22, 2015
View reviewed changes
Comment thread src/compiler/checker.ts

@JsonFreeman Jason Freeman (JsonFreeman) Dec 22, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@DanielRosenwasser
Amended comment.
8365410
JsonFreeman
JsonFreeman reviewed Dec 22, 2015
View reviewed changes
Comment thread src/compiler/checker.ts Outdated

@JsonFreeman Jason Freeman (JsonFreeman) Dec 22, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 22, 2015

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@JsonFreeman Jason Freeman (JsonFreeman) Dec 22, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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 ||.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@JsonFreeman

Jason Freeman (JsonFreeman) commented Dec 22, 2015

Copy link
Copy Markdown
Contributor

I think this change is good.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Dec 22, 2015

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Daniel Rosenwasser (DanielRosenwasser) added 2 commits December 22, 2015 16:13
@DanielRosenwasser
Rewrote loop, amended comment.
58580ed
@DanielRosenwasser
Add conditional expressions, be more conservative in checks for '||'.
069ff33
Daniel Rosenwasser (DanielRosenwasser) added 8 commits January 8, 2016 00:23
@DanielRosenwasser
Added tests for type assertions in match locations.
01cc2f1
@DanielRosenwasser
Accepted baselines.
259a3cf
@DanielRosenwasser
Factored out operand detection logic and made it recursive.
cc2ab55
@DanielRosenwasser
Included type assertions in operand detection logic.
5b9e5d1
@DanielRosenwasser
Accepted baselines.
fdd7fde
@DanielRosenwasser
Fix lint issues.
09d1762
@DanielRosenwasser
Added scenario to tests.
dec70f1
@DanielRosenwasser
Accepted baselines.
1dd43fa
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jan 8, 2016

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser
Merge branch 'master' into literalTypeLocations
ab25584
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Jan 15, 2016
Is there a special 'hack' for operators and string literal types? #6505
Closed
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) added the Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined label Jan 15, 2016
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Jan 21, 2016
Mutability widening for literal types #6554
Closed
@weswigham Wesley Wigham (weswigham) mentioned this pull request Feb 8, 2016
Strict Comparison Operator Doesn't Infer Singleton Types Correctly #6940
Closed
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Feb 18, 2016
String literal type comparison should result in an error if the compared value is invalid #7112
Closed
@mrcrowl

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Mar 9, 2016

Copy link
Copy Markdown
Member Author

mrcrowl See #606.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Apr 5, 2016
Singleton types under the form of string literal types #1003
Closed
@eboto Erem Boto (eboto) mentioned this pull request Apr 5, 2016
First cut at typescript-lite generator implementation. coursera/courier#20
Merged
Daniel Rosenwasser (DanielRosenwasser) added 3 commits April 5, 2016 11:39
@DanielRosenwasser
Merge branch 'master' into literalTypeLocations
9673152
@DanielRosenwasser
Accepted other baselines.
6b8bac3
@DanielRosenwasser
Merge branch 'master' into literalTypeLocations
740792c
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Jun 29, 2016
Number, enum, and boolean literal types #9407
Merged
Anders Hejlsberg (ahejlsberg) added a commit that referenced this pull request Jul 7, 2016
@ahejlsberg
Adding selected tests from #6196
82c26cd
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Jul 28, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Aug 29, 2016
String literal types #5185
Merged
@mhegazy

Mohamed Hegazy (mhegazy) commented Sep 13, 2016

Copy link
Copy Markdown
Contributor

closing in favor of #9407

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhegazy Mohamed Hegazy (mhegazy) closed this Sep 13, 2016
@mhegazy
Mohamed Hegazy (mhegazy) deleted the literalTypeLocations branch November 2, 2017 21:02
@microsoft 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.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Domain: Literal Types Unit types including string literal types, numeric literal types, Boolean literals, null, undefined

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@DanielRosenwasser @weswigham @RyanCavanaugh @JsonFreeman @mrcrowl @ahejlsberg @mhegazy @sandersn @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.