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

Inconsistent type compatibility for a type with a call signature and and index signature #23226

Open
dragomirtitian opened this issue Apr 6, 2018 · 8 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Apr 6, 2018

TypeScript Version: 2.9.0-dev

Search Terms: index signature intersection

Code

type INOk = {
    (): string;
    [name: string] : number;
}

type IOk = {
    (): string;
} & {
    [name: string] : number;
}

declare let val:(() => "") & { foo: number; }
let ok: IOk = val; // This works
let nok: INOk = val; // This does not

Expected behavior:
Both IOk and INOk have the same public structure, they both have a call signature and are indexable, both assign statements should be valid.

Actual behavior:
The second assign statement fails with the message Index signature is missing in type '(() => "") & { foo: number; }'.

Playground Link: link
Related Issues: #15300

Notes
Looking at the checker code, it appears that for IOk type compatibility is checked for each constituent of the intersection type, so we will have :

isRelatedTo (typeof val, IOk) = 
      isRealtedTo(typeof val, () => ""))   // == True, since val has a call signature
             ( // Above equal to 
                   isRelatedTo(() => "", () => "") // ==  True
                   ||
                   isRelatedTo({ foo: number; }, () => "") // == False but it's does not matter
             )
      &&
      isRealtedTo(typeof val, { [name: string] : number }) // = True since the { foo: number} part of typeof val has an inferable index (ie isObjectTypeWithInferableIndex( { foo: number} ) will return true)
            ( // Above equal to 
                   isRelatedTo(() => "",  { [name: string] : number }) // ==  False but does not matter
                   ||
                   isRelatedTo({ foo: number; },  { [name: string] : number }) // == True, 
             )

While for INOk the relation is checked directly, since INOk can't be split into constituents and we have

isRelatedTo (typeof val, INOk) = 
              isRelatedTo(() => "",  INOk) // ==  False INOk has index, but ()=> "" does not
              ||
              isRelatedTo({ foo: number; },  INOk) // == False, No compatible call signature

So then the checker falls back to structural checking (recursiveTypeRelatedTo) but this fails as well because when checking for index compatibility (inside indexTypesRelatedTo), it decides that the typeof value ((() => "") & { foo: number; }) does not have an inferable index (isObjectTypeWithInferableIndex returns false because the intersection type does not have a symbol and even if it did the condition for inferable index checks that the type does not have a call signature (!typeHasCallOrConstructSignatures(type)))

@dragomirtitian dragomirtitian changed the title Inconsitent type compatibility when for a type with a call signature and and index signature Inconsistent type compatibility when for a type with a call signature and and index signature Apr 7, 2018
@dragomirtitian dragomirtitian changed the title Inconsistent type compatibility when for a type with a call signature and and index signature Inconsistent type compatibility for a type with a call signature and and index signature Apr 7, 2018
dragomirtitian added a commit to dragomirtitian/TypeScript4ExtJS that referenced this issue Apr 7, 2018
dragomirtitian added a commit to dragomirtitian/TypeScript4ExtJS that referenced this issue Apr 7, 2018
dragomirtitian added a commit to dragomirtitian/TypeScript4ExtJS that referenced this issue Apr 7, 2018
@dragomirtitian
Copy link
Contributor Author

Proposed solution, allow isObjectTypeWithInferableIndex to return true for intersection types, if any of the intersection constituents have an inferable index.

dragomirtitian@1d7723a

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Apr 9, 2018
@RyanCavanaugh
Copy link
Member

@dragomirtitian we discussed this and think that the intersection type behavior (the assignment is OK) is the desired behavior.

Before we change anything, though, we'd like to understand how you found this - what does the unreduced repro look like?

@dragomirtitian
Copy link
Contributor Author

@RyanCavanaugh This was posted as a question on stackoverflow , the OP did not provide more information about their use-case.

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature and removed In Discussion Not yet reached consensus labels Apr 17, 2018
@RyanCavanaugh
Copy link
Member

Just to expand on what's going on here

One rule is that a type T is assignable to the type X & Y if T is assignable to X and T is assignable to Y; this is one of the first principles of intersection types. There's no direct reasoning about X & Y during this operation.

Another rule is that a type T is assignable to a type { [s: string]: U } if all the declared properties of T are assignable to U and T has no call or construct signatures. The second clause is there because a type like { [s: string]: Foo } is a common "map" type and we don't want bare functions (which have no properties at all) to be assignable to these maps simply because they don't have properties.

This leads us to a "consistency triangle" problem. We think the first rule is correct, and we think the second rule is correct, but we also think X & Y should generally behave the same as its "normalized" form, but doesn't.

A proposed change is that types with at least one property are not subject to the "... does not have call or construct signatures" rule. This would be reasonably straightforward and square the circle, so to speak, but we're not really going to make a change unless we first understand how someone noticed this in the first place. Sometimes people go out hunting for inconsistencies and this hunt usually turns up something; we'd rather spend our risk budget on "real" issues if possible.

TL;DR if anyone noticed this in real code please tell us what it looked like so we can understand the severity/priority of it.

@RaduSzasz
Copy link

RaduSzasz commented Apr 17, 2018

@RyanCavanaugh I posted the StackOverflow question @dragomirtitian is referring to. I didn't encounter the issue in the real world; I am doing my Master Thesis on a subject related to TypeScript and tried to get a better understanding of what assignments does the type system allow.

Thank you for your detailed comment. It's really useful!

@magnushiie
Copy link
Contributor

magnushiie commented Aug 16, 2018

The second clause is there because a type like { [s: string]: Foo } is a common "map" type and we don't want bare functions (which have no properties at all) to be assignable to these maps simply because they don't have properties.

@RyanCavanaugh I would argue that the root cause here is that nothing should be implicitly assignable to the common "map" type unless it is itself a compatible map type - a map type is equivalent to a call signature producing a guaranteed value for any input - most maps do not conform (overriding indexer is not (yet) permitted in JS, so the only option is to define the type as T | undefined). Assignment compatibility of index signatures is currently quite inconsistent, especially considering the assignment compatibility of the keyof types. It's also inconsistent with the equivalent call signatures.

type E = { };
type F = { a: number };
type G = { a: number, b: number };
type I = { [key: string]: number };

type KE = keyof E;
type KF = keyof F;
type KG = keyof G;
type KI = keyof I;

type CE = (key: KE) => number;
type CF = (key: KF) => number;
type CG = (key: KG) => number;
type CI = (key: KI) => number;

declare const e: E;
declare const f: F;
declare const g: G;
declare const i: I;

declare const ke: KE;
declare const kf: KF;
declare const kg: KG;
declare const ki: KI;

declare const ce: CE;
declare const cf: CF;
declare const cg: CG;
declare const ci: CI;

const accept = <T>(value: T) => {};

accept<KE>(ke); 
accept<KE>(kf); // error
accept<KE>(kg); // error
accept<KE>(ki); // error

accept<KF>(ke);
accept<KF>(kf);
accept<KF>(kg); // error
accept<KF>(ki); // error

accept<KG>(ke);
accept<KG>(kf);
accept<KG>(kg);
accept<KG>(ki); // error

accept<KI>(ke);
accept<KI>(kf);
accept<KI>(kg);
accept<KI>(ki);

// conclusion about assignment compatibility:
// KE <: KF <: KG <: KI
// -> transitive relationship

accept<CE>(ce);
accept<CE>(cf);
accept<CE>(cg);
accept<CE>(ci);

accept<CF>(ce); // error
accept<CF>(cf);
accept<CF>(cg);
accept<CF>(ci);

accept<CG>(ce); // error
accept<CG>(cf); // error
accept<CG>(cg);
accept<CG>(ci);

accept<CI>(ce); // error
accept<CI>(cf); // error
accept<CI>(cg); // error
accept<CI>(ci);

// conclusion about assignment compatibility:
// CI <: CG <: CF <: CE
// -> transitive relationship
// -> exact reverse of the `keyof` relationship

accept<E>(e);
accept<E>(f);
accept<E>(g);
accept<E>(i);

accept<F>(e); // error
accept<F>(f);
accept<F>(g);
accept<F>(i); // error

accept<G>(e); // error
accept<G>(f); // error
accept<G>(g);
accept<G>(i); // error

accept<I>(e);
accept<I>(f);
accept<I>(g);
accept<I>(i);

// conclusion about assignment compatibility:
// G <: F <: E <: I
// I <:> E
// -> not transitive relationship
// -> inconsistent with keyof
// -> inconsistent with equivalent call signature

EDIT: added minor comments. Also, although this specific example has not occurred to me in the real world, it has bitten me several times while writing generic libraries that try to constrain generics. Now that I have studied it, it has occurred to me that constraining generic arguments to types with index signatures is the wrong thing to do (although it appeared as if it was the right thing, as on the caller side this works). In general, I think it's a big issue with learning the language and gaining intuition, and also this inconsistency is not documented anywhere.

EDIT2: It is documented that arrays act covariantly, and I guess string index signatures are an extension of arrays, so it is somewhat understandable that maps work also covariantly, but IMO for generic scenarios, the keyof relationship is more important.

@RyanCavanaugh
Copy link
Member

I would argue that the root cause here is that nothing should be implicitly assignable to the common "map" type unless it is itself a compatible map type

This indeed used to be the behavior, but it was changed due to feedback. We got a lot of "bug" reports because people wrote code like this

function checkAges(map: { [name: string]: number }) { ... }
const x = {
  "bob": 32,
  "alice": 26,
  "eve": 42
};

// Error, wat, of course this should be valid!
checkAges(x);

We got enough reports that we seeded a StackOverflow question for it https://stackoverflow.com/questions/22077023/why-cant-i-indirectly-return-an-object-literal-to-satisfy-an-index-signature-re . The old behavior seems right on paper, but in practice led to a lot of unnecessary friction.

@magnushiie
Copy link
Contributor

I just tested 1.8 as well, it treated I and F/G as unrelated types, neither being assignment compatible with the other. If it was one-way compatible (contravariant with regards to the index), maybe there would have been less questions?

accept<E>(e);
accept<E>(f);
accept<E>(g);
accept<E>(i);

accept<F>(e); // error
accept<F>(f);
accept<F>(g);
accept<F>(i); // error

accept<G>(e); // error
accept<G>(f); // error
accept<G>(g);
accept<G>(i); // error

accept<I>(e); // error
accept<I>(f); // error
accept<I>(g); // error
accept<I>(i);

// conclusion about assignment compatibility:
// G <: F <: E
// I <: E
// -> transitive relationship

Furthermore, the next release from 1.8 mentioned in the SO article (2.0) introduced strictNullChecks that could fix it - if in the presence of this flag indexed types were always considered to behave as if the value was optional (implied union with undefined), then the problem would probably not exist. Maybe it would make sense to add this behavior to the flag - assuming the people most surprised by not being able to assign would not turn on strict checks. I understand that a separate flags are a maintenance nightmare, but maybe in this case the indexed types break the type system so fundamentally that it would make sense?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants