Skip to content

Make instanceof narrow better using an anonymous class returned from a function #57317

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

Open
DavidArchibald opened this issue Feb 7, 2024 · 4 comments · May be fixed by #57323
Open

Make instanceof narrow better using an anonymous class returned from a function #57317

DavidArchibald opened this issue Feb 7, 2024 · 4 comments · May be fixed by #57323
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@DavidArchibald
Copy link

🔎 Search Terms

Function return, anonymous class, instanceof, mixins, narrowing,

🕗 Version & Regression Information

This is the behavior in every version I tried, and I reviewed the FAQ for entries about instanceof.

I used every-ts (kudos to Jake Bailey for making it so easy) so I'm pretty confident this is true unless some random commit fixed it temporarily.

The closest entry is this: https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-x-instanceof-foo-narrow-x-to-foo

However as I understand it as long as if (x instanceof y) compiles, it should be narrowing x to basically typeof x & InstanceType<typeof y>. There might be some other edge cases I'm not aware of but as you'll see in the playground it seems like this isn't happening as precisely as desired with an anonymous class returned from a function.

⏯ Playground Link

https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABAWQIYGsCmBhANqgZwIB4AVAPgAoC4BbTAZShGGAC5FSBKRAbwFgAUIkQAnTM1FII+InyEiRNekxbBEAXkTLGzVgG4FiAL5DTgoRAQEoKVABMcsggDkQtAEaZRmu1jyEBJQAjABMAMxchoJWYDZ2jgFETKIwYADmvmj+zpQARMBwcHlRQpbWtmDuXqIAknFQqJCYvmCYAO4JToFunt6UpYJVfXUNTRCYAHQ6qgaIAPTziPXA3gSIhIgABsM1WxvrMLYEABZwILj2iF6TZTEV2nSY9TbjmBzg6GBw7UhaaFATpNRE17HQBohyIgAAyTACsiAA-Ig2p00IlnL0ahCOKiukkCCk0ukBtEYOpqE8Xo1mog0q9mnB1OjukQsd4eAJhI96NS3tMnrNgPoFksVmsDtsmgBPfaglFwWw7areLa3QTGIA

💻 Code

function MakeClass<T>(someStuff: T) {
  return class {
    someStuff = someStuff;
  }
}

const MadeClassNumber = MakeClass(123);
const MadeClassString = MakeClass("foo");

const numberInstance = new MadeClassNumber();
numberInstance.someStuff; // Infers as `number` as it should be.

const someInstance: unknown = Math.random() > 0.5 ? new MadeClassNumber() : new MadeClassString();
if (someInstance instanceof MadeClassNumber) {
  someInstance.someStuff; // Infers as `any` and not `number`.
}

🙁 Actual behavior

someInstance.someStuff is inferred as any.

🙂 Expected behavior

someInstance.someStuff is inferred as number.

Additional information about the issue

The code snippet is a simplified version of what I actually ran into. I was creating a bunch of classes for validation purposes. e.g. const Month = new NumberInRange(1, 12);, const Hour = new NumberInRange(0, 23);, const Minute = NumberInRange(0, 59);, or so on. You can imagine much more complicated validation of course.

Later on I wanted to be able to write x instanceof Hour so that I could firstly know that it'd already been validated and secondly know that it was specifically an Hour and not some other type. This might sound more convoluted but it meant validation could occur in one place and I could avoid the problem of all numbers seeming the same at runtime. For various reasons, in this case a discriminated union didn't end up being as ergonomic but these classes were put into a union and instanceof was basically trying to fulfill the same niche.

Now, you can always fix this from going from const MadeClass1 = MakeClass(123); to class MadeClass1 extends MakeClass(123) {}. So I've unblocked myself using that and I wouldn't classify the priority of fixing this very high. However I only happened to be informed enough that this seemed like a reasonable change to try to make. The errors I was getting had to do with the narrowing failing so I don't think the errors you get are intuitive and I think other developers might have given up this style.

In general it might bring extra benefits in terms of consistency in the TypeScript codebase to treat it this way. The issue seems to be that in normal contexts all instances of MadeClassNumber are "typed"* as MakeClass<number>.(Anonymous class) but the instanceof narrowing seems to lose track of that fact and will apply the base constraint of the parent class and then only narrow it based upon that. In this case that means it narrows to MakeClass<any>.(Anonymous class) which doesn't match InstanceType<typeof MadeClassNumber> like one would expect it to.

*I'm using the display of the type I happen to see but I understand there's no stable format nor way to refer to them by a valid identifier.

@Andarist
Copy link
Contributor

Andarist commented Feb 7, 2024

Some extra WAT behavior here:

function MakeClass<T>(someStuff: T) {
  return class {
    someStuff = someStuff;
  };
}

const MadeClassNumber = MakeClass(123);
const MadeClassString = MakeClass("foo");

declare const someInstance: unknown;
if (
  someInstance instanceof
  (MadeClassNumber as typeof MadeClassNumber & { prototype: any })
) {
  const someStuff = someInstance.someStuff;
  //    ^? const someStuff: number
}

The problem is that the .prototype takes priority over the construct signatures when determining this. This behavior is intentional per comment here.

So an extra thing here is that MadeClassNumber['prototype'] is also broken here because all type parameters are replaced with any by getTypeOfPrototypeProperty:
https://github.dev/microsoft/TypeScript/blob/c790dc1dc7ff67e619a5a60fc109b7548f171322/src/compiler/checker.ts#L10698-L10705

If I reason about this correctly, only localTypeParameters should be replaced with any here. I'm still trying to figure out if there are any extra considerations to be taken care of here before I open a PR for this.

@whzx5byb
Copy link

whzx5byb commented Feb 7, 2024

Seems like #17473, but #49863 doesn't fix this issue, only changes the inferred type from any to unknown.

@DavidArchibald
Copy link
Author

DavidArchibald commented Feb 7, 2024

I think the symptom of the problem is similar to what's going on in #17473. In fact it seems to explain why currently it's always any no matter what MakeClass's constraint is. However the solution there is to instantiate generic parameters with their base constraint. This is probably the best that can soundly happen in that case because the class itself is generic and so while it truly could be instantiated with anything people are finding any too permissive.

However, in this case I believe typeof MadeClassNumber and (typeof MadeClassNumber)['prototype'] should actually be equivalent since neither should be generic at all. This seems to be how it works for normal non-generic classes. I call MadeClassNumber non-generic because the only generic involved is the function MakeClass's generic but that should already have been reified. You can see this in the fact that new MadeClassNumber<string>() is an error, as it should be. There simply isn't a generic parameter to use but it seems reifying the prototype was missed or something. Maybe doing that would be unsound in some cases?

With Andarist's insightful discovery it's probably more accurate to title this issue "The prototype of an anonymous class returned from a function is not reified" which is different than "Infer constrained generic parameters after instanceof check" as in #17473 because there shouldn't be a generic at all in the prototype.

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Feb 7, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Feb 7, 2024
@rotu
Copy link

rotu commented Feb 15, 2024

Somewhat related to #56146, where a class expression evaluated twice is treated as a single class for purposes of private fields.

This also seems a bit like const x = (["a"] instanceof Array<number>), which is syntactically forbidden (The right-hand side of an 'instanceof' expression must not be an instantiation expression.(2848)), but TypeScript must allow returning classes since it's legal JS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
5 participants