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

instanceof should not exclude possible structural type #17344

Closed
kube opened this issue Jul 21, 2017 · 8 comments
Closed

instanceof should not exclude possible structural type #17344

kube opened this issue Jul 21, 2017 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@kube
Copy link

kube commented Jul 21, 2017

TypeScript Version: 2.4.2

I was playing with TypeScript, and faced this issue, where I can pass a structural type as an object nominally typed.

But using instanceof as a type guard makes TypeScript believe it could not be an Animal anymore.

Code

class Animal {
  run() { }
}

// Create a nominally-typed animal
const a = new Animal()

// Create a structurally-typed animal
const b: Animal = {
  run() { }
}

function makeAnimalRun(animal: Animal | string) {
  if (animal instanceof Animal) {
    // Here animal is inferred as an Animal, which is true
    animal.run()
  }
  else {
    // Here animal is inferred as a string, but still COULD BE of structural type Animal
    console.log(`${animal.toUpperCase()}!!!!`)
  }
}

makeAnimalRun(a)
makeAnimalRun(b) // Why is this accepted?

So instanceof should not work as a type guard anymore (or at least not remove the type as a possibility from the other block), or structural typing should not be permitted with nominal types.

@ghost
Copy link

ghost commented Jul 21, 2017

See #202. Currently, there are no nominal types.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jul 21, 2017
@kube
Copy link
Author

kube commented Jul 22, 2017

@andy-ms @RyanCavanaugh
This is not a duplicate.

It's about the instanceof bug that narrows the type Animal | string to string in the second block, but should not, as it could still be a structural Animal.

@kube kube changed the title Structural Type accepted as Nominal Type? instanceof should not exclude possible structural type Jul 22, 2017
@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 23, 2017

There is no such thing as a structurally typed Animal and a nominally typed Animal as your comments suggest. In your example, a and b have exactly the same type.

I would recommend that you simply not use the instanceof operator for this at all. Instead, a User Defined Type Guard function or simply using typeof x === "string" would be both more correct and more robust in a scenario where you want to match a type of both structurally and nominally while ruling out union members.

@kube
Copy link
Author

kube commented Jul 23, 2017

@aluanhaddad
I'm highlighting a bug, I'm not asking for a workaround.

Even if TypeScript handles all types as structural types, the runtime check done by instanceof is nominal. (Maybe a little less now with ES2015 Symbol.hasInstance)

I'm not asking for nominal types in TypeScript here, just highlighting the fact that this problem could cause real issues when dealing with libraries that expect nominal types:

You could pass a structurally identical object to a function that expects a nominal type (performs an instanceof check at runtime), and TypeScript would not be able to reveal any error, but the runtime will.

If function calls don't make any difference between nominal or structural type, instanceof should.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Jul 23, 2017

I'm saying it's not a bug. instanceof is hardly nominal type checking at runtime. It tests a mutable property of an object that doesn't necessarily imply anything about its heritage.

If an API wants to do a pseudo nominal check for some reason, then that API will be a pain to work with but I can work around it by setting the __proto__ property or by using the Reflect API and so on.

These kinds of checks don't even make sense because they don't tell you anything useful about the so-called subclasses because they don't enforce an interface. Everything about a base class, it's subclasses, and even its heritage is mutable, making it a fairly arbitrary criteria in the first place.

People keep trying to use JavaScript classes as if they were writing in a language or like C# or Java but it's a pointless exercise.

The functions that actually would be very useful to derive from, Array for example, still cannot be reliably subclassed.

Now I realize you may be consuming these APIs, not creating them, but if you are writing APIs that perform such checks please stop wasting people's time by trying to make them conform to a contract that isn't a contract.

@DanielRosenwasser DanielRosenwasser removed the Duplicate An existing issue was already created label Jul 28, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jul 28, 2017

This is not a bug about nominal types (thus not a dupe). @ahejlsberg has more context on this, but see #1719 for what I believe was the original discussion.

@DanielRosenwasser DanielRosenwasser added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jul 28, 2017
@DanielRosenwasser
Copy link
Member

Sorry that comment got amended a few times, I've been trying to dig up the context and match changes in behavior.

@RyanCavanaugh
Copy link
Member

#1719 (comment) really outlines why this works the way it does. TL;DR: While structural matches on class types exist, in practice anyone using instanceof never creates those kinds of objects.

@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.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants