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

this subclass inference problem #15615

Closed
mjbvz opened this issue May 5, 2017 · 8 comments · Fixed by #19671
Closed

this subclass inference problem #15615

mjbvz opened this issue May 5, 2017 · 8 comments · Fixed by #19671
Assignees
Labels
Fixed A PR has been merged for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue

Comments

@mjbvz
Copy link
Contributor

mjbvz commented May 5, 2017

From @JonathanMEdwards on May 5, 2017 19:19

  • VSCode Version: Code 1.12.1 (f6868fce3eeb16663840eb82123369dec6077a9b, 2017-05-04T21:18:32.269Z)
  • OS Version: Darwin x64 16.5.0
  • Extensions:
Extension Author Version
ExtensionUpdateCheck HookyQR 0.0.2
vscode-npm-script eg2 0.1.9
auto-close-tag formulahendry 0.3.12
backspace-plusplus jrieken 0.0.15
Theme-MaterialKit ms-vscode 0.1.4
debugger-for-chrome msjsdiag 3.1.1
theme-black-plus none 0.0.1
theme-visualstudio black none 0.0.1

Steps to Reproduce:

class A {
  foo = 0;
  method() {
    if (this instanceof B) {
      this.foo;
    } else {
      this.foo;
    }
  }
}
class B extends A {
}

Produces error:

'Property 'foo' does not exist on type 'never'.'

Copied from original issue: microsoft/vscode#26071

@mjbvz mjbvz added the VS Code Tracked There is a VS Code equivalent to this issue label May 5, 2017
@mjbvz
Copy link
Contributor Author

mjbvz commented May 5, 2017

Using TS 2.3.2, I confirmed this error in a js file with checkJs enabled

@DanielRosenwasser
Copy link
Member

We narrow out A because B is structurally identical to A. This is an awkward sort of problem that applies to TypeScript as well.

@DanielRosenwasser DanielRosenwasser added the Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. label May 5, 2017
@JonathanMEdwards
Copy link

I see. adding
private nominal() { }
to the subclass fixes the problem

@JonathanMEdwards
Copy link

But why does this work?

class A {
  foo = 0;
  method() {
    let t = this; // <----- type magic
    if (t instanceof B) {
      this.foo;
    } else {
      this.foo;
    }
  }
}
class B extends A {
}

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. and removed Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Bug A bug in TypeScript labels May 5, 2017
@DanielRosenwasser
Copy link
Member

TypeScript doesn't catch that t and this refer to the same entity because it would require analysis for aliasing of the runtime value. So if you look at t in the else branch, you'll have never, but this will appear unchanged in its type.

@jbrownson
Copy link

jbrownson commented Oct 25, 2017

Using typescript 2.5.3 w/ noImplicitAny, strictNullChecks

Here's another variation of this problem:

export class Foo {x: number}
export class Bar {}
export function f(x: Foo | Bar) { x instanceof Bar || x instanceof Foo }

You get an error on the x in x instanceof Foo because its inferred type is never. The fundamental problem in both my and Jonathan's case seems to be related to conflating structural and nominal typing. There is some structure to the value that Typescript isn't considering, namely the constructor.

Another way to think of it is the first instanceof does indeed prove that x conforms to the structural type of either Foo or Bar, but it doesn't prove that in the second clause of the || that that x doesn't still have that structure, but Typescript assumes it does.

It may be interesting to note that this is distinct from the behavior of typeof:

export function g(x: string | number) { typeof x === "string" || x / 2 }

This code correctly type checks because it isn't possible for x to be a string in the second clause of the || because the typeof x === "string" check not only proves that x is a "structural" string in the first clause, but it proves that it can't be in the second clause as opposed to the instanceof behavior above.

Also, funny running into you here w/ the same bug I was about to report Jonathan, howdy :)

@jbrownson
Copy link

It looks like there is a long history here:
#8168
#8503
#10934
#11245
#11664
#16693
#18276

Looks like a long time ago it worked as I suggested above, but it was moved to work this way. The new feature to not allow instanceof on things that can't be objects made some code that had been working break, though I now realize it had inferred some really strange types before so I'd rather have it break I suppose.

Anyway I've left enough notes here, I'm going to go ahead and work around it by making all of the classes structurally different w/ some dummy functions.

@mhegazy mhegazy added this to the TypeScript 2.7 milestone Nov 2, 2017
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Nov 2, 2017
@jbrownson
Copy link

This seems to be resolved w/ Typescript 2.7

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants