Skip to content

For consistency, don't make inferences for unused type parameters when comparing two references to the same generic type.#26847

Closed
mattmccutchen wants to merge 1 commit intomicrosoft:masterfrom
mattmccutchen:issue-26815
Closed

For consistency, don't make inferences for unused type parameters when comparing two references to the same generic type.#26847
mattmccutchen wants to merge 1 commit intomicrosoft:masterfrom
mattmccutchen:issue-26815

Conversation

@mattmccutchen
Copy link
Copy Markdown
Contributor

We want this even when strictFunctionTypes is off. Variance computation
was previously disabled when strictFunctionTypes is off, but I removed
that check without breaking any tests.

Fixes #26815.

comparing two references to the same generic type.

We want this even when strictFunctionTypes is off.  Variance computation
was previously disabled when strictFunctionTypes is off, but I removed
that check without breaking any tests.

Fixes microsoft#26815.
/// <reference path='fourslash.ts'/>

////interface Collection<T, U> {
//// dummyT: T;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is setting off breaking change alarm bells for me. What's the impact here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user (like this test case) only relies on inference to match up different parameterizations of the same generic interface Collection<T, U> and never passes a subinterface or an implementing class where a Collection<T, U> is expected, then they may have gotten away (until this PR) with having no reference to T or U in Collection<T, U>. After this PR, no inference will be made and their code will likely fail to type check until dummy properties referencing T and U are added, as I've done to the test case. If a user expects inference to work from a subinterface or an implementing class to Collection<T, U>, then they would already have seen the problem and would already have been forced to add the dummy properties.

Does that answer your question? Perhaps running the PR against your real-world code suite is the best way to get an idea of who's relying on the current inconsistent and undocumented behavior.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'll start effectively requiring the use of structures similar to phantom data. At some point do we make an unreferenced type parameter as a proper error?

Copy link
Copy Markdown
Contributor Author

@mattmccutchen mattmccutchen Sep 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'll start effectively requiring the use of structures similar to phantom data.

As stated in the FAQ, we already require this in all cases except the one inconsistent case I'm fixing in this PR.

At some point do we make an unreferenced type parameter as a proper error?

Careful, in interface Foo<T> { x?: Foo<T>; }, the type parameter is referenced but TypeScript's variance check correctly determines that it's nevertheless independent. The error should be for an independent type parameter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then they may have gotten away (until this PR) with having no reference to T or U in Collection<T, U>

This pattern is extremely widely-(ab)used. I don't think we can move forward with this PR under this limitation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is the only case that the PR changes, so if you don't want to change this case, I guess I'll abandon the PR. I wish you had told me here before I did the work.

@RyanCavanaugh RyanCavanaugh added this to the Community milestone Sep 17, 2018
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants