Skip to content

Erroneous compiler error when using generic intersection type as generic class parameter #14022

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
jacamera opened this issue Feb 12, 2017 · 10 comments
Assignees
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@jacamera
Copy link

jacamera commented Feb 12, 2017

TypeScript Version: 2.1.6 and 2.2.0-dev.20170212

Code

class Foo<T> {
	f(x: Partial<T>) { }
}
class Bar<T> extends Foo<T & { a: number }> {
	constructor() {
		super();
		this.f({ a: 0 });
	}
}

Expected behavior:
Code compiles without error. Worked fine with 2.2.0-dev.20161204 and broke on 2.2.0-dev.20161205.

Actual behavior:
The following error is generated:

main.ts(7,10): error TS2345: Argument of type '{ a: 0; }' is not assignable to parameter of type 'Partial<T & { a: number; }>'.

@jacamera jacamera changed the title Erroneous compiler error when using intersection type as generic class parameter Erroneous compiler error when using generic intersection type as generic class parameter Feb 12, 2017
@mhegazy mhegazy added the Bug A bug in TypeScript label Feb 14, 2017
@mhegazy mhegazy added this to the TypeScript 2.3 milestone Feb 14, 2017
@sandersn
Copy link
Member

Broken by #12643. The break may have been intentional; I need to re-read the change.

@sandersn
Copy link
Member

After looking at #12643, I'm not yet sure whether the example should be legal or not, but I do know that it worked by mistake previous to the change. Before #12643, keyof (T & { a: number }) was incorrectly identical to keyof { a: number }.

The breaking change happens in getIndexType, which creates types for keyof T. Before #12643, if the type was a type parameter, then a generic index type was created (keyof T, basically). Otherwise the index type was created immediately. After #12643, the code checks whether the type is a type parameter or is a union/intersection that contains a type parameter.

I'm not sure whether { a: 0 } should be assignable to Partial<T & { a: number }> or not. Seems like it should on the surface. I'll look into that next.

@jacamera
Copy link
Author

Appreciate your looking into this!

I am using this structure to create abstract, reusable React components with TypesScript. A simplified example might be something like the following:

abstract class Dialog<P, S> extends React.Component<P, S & { submitButtonText: string }> {
	f() {
		this.setState({ submitButtonText: '' });
	}
}
class SignUpDialog extends Dialog<{}, { userName: string }> {
	g() {
		this.setState({ userName: '' });
	}
	h() {
		this.setState({ submitButtonText: '' });
	}
}

This way the abstract components can still manage the state that they are aware of.

The compiler logic is a bit over my head, but in the above example the error is only thrown on the statement in f() and not h() which doesn't seem right to me.

@sandersn
Copy link
Member

Hm. mappedTypeRelatedTo has a special case for Partial and friends: ONLY {} is assignable to it. I don't think this special case takes into account that the constraint type of a mapped type might have known properties like Partial<T & { known: number }> does.

@ahejlsberg, can you take a look? It looks like mappedTypeRelatedTo needs to change to account for cases like this.

@gcnew
Copy link
Contributor

gcnew commented Feb 15, 2017

I think the current behaviour is correct but unexpected. The explanation is given here.

I guess using spread types instead of intersections would yield the expected result.

@sandersn
Copy link
Member

In this case we do know some properties of T & { a: number}, so {a: number} should be assignable to Partial<T & { a: number }> if it is assignable to Partial<{ a: number }>.

@gcnew
Copy link
Contributor

gcnew commented Feb 15, 2017

T may have a property a: string. The resulting type would then be a: string & number which makes a: number not assignable to the intersection. Generally, the values of the properties of type T are not assignable to the corresponding properties in T & U because of deep merging. In the above case U is simply a: number.

@jacamera
Copy link
Author

jacamera commented Feb 16, 2017

This might be a bit off track, but I'm surprised the result of conflicting properties on an intersection type, where any one conflicted property is a primitive, is an intersection of those properties (how can a string & number type exist?). In such cases I would expect the result type to instead be a union of the conflicted properties in which case I don't think there would be a problem with the assignment.

@sandersn
Copy link
Member

@gcnew Oh, I see now. You are right, it is surprising but unfortunately correct that { a: number } is not assignable to T & { a: number } inside of Partial.

@jacamera string & number is impossible to create but string | number is not correct because the resulting T & { a: number} might end up with a: string instead of a: number, and uses of it inside Bar would incorrectly assume it to be a number.

The usual case for intersections is either no overlap or agreement in types. Intersecting conflicting types is just a fallback that is correct once in a while, and unusable the rest of the time.

@sandersn sandersn added Design Limitation Constraints of the existing architecture prevent this from being fixed and removed Bug A bug in TypeScript labels Feb 16, 2017
@jacamera
Copy link
Author

jacamera commented Feb 16, 2017

Understood. Thanks again for your time.

To anyone else who finds this, the dialog example from above can be rewritten in the following way to avoid any compiler errors. The only downside I see is that all the properties of the abstract component's state type have to be optional so the compiler can't enforce the required properties in the initial state assignment.

interface DialogState { submitButtonText: string }
abstract class Dialog<P, S extends Partial<DialogState>> extends React.Component<P, S> {
	f() {
		this.setState({ submitButtonText: '' });
	}
}
class SignUpDialog extends Dialog<{}, Partial<DialogState> & { userName: string }> {
	g() {
		this.setState({ userName: '' });
	}
	h() {
		this.setState({ submitButtonText: '' });
	}
}

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants