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

Use assignability relation in Best Common Type when checking contextual type against candidates #644

Closed
RyanCavanaugh opened this issue Sep 10, 2014 · 4 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@RyanCavanaugh
Copy link
Member

Code examples:

class Animal {
    name: string;
}
class Dog extends Animal {
    name: any;
    woof() { }
}

var zoo: Animal[];
zoo[0] = new Dog(); // OK
zoo = [new Dog(), new Animal()];  // Error, cannot convert {}[] to Animal[]
enum E { a }
var x: { e: E }[];
x[0] = {e: 0, s: ''}; // OK
x = [{e: 0, s: ''}, { e: E.a }]; // Error, cannot convert {}[] to ...[]

Our current error is:

Type '{}[]' is not assignable to type 'Animal[]'

This is because the contextual type is not a supertype of all the candidate types, nor is any candidate type a supertype of the rest. We do need to use a subtype relationship check here instead of assignability (when checking among candidates) so that any values properly cause arrays to be of type any[] (e.g. [number, string, any] should be any[], not {}[]).

The current spec for Best Common Type (3.10) says:

  • If the set of types is empty, the best common type is C.
  • Otherwise, if C is a supertype of every Tn, the best common type is C.
  • Otherwise, if one exists, the first Tx that is a supertype of every Tn is the best common type.

In the second bullet, we use the subtype/supertype relation. However, when a contextual type is present, we know the next thing we're going to do after computing the BCT is to check that BCT for assignability against the contextual type. Even though we can't use the assignability relation when finding a 'best' type among the candidates, it still makes sense to use the assignability relation when attempting to use the contextual type because this is the relation we're going to try to satisfy anyway.

Instead, this should be:

  • Otherwise, if C is assignable from every Tn, the best common type is C.

This fixes all of the examples above and below.

@RyanCavanaugh RyanCavanaugh added the Suggestion An idea for TypeScript label Sep 10, 2014
@RyanCavanaugh RyanCavanaugh changed the title Report meaningful error when BCT fails due to subtype/assignability differences Use assignability relation in Best Common Type when checking contextual type against candidates Sep 10, 2014
@RyanCavanaugh
Copy link
Member Author

Running through all the places we use BCT:

  • 3.8.5 Contextual signature instantiation -- no contextual type applies, so no impact
  • 4.5 Object literals:
var z: { [s: string]: Animal };
z = { // Desired: No error
    'a': new Animal(),
    'd': new Dog()
};
  • 4.6 Array literals -- already covered
  • 4.12.2 Type inference -- no contextual type
  • 4.15.7 || operator:
var a: Animal;
a = a || (new Dog()); // Desired: no error
  • 4.16 ? operator
var a: Animal;
a = true ? a : (new Dog()); // Desired: no error
  • 6.3 Function implementations -- no contextual type

@JsonFreeman
Copy link
Contributor

I think this is a very good idea. Simple, and very unlikely to break anything.

One thing: "Important to note that we need to use a subtype relationship check here instead of assignability so that null/undefined/any types don't 'poison' arrays" - null and undefined are actually not concerning here because they do not get widened until after best common type (and if there is a contextual type they never get widened at all).

@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Sep 19, 2014
@JsonFreeman
Copy link
Contributor

Here's an interesting thought. Once we change this to assignability, why even have the quadratic subtype pass afterward? Why not just fall to {} directly from there? The proposed algorithm would then become:

  • If every candidate is assignable to the contextual type, the contextual type is the BCT.
  • Otherwise, the BCT is {}.

This seems equivalent to the old algorithm because in the old one, one of two things would happen after the assignability pass fails

  • One of the candidates would become the BCT, and would fail assignability to the contextual type (for the second time)
  • {}, which is identical to the new proposed algorithm

There are two potential differences between the 2 algorithms:

  • Selecting a BCT from the candidates before trying to assign to the contextual type would give a better error message
  • If there is ever a case (and I don't believe there is), where the next step was not to assign the BCT to the contextual type, this algorithm would fall down where the old one would produce a good type.

With union types (#805), the old BCT algorithm (with a contextual type) becomes weird. It essentially would go like this:

  • If every candidate is assignable to the contextual type, contextual type is the BCT
  • If one of the candidates is a common supertype, choose it, and be ready to check its assignability (again) to the contextual type.
  • Otherwise, take the union of the candidates, and be ready to check assignability from all of them (again) to the contextual type.

A lot of this is repetitive and redundant. At the end of the day, we just want to check that all candidates are assignable to the contextual type.

@RyanCavanaugh RyanCavanaugh added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Feb 27, 2015
@RyanCavanaugh
Copy link
Member Author

This is resolved by union types

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants