microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.08k stars 12.49k forks source link

Passing an intersection of a generic type with a concrete type to generic superclass, which then uses it via indexed access + `Parameters` utility to determine rest parameters produces surprising (potentially incorrect?) results #50145

Closed coreh closed 8 months ago

coreh commented 2 years ago

Hey there 👋 I believe this is a bug. I struggled to find anything related given that the error message is fairly common but this specific usage ends up combining a lot of features in a way that's a bit hard to describe/search. If it turns out this is either not a bug (and is instead a soundness issue with my approach) or if this is a duplicate, sorry in advance.

Bug Report

🔎 Search Terms

Spread, Rest, Generic, Indexed Access, Inheritance, Intersection

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code

💻 Code

type EventsRecord = Record<string, (...args: any[]) => void>;

class MyEmitter<E extends EventsRecord> {

  emit<K extends keyof E>(eventName: K, ...args: Parameters<E[K]>): void;
  emit(eventName: string, ...args: any[]): void {
    // ...
  }
}

type ChildEvents = {
  'hello': () => void;
  'hello-with-message': (message: string) => void;
}

class NonGenericChildEmitter extends MyEmitter<ChildEvents> {
  hello() {
    // This works
    this.emit('hello');
    this.emit('hello-with-message', 'Sup?');
  }
}

class ChildEmitter<E extends EventsRecord> extends MyEmitter<E & ChildEvents> {
  hello() {
    // This doesnt work
    this.emit('hello'); // <---- Argument of type '[]' is not assignable to parameter of type 'Parameters<(E & ChildEvents)["hello"]>'.
    this.emit('hello-with-message', 'Sup?'); // <---- Argument of type '["Sup?"]' is not assignable to parameter of type 'Parameters<(E & ChildEvents)["hello-with-message"]>'.
  }
}

🙁 Actual behavior

The code above fails producing the two following errors:

Argument of type '[]' is not assignable to parameter of type 'Parameters<(E & ChildEvents)["hello"]>'.
Argument of type '["Sup?"]' is not assignable to parameter of type 'Parameters<(E & ChildEvents)["hello-with-message"]>'.

🙂 Expected behavior

Some background: I have a custom, strongly typed generic event emitter. I then create a generic subclass that inherits from it and specifies a few events that are supported, while also allowing future descenders to specify their own events, by using an intersection type with a generic parameter.

I expected to be able to use the concrete events I just specified with the emit() function inherited from the superclass without issue.

MartinJohns commented 2 years ago

See #50066 and many others.

coreh commented 2 years ago

Hey, thanks for the quick response. Reading through the issue and related issues, and trying to wrap my head around it.

If I understand it correctly, does that mean that by the time that this.emit('hello') is type checked, E & ChildEvents has already been type checked and determined to be of a potentially unbounded type, like {}?

What's actually causing the issue here? Is it the Record<>, the Parameters<> the generic parameter of the child class, the generic parameter of the superclass or something else?

You mention in the linked issue that “Constraints are not taken into consideration.”. Does that mean that the extends EventsRecord clause is only used to externally restrict what generic arguments can be passed in, but not internally to determine what is the type of the generic parameter? If that's the case, why does the following code snippet work:


type EventRecord = Record<string, (...args: any[]) => void>;

class Bar<T extends EventRecord> {
  x: T;
}

class Foo<T extends EventRecord> extends Bar<T & { a: () => void }> {
  foo() {
    this.x.a();
  }
}

Does this only happen when generic methods/functions are also involved?

MartinJohns commented 2 years ago

If I understand it correctly, does that mean that by the time that this.emit('hello') is type checked, E & ChildEvents has already been type checked and determined to be of a potentially unbounded type, like {}?

Within your ChildEmitter<E> the type E is unbound. The compiler does not know what type this is, as it will be provided outside the definition of your class.

In your NonGenericChildEmitter example there is no unbound generic type anymore. The type of E is known to be ChildEvents.

What's actually causing the issue here? Is it the Record<>, the Parameters<> the generic parameter of the child class, the generic parameter of the superclass or something else?

Parameters<> is a conditional type, and the conditional type can't be resolved because the compiler does not know what the type E is at this point.

You mention in the linked issue that “Constraints are not taken into consideration.”. Does that mean that the extends EventsRecord clause is only used to externally restrict what generic arguments can be passed in, but not internally to determine what is the type of the generic parameter?

Yes, it limits what type can be provided for E, but it's not used to resolve the conditional type Parameters<>.

If that's the case, why does the following code snippet work:

It's not using a conditional type.

coreh commented 2 years ago

Oh, that makes sense then. Thanks for helping me understand this!

I had thought of conditional types as being limited to ? :, and not also the built-in utility types. I guess indexed access also counts as a conditional? Even removing the Parameters<> but keeping the E[K], with the entries in the Record referring only to the parameters and not each method, I couldn't get it to work.

BTW, is the reason behind this not being supported performance? Could it get potentially out of hand due to recursion? Or would it lead to much greater complexity on the type checker?

Would it be possible to somehow internally "flag" this type as "deferred" (or rely on an already existing internal flag) to display a more specific error message?

Maybe something like:

Unable to verify compatibility of types '[]' and 'Parameters<(E & ChildEvents)["hello"]>'. Generic type is not yet known when evaluating the conditional type.

Edit: For anyone finding this thread in the future with a similar problem, I was able to work around the limitation by using a type assertion with the parent type without the generic type:

(this as MyEmitter<ChildEvents>).emit('hello');
(this as MyEmitter<ChildEvents>).emit('hello-with-message', 'Sup?');
fatcerberus commented 2 years ago

I had thought of conditional types as being limited to ? :, and not also the built-in utility types.

You are actually correct on point 1 here - but the built-in utility types are actually defined externally (in library files like lib.d.ts etc.) and some of them are implemented as conditional types, either because they need the distributive behavior (mapping over unions), or because they rely on infer T to extract components of a type—or often, both.

Incidentally the distributive case is generally why conditional types have to be deferred even when it looks like they can be resolved in advance: the input type might be a union of any size so eagerly taking only the true or false branch would be premature.

(On this last note I’ve found with TS that when generics fail to typecheck in cases where it looks like they should, it’s almost always because you’ve failed to account for the possibility of instantiating them with union types. Having two things typed as T sadly isn’t enough to guarantee they always hold compatible values… and the compiler will let you know about it)

RyanCavanaugh commented 2 years ago

BTW, is the reason behind this not being supported performance? Could it get potentially out of hand due to recursion? Or would it lead to much greater complexity on the type checker?

You'd need a completely new kind of reasoning to make this work. Today, we can make good inferences when we see that something is true. But what you need here is a kind of inference that says something couldn't possibly be false -- that it's definitely impossible to construct a type that fits E that doesn't produce an incompatible value when fed through Parameters. Since conditional types are not required to be linear (meaning, if U -> V, then T<U> -> T<V> for all possible U and V) this isn't really even possible unless you also come up with a definition of linearity that can be correctly measured on a conditional type. But even that, here, depends on the fact that classes are subject to inheritance-is-subtyping, which is really only true for bare classes and isn't a guaranteed property of higher-order combinations of classes with other types (e.g. intersections or mixins).

applmak commented 2 years ago

I'm not sure the inheritance is necessarily related to this issue. See this example:

export type Handler = (...payload: any[]) => void;
export type EventDefinition = Record<string, Handler>;
type WSMessages = {
  connect(socketId: number): void;
}
export class WS<CustomMessages extends EventDefinition> {
  f() {
    const b: Parameters<(CustomMessages & WSMessages)["connect"]> = [5];
  }
}

I wonder if it would be possible to know that the the Parameters of the function had to be [number], because of the way that & works: there is no way for any generic type as specified in CustomMessages to "override" or change the resulting type of the "connect" field of CustomMessages & WSMessages.

andrei9669 commented 1 year ago

could my issue be related to this? playground link

everhardt commented 8 months ago

I checked the latest beta (5.4.0) and this does not seem to be fixed there. Should this have been closed as "not planned" or is this actually fixed?

RyanCavanaugh commented 8 months ago

The bulk "Close" action I applied to Design Limitation issues doesn't let you pick what kind of "Close" you're doing, so don't read too much into any issue's particular bit on that. I wish GitHub didn't make this distinction without a way to specify it in many UI actions!

The correct state for Design Limitation issues is closed since we don't have any plausible way of fixing them, and "Open" issues are for representing "work left to be done".