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

Treat `in` operator as type guard #10485

Closed
RyanCavanaugh opened this issue Aug 22, 2016 · 39 comments
Closed

Treat `in` operator as type guard #10485

RyanCavanaugh opened this issue Aug 22, 2016 · 39 comments

Comments

@RyanCavanaugh
Copy link
Member

@RyanCavanaugh RyanCavanaugh commented Aug 22, 2016

Inspired by #10421

The in operator could trivially be seen as a type guard:

interface A {
  x: number;
}
interface B {
  y: string;
}

let q: A | B = ...;
if ('x' in q) {
  // q: A
} else {
  // q: B
}

Basically, for a n in x where n is a string literal or string literal type and x is a union type, the "true" arm narrows to types which have an optional or required property n, and the "false" arm narrows to types which have an optional or missing property n.

@aravindarun

This comment has been minimized.

Copy link

@aravindarun aravindarun commented Aug 23, 2016

interface A {
    x: number;
}

interface B {
    y: string;
}

interface C {
    x: string;
}

let q: A | B | C = ...;
if ('x' in q) {
    // q: A or C
    // q.x ?
} else {
    // q: B
}

What would be the type of q.x in this case?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Aug 23, 2016

if ('x' in q) {
    // q: A | C
    // q.x: number | string
} else {
    // q: B
}
@aravindarun

This comment has been minimized.

Copy link

@aravindarun aravindarun commented Aug 23, 2016

My bad.. 👍

@jeffreymorlan

This comment has been minimized.

Copy link
Contributor

@jeffreymorlan jeffreymorlan commented Aug 23, 2016

This only makes sense if the interface B has some way to declare that it does not have the property x. Otherwise, objects implementing B may very well have an x as an implementation detail:

class C implements B {
    y = 'foo';
    // this private method makes the type guard consider objects of this class to be an A, and not a B
    private x() { ... }
}
@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Aug 23, 2016

@jeffreymorlan While that's true, in practice people write user-defined type predicates all the time which basically assume that things are effectively sealed.

@jeffreymorlan

This comment has been minimized.

Copy link
Contributor

@jeffreymorlan jeffreymorlan commented Aug 23, 2016

That should be expressed in the declaration; otherwise you're constantly forced to consider whether an interface is sealed or not. Avoiding that kind of ad-hoc mental typing is the main reason for TypeScript to exist at all.

@aravindarun

This comment has been minimized.

Copy link

@aravindarun aravindarun commented Aug 23, 2016

@jeffreymorlan The guard in question have no info on Class C whatsoever. And since q is a union type, nothing breaks at compile time.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Aug 24, 2016

The in operator could trivially be seen as a type guard

@RyanCavanaugh added Suggestion Accepting PRs Difficult

😜

@jeffreymorlan

This comment has been minimized.

Copy link
Contributor

@jeffreymorlan jeffreymorlan commented Aug 24, 2016

@aravindarun It breaks at run time:

let q: A | B;
q = new C(); // allowed because C is assignable to B
if ('x' in q) { // this returns true at runtime
    // compiler would think q is an A here - it is not
    q.x.toFixed(); // fails at runtime, because q.x is a function, not a number
}

If the interface B could be sealed so that types with additional properties are not assignable to it, then that type guard would be correct.

Otherwise, this adds a major "gotcha" to the language. You won't be able to assign any object to an interface type without thinking about whether the interface is sealed or not. And since people will inevitably forget that, in existing code you won't be able to add any property, even a private one, without thinking if the name might clash with some other interface's tag.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Aug 24, 2016

@jeffreymorlan this line of argument isn't really a productive one because people have been writing code with this "hole" for years already. See the code in #10421 -- it could fail in exactly the same way you're describing. In fact, there are many trivial ways to create unsound programs that fail to meet their types at runtime, it's just that in practice no one aggressively tries to break themselves.

@aravindarun

This comment has been minimized.

Copy link

@aravindarun aravindarun commented Aug 24, 2016

@jeffreymorlan

if ('x' in q) { // this returns true at runtime
    // compiler would think q is an A here - it is not 
    q.x.toFixed(); // fails at runtime, because q.x is a function, not a number
}

// compiler would think q is an A here - it is not

Well, it better be (since q's declared type is A | B). That is the whole point of Typescript and typing. Right?

@RyanCavanaugh +1 for the last comment.
And Typescript did prefix private fields with '_' in atleast a few of the previous versions right?

@ethanresnick

This comment has been minimized.

Copy link
Contributor

@ethanresnick ethanresnick commented Sep 20, 2016

I think this is a duplicate of #1427.

@mhegazy

This comment has been minimized.

Copy link

@mhegazy mhegazy commented Sep 20, 2016

thanks @ethanresnick, This issue seems to have more up-to-date discussion. closing #1427 in favor of this issue.

@tdsmithATabc

This comment has been minimized.

Copy link

@tdsmithATabc tdsmithATabc commented Jan 4, 2017

Is there a preferred alternative that folks are using here? I understand that "exclusive" unions and sealed types are theoretically thorny, but it's also the natural paradigm my brain suggests when trying to do runtime determination of configuration options, like so:

interface ConfigObject {a: any;};
interface ConfigAlternative {b: any;};

export class Foo {
  constructor (config : ConfigObject | ConfigAlternative) {
    if ("a" in config) {
      this.a = (<ConfigObject> config).a;
    } else {
      this.b = (<ConfigAlternative> config).b;
    }
  }
}

(As you can see I'm currently getting around it with assertions.) In such a simple example I could just do away with the conditionals and just use Object.assign or similar, but in situations where I'm processing the input in some way, I'll need some cases regardless, so this structure feels appropriate.

Is there another paradigm I should be using? I'd prefer not to use real classes because it would require adding a great number of imports (including some detangling of cycles) and it seems overkill for such short-lived literals.

EDIT: I guess I can be writing my own type predicates that are internally backed by in, but I was hoping for something cleaner, since each configuration type would need its own predicate and would generally only be used once or twice.

@pelotom

This comment has been minimized.

Copy link

@pelotom pelotom commented Jan 26, 2017

@tdsmithATabc: as far as an alternative, as I commented in #13695 you can define a generic type guard that works for any property:

export function hasKey<K extends string>(k: K, o: {}): o is { [_ in K]: {} } {
  return typeof o === 'object' && k in o
}

then use it like so, for example:

type Foo = { x: number } | { y: string }

function f(foo: Foo) {
  if (hasKey('x', foo)) {
    console.log(foo.x + 5)
  } else {
    console.log(foo.y.length)
  }
}
@tdsmithATabc

This comment has been minimized.

Copy link

@tdsmithATabc tdsmithATabc commented Jan 26, 2017

Ahh thanks, I haven't started using mapped types because my work environment isn't on 2.1 yet. But that's pretty slick. 😄

@niieani

This comment has been minimized.

Copy link

@niieani niieani commented Jan 27, 2017

Might we do the same for hasOwnProperty and simple if property or if typeof property?:

interface A {
  x: number;
}
interface B {
  y: string;
}

let q: A | B = ...;
if (q.x) {
  // q: A
} else {
  // q: B
}

// and also:

if (typeof q.x !== 'undefined') {
  // q: A
} else {
  // q: B
}

// and:

if (q.hasOwnProperty('x')) {
  // q: A
} else {
  // q: B
}
@tdsmithATabc

This comment has been minimized.

Copy link

@tdsmithATabc tdsmithATabc commented Jan 27, 2017

Well, to reiterate what others said above: interfaces make no assertion that an object doesn't have additional members. Because of this, there's all sorts of "deficient" situations:

class B2 {
  public x : boolean = false;  // does not satisfy A!
  public y : string = "~";
}
const q : A | B = new B2();

if (q.x) {
  // q is NOT A!!
} else {
  // q: B
}

People have mentioned run-time manipulations that make this trickier too.

@niieani

This comment has been minimized.

Copy link

@niieani niieani commented Jan 27, 2017

@tdsmithATabc I think that's a different problem. It shouldn't be possible to cast B2 to A | B, because B2 can never satisfy A -- only B.

Therefore A | B should immediately be limited to B or throw an error/warning.

If there was a type guard in place preventing this type of union casting of incompatible types, the problem would not occur.

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jan 27, 2017

I would like to see #1260 included as part of this.

@tdsmithATabc

This comment has been minimized.

Copy link

@tdsmithATabc tdsmithATabc commented Jan 27, 2017

I think that's a different problem. It shouldn't be possible to cast B2 to A | B, because B2 can never satisfy A -- only B.

Therefore A | B should immediately be limited to B or throw an error/warning.

@niieani I think you're creating a different operator than union here (a partial of an intersection?). How would you handle literals with that logic? Nothing could ever "satisfy" true and false, but I can certainly cast true to true | false (boolean).

Plus, what about types that can't be determined at compile-time?

const truth : string | boolean = Math.random() > 0.5 ? "true" : true;

I agree my constant example was poor because it could be immediately narrowed, but that doesn't change the general case of how interfaces and unions currently work.

@tdsmithATabc

This comment has been minimized.

Copy link

@tdsmithATabc tdsmithATabc commented Jan 27, 2017

This all said, separating the issue of XOR-unions and member-exclusion-types, I would still like to see "keyName" in x to automatically narrow to x is {keyName: any}. At present you still get an error from property access after corresponding in that doesn't seem necessary..?

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jan 27, 2017

Inspired by @pelotom, changing the hasOwnProperty definition to:

hasOwnProperty<V extends PropertyKey>(v: V): this is { [_ in V]: any }

Would make this work for hasOwnProperty, right?

Or someone could add:

interface Object {
    hasOwnProperty<V extends PropertyKey>(v: V): this is { [_ in V]: any }
}

to their code.

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jan 27, 2017

Something similar could also be done for Reflect.has.

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jan 28, 2017

Testing out the hasOwnProperty definition above, it messes things up when combined with interfaces with index signatures.

@IdeaHunter

This comment has been minimized.

Copy link
Contributor

@IdeaHunter IdeaHunter commented Apr 16, 2017

@RyanCavanaugh is this issue opened for community(can't see it in the community milestone)?
If yes, what is acceptance criteria? I have played with tsc a little (+10 loc) and got following result :

    class A { a: string; }
    class B { b: string; }
    function negativeClassesTest(x: A | B) {
        if ("a" in x) {
            x.b = "1";
              ~
!!! error TS2339: Property 'b' does not exist on type 'A'.
        } else {
            x.a = "1";
              ~
!!! error TS2339: Property 'a' does not exist on type 'B'.
        }
    }

The only problem i can see right now is dead code detection in following code:

    class AWithMethod {
        a(): string { return "" }
    }
    class BWithMethod {
        b(): string { return "" }
    }
    function negativeTestClassesWithMemberMissingInBothClasses(x: AWithMethod | BWithMethod) {
        if ("c" in x) {
            x.a();
              ~
!!! error TS2339: Property 'a' does not exist on type 'never'.
            x.b();
              ~
!!! error TS2339: Property 'b' does not exist on type 'never'.
        } else {
            x.a();
              ~
!!! error TS2339: Property 'a' does not exist on type 'AWithMethod | BWithMethod'.
!!! error TS2339:   Property 'a' does not exist on type 'BWithMethod'.
            x.b();
              ~
!!! error TS2339: Property 'b' does not exist on type 'AWithMethod | BWithMethod'.
!!! error TS2339:   Property 'b' does not exist on type 'AWithMethod'.
        }
    }

The problem is the unreachable code detection occurs in binding phase and in the moment we do narrowing its already over

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Apr 17, 2017

@IdeaHunter yes, we're accepting PRs for this.

Acceptance criteria would be a working feature with appropriate tests. The behavior you have there seems reasonably good - perhaps the author misspelled "c", after all ?

We can run it on our internal test suite of partner code and see what the real-world breaks look like.

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jul 3, 2017

I love Typescript and I'm glad this is open source work, but I think, at least as evidenced by this issue, the interaction between the maintainers and the community is lacking. This is an issue that was marked as "Accepting PRs" and @IdeaHunter spent what I'm sure was considerable effort to submit a 1400+ LOC PR, but it has been sitting ignored for going on 3 months. This has got to be off-putting to those in the community who are capable and willing to contribute.

@IdeaHunter

This comment has been minimized.

Copy link
Contributor

@IdeaHunter IdeaHunter commented Jul 10, 2017

@EliSnow to be precise this PR have
~20 LOC that actually do the job
~200-300 LOC of tests
the last +1000 LOC is just auto-generated data for tests

As you can see, I did nothing more that just enable existing code to handle new use case and put some unit tests to check that the results are sane

IMHO i believe they on tight schedule working on plugin system design (which Im personally love to see released ahead of my PR)

@EliSnow

This comment has been minimized.

Copy link

@EliSnow EliSnow commented Jul 11, 2017

@IdeaHunter, I'm sure they are working on some pretty awesome features, many of which I'm looking forward to with excitement. I am not trying to be grossly critical, my only goal is to provide some constructive criticism in hopes that interaction between maintainers and the community can be improved.

The maintainers have pretty strict rules in place for what and how the community can contribute, and that's fine, but IMHO when there are issues specifically marked for the community to work on, I think its a disservice to allow their contributions to sit ignored for long periods of time.

@yahiko00

This comment has been minimized.

Copy link

@yahiko00 yahiko00 commented Jul 16, 2017

Good job @IdeaHunter! 👍
Happy to see a part of my proposal is breaking through 😃

@pelotom

This comment has been minimized.

Copy link

@pelotom pelotom commented Dec 1, 2017

Bueller?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 4, 2017

@pelotom Can you be more specific?

@pelotom

This comment has been minimized.

Copy link

@pelotom pelotom commented Dec 4, 2017

@RyanCavanaugh this issue was marked as “help wanted”, and a PR was submitted 6 months ago, which has received no reaction from the TS team. Are there any plans to review and move forward with this?

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 4, 2017

@pelotom reviewed. Sorry for the delay

@pelotom

This comment has been minimized.

Copy link

@pelotom pelotom commented Dec 4, 2017

@RyanCavanaugh thank you! 😄

@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 6, 2017

Thanks @IdeaHunter for implementing this!

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 2.7 milestone Dec 6, 2017
@RyanCavanaugh

This comment has been minimized.

Copy link
Member Author

@RyanCavanaugh RyanCavanaugh commented Dec 6, 2017

@donaldpipowitch

This comment has been minimized.

Copy link
Contributor

@donaldpipowitch donaldpipowitch commented Dec 6, 2017

Cool, thank you.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.