Skip to content

[3.6 regression] Method unions with different generic defaults now error #32506

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

Closed
Validark opened this issue Jul 22, 2019 · 6 comments
Closed
Labels
Breaking Change Would introduce errors in existing code Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Validark
Copy link

TypeScript Version: 3.6.0-dev.20190720

Search Terms: Generic default conflict initializer

Code

interface A {
	bar<T extends number = 1>(): T;
}
interface B {
	bar<T extends number = 2>(): T;
}

declare const foo: A | B;
foo.bar();

Expected behavior: foo.bar() should return 1 | 2 in this case

Actual behavior: error

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug Breaking Change Would introduce errors in existing code labels Jul 31, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.6.1 milestone Jul 31, 2019
@RyanCavanaugh
Copy link
Member

The prior behavior was to give foo.bar() the type 1, which is clearly not correct. The error is an improvement in correctness but a breaking change we need to record in the release notes.

@Validark
Copy link
Author

Validark commented Aug 1, 2019

Is it wrong to evaluate this as 1 | 2? Removing the type checker's ability to merge function definitions has serious implications. I get that it was broken before, but this functionality is useful. Consider this case here:

interface Instances {
	Instance: Instance;
	Model: Model;
	Folder: Folder;
	Camera: Camera;
	ShakyCamera: ShakyCamera;
}

interface Instance {
	ClassName: "Model" | "Folder" | "Camera" | "ShakyCamera";
	Name: string;
	Parent: Instance | undefined;
	IsA<T extends this["ClassName"]>(className: T): this is Instances[T];
}

interface Model extends Instance {
	PrimaryPart: Instance | undefined;
	ClassName: "Model";
}

interface Folder extends Instance {
	ClassName: "Folder";
}

interface Camera extends Instance {
	ClassName: "Camera" | "ShakyCamera";
}

interface ShakyCamera extends Camera {
	ClassName: "ShakyCamera";
}

function f(o: Folder | Model) {
	if (o.IsA("Model")) {
		// This expression is not callable.
		// Each member of the union type
		// '(<T extends "Model">(className: T) => this is Instances[T])
		// | (<T extends "Folder">(className: T) => this is Instances[T]) '
		// has signatures, but none of those signatures are compatible with each other.
		o; // should be a Model
	} else {
		o; // should be a Folder
	}
}

In the above example, the type checker currently knows o.IsA is ("Model") -> this is Model | ("Folder") -> this is Folder. However, it won't type this as "Model" | "Folder", it will just error because "Model" and "Folder" are different. Now, in this case, I can get around this by changing the definition to pull from keyof Instances instead of this["ClassName"] and permit kinda shaky types if used improperly. Now consider this:

type GetProperties<T extends Instance> = {
	[K in keyof T]: K extends "GetPropertyChangedSignal" | "ClassName" ? never : ((() => any) extends T[K] ? never : K)
}[keyof T];

interface Signal {}

interface Instance {
	GetPropertyChangedSignal<T extends GetProperties<this>>(propertyName: T): Signal;
}

function g(o: Folder | Model) {
	o.GetPropertyChangedSignal("Name");
	// This expression is not callable.
	// Each member of the union type
	// '(<T extends "Name" | "Parent" | "PrimaryPart">(propertyName: T) => Signal)
	// | (<T extends "Name" | "Parent">(propertyName: T) => Signal)'
	// has signatures, but none of those signatures are compatible with each other.
}

In this case, it should only allow "Name" | "Parent" as parameters, as that is the commonality between the two function definitions. Just simply make the type checker read the mind of the developer and figure out their intentions and then decide whether to union or intersect the function definitions based on that. Thank you.

@Validark
Copy link
Author

Validark commented Aug 1, 2019

Even in 3.6.0-dev.20190720, this is how the type checker acts if you move the type out of a generic parameter:

interface Instance {
  IsA<T extends keyof Instances>(className: T): this is Instances[T];
  GetPropertyChangedSignal(propertyName: GetProperties<this>): Signal;
}

function h(o: Folder | Model) {
  // Instance.GetPropertyChangedSignal(propertyName: "Name" | "Parent")
  o.GetPropertyChangedSignal("Name");

  if (o.IsA("Model")) {
    // Instance.GetPropertyChangedSignal(propertyName: "PrimaryPart" | "Name" | "Parent")
    o.GetPropertyChangedSignal("PrimaryPart");
  }
}

If you try a similar thing with the IsA method, it types as never:

interface Instance {
  IsA(className: this["ClassName"]): boolean;
}

function j(o: Folder | Model) {
  o.IsA();
  // IsA(className: never): boolean
}

So, I guess the right answer here is intersection, which the type checker currently won't do for my first GetPropertyChangedSignal example.

@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@dsherret
Copy link
Contributor

dsherret commented Aug 31, 2019

@RyanCavanaugh can we change this to an improvement request to make this work? I know type parameters like this our discouraged, but what the compiler is saying isn't true ("This expression is not callable").

In my case, I was using a "local type parameter" like this to relax the type checker's assignability checks. Shifting the type parameter's initializer to the return type causes many assignability errors (ex. I can't assign this to the class' type).

Edit: I ended up finding a workaround for this change, but it was sadly to code generate stuff in the user consumed declaration file (see diff for lib/ts-morph.d.ts).

@dsherret
Copy link
Contributor

dsherret commented Aug 31, 2019

Also, I don't think this was added to the release notes as a breaking change?

Edit: Actually, I guess looking at the other issues in the release there are lots of stuff that don't make the release notes. I'm not sure what's important enough to make it there, but was just wondering because it was mentioned that this should be in the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code 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