Skip to content
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

Relations based on variance markers do not take into account parameters used as key types. #29698

Open
jack-williams opened this issue Feb 2, 2019 · 4 comments
Assignees
Labels
Bug A bug in TypeScript
Milestone

Comments

@jack-williams
Copy link
Collaborator

jack-williams commented Feb 2, 2019

TypeScript Version: 3.3

Code

type Record2<K extends keyof any, T> = {
    [P in K]: T;
};

function defaultRecord(x: Record<'a', string>, y: Record<string, string>) {
    x = y; // no error, but error expected.
}

function customRecord(x: Record2<'a', string>, y: Record2<string, string>) {
    x = y; // no error, but error expected.
}

function mixed1(x: Record2<'a', string>, y: Record<string, string>) {
    x = y; // error
}

function mixed2(x: Record<'a', string>, y: Record2<string, string>) {
    x = y; // error
}

Playground Link: link

Related Issues: #28798

@jack-williams jack-williams changed the title Relating generic mapped types does not instantiate constraint type. Relating generic mapped types does not instantiate constraint type (in some cases). Feb 2, 2019
@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Feb 5, 2019
@ahejlsberg
Copy link
Member

The cause of this is #27804. With that PR we started measuring variances for type aliases to object and conditional types. The variance measuring logic determines that K is contravariant and T is covariant in Record<K, T>. This is almost true: An instantiation where K is string is unrelated to an instantiation where K is a literal type (because a string index signature doesn't satisfy an actual property).

@weswigham Perhaps we should exclude aliases to mapped types from the variance measuring shortcut?

@jack-williams
Copy link
Collaborator Author

Thanks for looking into this @ahejlsberg. I guess the difference I'm seeing when using two different aliases is because variance measurements are not shared across different aliases of identical types?

An instantiation where K is string is unrelated to an instantiation where K is a literal type (because a string index signature doesn't satisfy an actual property).

This seems a similar problem to #28798.

I guess the underlying problem is that the semantics of string or number as a value type is different to the semantics as a key type. The logic to relate keys can reuse the logic to relate values by inverting the relationship, which works in most cases, but not when string is involved.

Would another---albeit more costly ---solution be to have a key relation that interprets key types into their value type equivalent before using the value relation?

@ahejlsberg
Copy link
Member

Also see #29393 which appears to be an effect of variance measuring for aliased types.

@jack-williams jack-williams changed the title Relating generic mapped types does not instantiate constraint type (in some cases). Relations based on variance markers do not take into account parameters used as key types. Feb 7, 2019
@jack-williams
Copy link
Collaborator Author

jack-williams commented Feb 7, 2019

I changed the title to something that is hopefully closer to the actual issue. Feel free to change if it is misleading.

My understanding now, after your comments and reading the source, is that a contravariant marker is inferred for the parameter K after:

isRelatedTo(getConstraintTypeFromMappedType(target), getConstraintTypeFromMappedType(source), reportErrors))

but this does not take into account the fact that K is used a key type. Is this correct?

Would it be completely unreasonably to collect uses as key types along with variances, and use that to correctly determine when and how two instantiations of K should be related? I feel that there are some wider issues (#29393) that I don't fully understand, so may well be in completely the wrong ballpark.

Am I right in saying that this is going to be a problem if #26797 lands? Where you could have:

interface Record<K extends string,T> {
    [x: K]: T
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript
Projects
None yet
Development

No branches or pull requests

3 participants