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 — Add type guard for `in` keyword by IdeaHunter · Pull Request #15256 · microsoft/TypeScript

Conversation

@IdeaHunter

@IdeaHunter IdeaHunter commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Fixes #10485

Added support for typeguarding for following cases:

if ("a" in (x))
if ("a" in x)
if ("a" in x.prop)
if ("a" in x.outer.prop) 
if ('a' in this)
if ('a' in this.prop)

Sorry, something went wrong.

Uh oh!

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

EliSnow, hax, bykbtzr, jovdb, johnfn, styfle, and parker-mike reacted with thumbs up emoji svieira, ntindall, saschanaz, EliSnow, tinganho, alexisfontaine, sgoll, johnfn, styfle, andrefarzat, and parker-mike reacted with hooray emoji mihailik reacted with heart emoji
All reactions
  • 👍 7 reactions
  • 🎉 11 reactions
  • ❤️ 1 reaction
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-signed label Apr 18, 2017
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) reviewed Apr 18, 2017
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
function narrowByInKeyword(type: Type, literal: LiteralExpression, assumeTrue: boolean) {
if ((type.flags & (TypeFlags.Union | TypeFlags.Object)) || (type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType)) {
const propName = literal.text;
return filterType(type, t => !!getPropertyOfType(t, propName) === assumeTrue);

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Apr 18, 2017

Copy link
Copy Markdown
Member

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.

Two spaces here... we should turn on a lint rule

Sorry, something went wrong.

Uh oh!

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

All reactions

@IdeaHunter IdeaHunter Apr 18, 2017

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

Ryan Cavanaugh (@RyanCavanaugh) fixed

Sorry, something went wrong.

Uh oh!

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

All reactions
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) reviewed Apr 18, 2017
View reviewed changes
Comment thread tests/baselines/reference/inKeywordTypeguard.errors.txt
if ("a" in x) {
x.a = "1";
} else {
x.b = "1";

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Apr 18, 2017

Copy link
Copy Markdown
Member

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.

It isn't correct to narrow to BOpn in this branch

Sorry, something went wrong.

Uh oh!

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

All reactions

@IdeaHunter IdeaHunter Apr 18, 2017

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

Ryan Cavanaugh (@RyanCavanaugh) it isn't clear what should we do here since "a" in {a:undefined} === true and how we should look on it from strictNullChecks perspective...
Can you please give an advice how we should narrow this guy?

Sorry, something went wrong.

Uh oh!

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

All reactions

@IdeaHunter IdeaHunter Apr 18, 2017

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

Right now we have following logic: propertyIsInType === assumeTrue. If i got you right then we should change this logic to

propertyIsOptional 
            ? assumeTrue && propertyIsInType 
            : propertyIsInType === assumeTrue

right?

Sorry, something went wrong.

Uh oh!

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

All reactions

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Apr 19, 2017

Copy link
Copy Markdown
Member

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 seems like it might do the trick

Sorry, something went wrong.

Uh oh!

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

All reactions

@IdeaHunter IdeaHunter Apr 20, 2017 •
edited
Loading

Uh oh!

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

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

Im changing behavior to requested one and now line 57 it gives us an error that seems make sense

tests/cases/compiler/inKeywordTypeguard.ts error TS2339: Property 'b' does not exist on type 'AWithOptionalProp | BWithOptionalProp'.
  Property 'b' does not exist on type 'AWithOptionalProp'

Sorry, something went wrong.

Uh oh!

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

All reactions
@IdeaHunter

IdeaHunter commented Apr 20, 2017

Copy link
Copy Markdown
Contributor Author

Ryan Cavanaugh (@RyanCavanaugh) I have updated code to match requested behavior, can you, please, review it?
I would love to hear a feedback from you since i didn't even know do I moving in the right direction or not...

All reactions

Sorry, something went wrong.

Uh oh!

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

@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Apr 24, 2017

Copy link
Copy Markdown
Member

Looks good overall to me. Anders Hejlsberg (@ahejlsberg) care to review?

All reactions

Sorry, something went wrong.

Uh oh!

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

@IdeaHunter

IdeaHunter commented May 1, 2017

Copy link
Copy Markdown
Contributor Author

ping Ryan Cavanaugh (@RyanCavanaugh) Anders Hejlsberg (@ahejlsberg)

All reactions

Sorry, something went wrong.

Uh oh!

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

@EliSnow

EliSnow commented May 16, 2017

Copy link
Copy Markdown

I'm really looking forward to this landing!

mihailik reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@EliSnow

EliSnow commented Jun 7, 2017

Copy link
Copy Markdown

Is it possible this will land for the Typescript 2.4 release?

tdsmithATabc and normalser reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@hax

HE Shi-Jun (hax) commented Sep 12, 2017

Copy link
Copy Markdown

Can I ask what block this PR being merged?

EliSnow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@msftgits Microsoft GitHub User (msftgits) removed the cla-signed label Sep 26, 2017
@microsoft Microsoft (microsoft) deleted a comment from Microsoft Contribution License Agreements (msftclas) Sep 27, 2017
@pelotom

Tom Crockett (pelotom) commented Nov 5, 2017

Copy link
Copy Markdown

Can anyone comment as to why this PR is still languishing 6 months later?

EliSnow and aluanhaddad reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

@IdeaHunter

IdeaHunter commented Nov 5, 2017

Copy link
Copy Markdown
Contributor Author

There is two reasons of it:

  1. They don't care about it/don't like/don't have time for it/didn't yet got bitten in most precious place by a relevant bug 😆/have more important stuffs to do (reason doesn't really matter in this but the lack of motivation to spend some time on it is the only important thing, otherwise this PR would already be merged)
  2. Me not pushing it forward, because of using to babel+flow and didn't use typescript anywhere else(lack of motivation from my side to use typescript)
jovdb, pelotom, aluanhaddad, and hax reacted with confused emoji
All reactions
  • 😕 4 reactions

Sorry, something went wrong.

Uh oh!

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

@jovdb Van den Berghe Jo (jovdb) mentioned this pull request Nov 5, 2017
Narrow union type with 'in' operator #19754
Closed
RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) reviewed Dec 4, 2017
View reviewed changes
Comment thread src/compiler/checker.ts Outdated

function isTypePresencePossible(type: Type, propName: string, shouldHaveProperty: boolean) {
const prop = getPropertyOfType(type, propName);
if (!prop) {

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Dec 4, 2017

Copy link
Copy Markdown
Member

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.

Two edits in this body - rename shouldHaveProperty to assumeTrue and reduce the if to

if (prop) {
  return (prop.flags & SymbolFlags.Optional) ? true : assumeTrue;
}
return !assumeTrue;

Sorry, something went wrong.

Uh oh!

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

All reactions
@RyanCavanaugh

Ryan Cavanaugh (RyanCavanaugh) commented Dec 4, 2017

Copy link
Copy Markdown
Member

Good to merge after one edit. Sorry for the delay on this 🙁

saschanaz, IdeaHunter, pelotom, and hax reacted with thumbs up emoji mihailik reacted with laugh emoji
All reactions
  • 👍 4 reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

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

Jessidhia
Jessica Franco (Jessidhia) reviewed Dec 5, 2017
View reviewed changes
Comment thread tests/baselines/reference/inKeywordTypeguard.errors.txt
if ("a" in x) {
x.b = "1";
~
!!! error TS2339: Property 'b' does not exist on type 'A'.

@Jessidhia Jessica Franco (Jessidhia) Dec 5, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

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'd really like to be able to use in for narrowing, but I'm not sure this is it...

Consider:

const foo = { a: true, b: '' }
negativeClassesTest(foo)

foo is only valid as a B, but it would pass the in check, and then be treated as an A; but its a key is not of type string.

This means that, within the branch, the type of x should still be A|B, but you are allowed to access .a; I just don't know what type .a should have. It probably should be any, which should trip --noImplicitAny if it's not used with a type narrowing expression.

Scenarios:

function negativeClassesTest(x: A | B) {
  if ("a" in x) {
    x.a // --noImplicitAny error
    const y: string = x.a // ideally a --noImplicitAny error, if it's possible to have "signalling any"s
    const z = x.a as string // accepted because it's a cast
    const u: string = typeof x.a === 'string' ? x.a : '' // accepted because it's narrowed
  }
}

It doesn't seem to be all that useful to work like this, but it at least allows you to access .a at all.

Bonus points, but probably hard to actually do in practice:

function negativeClassesTest(x: A | B) {
  if ("a" in x && typeof x.a === "string") {
    // x _should_ be an A!
  }
}

Alternative solution: Have a way to indicate that B has "not this key". Maybe, for example, interface B { a: never; b: string } should mean that B is not allowed to have an a key at all; even if it's set to a value of type never such as undefined!.

Sorry, something went wrong.

Uh oh!

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

All reactions

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Dec 5, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

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.

We considered the soundness aspect and it's not very different from existing soundness issues around aliased objects with undeclared properties. IOW you can already write this code, error-free (even in flow!), which observably fails:

interface T { x: string, y?: number };
const s = { x: "", y: "uh oh" };
const not_t: { x: string } = s;
const unsound: T = not_t;
(unsound.y && unsound.y.toFixed());

The reality is that most unions are already correctly disjointed and don't alias enough to manifest the problem. Someone writing an in test is not going to write a "better" check; all that really happens in practice is that people add type assertions or move the code to an equally-unsound user-defined type predicate. On net I don't think this is any worse than the status quo (and is better because it induces fewer user-defined type predicates, which are error-prone if written using in due to a lack of typo-checking).

For automatically disjointed unions, see #14094.

Sorry, something went wrong.

Uh oh!

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

saschanaz, Jessidhia, and rictic reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

@IdeaHunter IdeaHunter Dec 6, 2017

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

@Kovensky im agree, this is nasty case, but it grows from the fact we have structural type system in place. If we, for example, had nominal type system, the info we provided in function definitions would exactly matching all its usages and we never had this problem in place.

Workaround is to give compiler more complete info about types given, like this:

class A { a: string }
class B { b: string }
class C { a: number; b: string }
function z(x: A|B|C){
	if("a" in x){
		var num_a: number = x.a; // error TS2322: Type 'string | number' is not assignable to type 'number'.
                                         // Type 'string' is not assignable to type 'number'.
		var str_a: string = x.a; // error TS2322: Type 'string | number' is not assignable to type 'string'.
                                         // Type 'number' is not assignable to type 'string'.
	}
}

Sorry, something went wrong.

Uh oh!

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

Jessidhia reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@IdeaHunter

IdeaHunter commented Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

Going to to do requested fixes later, after my work day ends :-)

mihailik and hax reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

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

IdeaHunter added 4 commits December 6, 2017 00:02
@IdeaHunter
Add initial support for 'in' typeguarding
9c3c2ad
@IdeaHunter
Remove whitespace
a5d6be1
@IdeaHunter
Change type narrowing for optional properties
069f73d
@IdeaHunter
Change isTypePresencePossible, accept baseline
04b9b30
@RyanCavanaugh
Ryan Cavanaugh (RyanCavanaugh) merged commit c2fc5ea into microsoft:master Dec 6, 2017
@sandersn Nathan Shively-Sanders (sandersn) mentioned this pull request Dec 7, 2017
in keyword doesn't narrow types with string-indexers correctly #20540
Closed
@ghost ghost mentioned this pull request Dec 11, 2017
property does not exists on type never when using "in" operator #20590
Closed
@massimonewsuk massimonewsuk mentioned this pull request Jan 25, 2018
in keyword doesn't seem to narrow types meta/flow#5717
Closed
@mhegazy Mohamed Hegazy (mhegazy) mentioned this pull request Jan 31, 2018
Invalid TS2339 Property '<prop>' does not exist on type 'never' in if/else branch #21517
Closed
@jcalz Joe Calzaretta (jcalz) mentioned this pull request Feb 7, 2018
Suggestion: treat in operator as type guard which asserts property existence #21732
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) RyanCavanaugh left review comments
+1 more reviewer
@Jessidhia Jessica Franco (Jessidhia) Jessidhia left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

8 participants

@IdeaHunter @RyanCavanaugh @EliSnow @hax @pelotom @Jessidhia @msftclas @msftgits
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.