Index signature parameter type allows Enum and type alias = number|strin...

[jbondc]

Fixes #2491

[msftclas]

Hi @jbondc, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

[DanielRosenwasser]

Remove stray space

[DanielRosenwasser]

According to the Travis build, it looks like you'll need to update the baselines because we tweaked the error message.

[DanielRosenwasser]

So I think there are other parts we'd need to modify, specifically related to type compatibility and whatnot, but it's hard for me to say. I think you'll need to look at numberIndexTypesRelatedTo. I think @ahejlsberg and @JsonFreeman would definitely be able to give more insight to the parts of the compiler that this affects.

Also, we'll want to add in the following test:

enum E {
    A, B, C
}

var x: { [x: string]: string }
var y: { [x: number]: string }
var z: { [x: E]: number }

x = x;
x = y;
x = z;

y = x;
y = y;
y = z;

z = x;
z = y;
z = z;

[CyrusNajmabadi]

I would not do this check.

Instead, just make the call to getTypeFromSomething(parameter.type), and check if it is the string, number, or numberlike type.

Note: parameter.type may be null iirc.

[jbondc]

@DanielRosenwasser @CyrusNajmabadi Really good feedback

Added more tests, if an enum or type alias is used, the following is now shown:

Does that seem ok?

[JsonFreeman]

As @DanielRosenwasser hinted, doing this change properly actually involves a lot of downstream changes in the type checker to get all the expected behavior. It is not a simple matter of allowing it.

[jbondc]

@JsonFreeman Ya noticing now, could use some help. There's a bug in getIndexTypeOfType() somewhere:

[JsonFreeman]

That is the expected behavior. You cannot assign something without a string indexer to something with a string indexer.

[jbondc]

@JsonFreeman The error "Index signature is missing in type {[x: number]: string}" is correct?

Not sure I understand why the reverse is allowed / not an error:
y = x

[JsonFreeman]

This is because indexer parameters are considered contravariant (where number is considered a subtype of string).

Think of an indexer as a contract that says the object can be indexed with a particular type. A string indexer means the object can be indexed with any key. A numeric indexer means that the object can be indexed only with a numeric key (like 0 or "0"). So putting a number indexer on an object is a weaker constraint than putting a string indexer on an object.

For y = x, you are taking something that can be indexed with any string (a very strong contract), and saying that you will only care about indexing into it with numeric keys. That's fine because it means you will only use a subset of the keys available, but it supports indexing with any key.

For x = y, you are taking something that only handles numeric keys, and saying that you may index into it with any key. So y might only be indexable by 0, 1, etc, but after assigning to x, you may try to index with "foo". That's why it is not safe.

Type theoretically, this is how function parameters should work as well. A function that takes a base type should be assignable to a function taking a more derived type, but not vice versa. However, typescript is rather loose about this, and for function parameters, the assignment is allowed in both directions.

[jbondc]

Thanks @JsonFreeman , makes a lot of sense.

So would these rules look correct?
string > number > enum<number>
string > enum<string>

e.g.

enum e {}
var x : { [x: number]: number } 
var y : { [x: e]: number } 

x = y; // ok
y = x; // should error 

It would be nice if this work would allow adding enum<string> later on.

[JsonFreeman]

Your comparisons above look correct, but that means x = y is an error and y = x is fine. So your assignability results should be the other way around.

I actually am somewhat doubtful of the value of this feature. I understand the appeal, that you want to be able to say that all keys of this enum map to the same type. But I feel like number and string represent well known sets of values, and the relationship between the two sets is known. Because string represents any string key, and number represents any numeric string key, we know that number keys are a subset of string keys. Now for enums, we know that each enum type is a subtype of number, but the sets of values represented by different enums can overlap in arbitrary ways. So does it make sense to allow two different indexers, one keyed by E1, and one keyed by E2? They might represent the exact same set of values, or one might be a subset of the other. But we have no way of enforcing that. It would only makes sense if all enums represented disjoint sets of values.

It also adds quite a bit of complexity in the type system in terms of how a type is represented. Instead of an object type just having call signatures, construct signatures, number indexer, and string indexer, an object type now can have an arbitrary number of kinds of signatures, which is actually quite a large infrastructural change.

So while I understand the appeal of this feature, and I get what you are trying to express, I am doubtful that it's as meaningful as it seems at first glance, and I think the cost is quite high in terms of impact on the type system.

[jbondc]

The context was this ~ JSON returned by an api:

enum Positions {
   center,
   right,
}
var statsByPosition = {"0": {count: 10, goals: 5, assists: 2},
                                 "1" : {count: 0}};

// Or recently
enum VotePoints {
    None = 0,
    Love = 2
}
interface Thing {
    // Vote counts by # points
    votes: { [x: VotePoints]: number }
}

It allows to model & document the expected data better. Agree it shouldn't come at the cost of too much complexity. Is it simpler to reject the assignment comparison of two enums & stay safe?

[JsonFreeman]

It is definitely best to reject assignment comparisons of two enums. We don't know anything about them, so we should assume they are not compatible.

I think the cost is more about trafficking around the information about the various indexers of a type. I guess you can have a list of enum indexers in the same way that you have a list of call signatures. And then you'd have to visit all the places that we do anything with indexers, and make sure enum indexers are behaving the same way.

What would happen with an indexer whose key is a union of two enums?

[jbondc]

Would adding a 'numberIndexerReferenceName' or 'stringIndexerReferenceName' as the declared name of the enum work? The name would seem enough to format numberIndexerReferenceName<number> or stringIndexerReferenceName<string> and tell if something is a subset/complex enum to compare.

if(source.numberIndexerReferenceName && target.numberIndexerReferenceName && source.numberIndexerReferenceName !== target.numberIndexerReferenceName )
  error(cant_assign_different_enums)

Didn't follow the union part... like votes: { [x: VotePoints|VotePoints2]: number }?

[JsonFreeman]

Yes, that is what I mean by "union of two enums".

I am not sure what you mean about the reference names.

[jbondc]

Never mind about 'reference names', need to do more head scratching. I think what's missing is two types:
'stringSubset'
'numberSubset'

getApparentType(enumOfIndexSignature) would return the NumberSubset.

Thinking it's just the 'Number' type with a flag TypeFlags.Subset
So 'Subset' types are incompatible since we can't compare them.

[jbondc]

There are two ways to declare a number 'Subset' type:

• Enum declaration: enum foo {a,b,c}
• Type alias with a where clause: type a = number where <T in[foo,5,6]>; // could support this

There are two ways to declare a string 'Subset' type:

• Enum declaration: enum<string> bar {a,b,c}
• Type alias with a where clause: type b = string where <T in[foo,d,e]>; // could support this

@JsonFreeman what do you think?

[JsonFreeman]

So there would be two number types, one that is a subset and one that is not? What is the relationship between those two types? How do you know when to use one vs another?

Also, you still have the problem of determining how the subsets are related to each other.

[jbondc]

@JsonFreeman Have a look at the new patch, there's a few quirks to work around still but importantly:
https://github.com/jbondc/TypeScript/blob/436b0f64980ac53f65eec21653902d79e10f7fc6/src/compiler/types.ts#L1489

Not sure how subsets should be related but think it would work well.

[jbondc]

@JsonFreeman There's a lot of similarity between union types & enums, some thoughts here:
https://gist.github.com/jbondc/fa3eb93781dec825c921

Likely lots of cases to work out. If the source type is a subset type & target type is a number, would be nice to check if the value is in the set. Have some experiments to do this by attaching a constraint to the symbol, in this case a 'set constraint' that holds the possible values. So a symbol has a type + constraints, the constraints have nothing to do with type safety.

[JsonFreeman]

I think the question of more specific indexers would best be revisited when we can look at it holisitically with #1003. I have a feeling these two features could be intimately related, and it'd be a shame to add infrastructure for the indexers without having a plan for singleton types, and unions of singleton types.

[mhegazy]

can you please remove the bin folder from this PR. we do not do that in branches.

[mhegazy]

ok now i see. u r using it later on. still i would do that in different changes.

[mhegazy]

Thanks @jbondc for the PR. at this point the change seems too complicated given the scenario it aims to support. it also seems that #1003 is a prerequisite to this change, and will make this feature more natural and provide more value. At this point i would hold off on the change until #1003 is in place and revisit it then.
As a side note it is easier for us to take small incremental changes than to take bigger ones. It is also usually helpful to discuss the details of the design first before jumping into implementation as it allows everybody on this side to be aware of the concepts as they are reviewing the changes.

[jbondc]

Frustrating to say the least, for reference updated here:
master...jbondc:issue-2491

[jbondc]

As far as discussing details of the design, that seems to fail too, see #311. Very little feedback there.

[danquirk]

@jbondc re:feedback, we have finite time, do know we are looking at essentially every comment made on a daily basis even if we don't always write lengthy responses. I spend around an hour a day (or more) reading/triaging every GitHub notification to the repo, more time than I spend on my work email. On that thread in particular Ryan responded not that long ago saying there was a clear proposal but that we have higher priority features in flight in the near term before we can evaluate possible options in depth (#311 (comment)). We've talked a lot about mixins/traits/intersections types across a few different issues (also with discussions in the team room/design meetings) and they are absolutely affected by the feedback you all have given. We simply can't prioritize features like that one over ES6 compatibility at the moment and that obviously effects the amount of time we can/should devote to collaborate on designing them in detail (and there're more than a few other feature suggestions in a similar boat with a lot of traffic too, we track them all closely). This doesn't mean they're not important to us or we don't intend to take time to digest all the opinions in there at a future date. All that said, certainly open to suggestions for way to improve the process.

I will say that personally I find GitHub's commenting system starts to break down (in terms of UX) as threads get particularly long. It's possible that for feature requests like that one (high value but high complexity so they exist for awhile generating lots of feedback) at some point we should try to consolidate the feedback/proposal(s) into a new issue to simplify and reset the thread context so that it's easier for everyone to get back on board with the discussion. Otherwise when a new comment is made on a thread like that after some amount of time away it's difficult for me to easily add comments that aren't rehashing what's already been discussed, which then exacerbates the problem further for all future commenters (including future me).

Right now that thread is fairly deep with multiple different experiments/implementations linked, multiple research papers linked, multiple existing systems to draw inspiration from linked. That's a lot of context to gather for someone trying to jump into the conversation and make a meaningful contribution. When it comes time to really dig in all that context is very valuable, but in the meantime it can cause what understandably appears to be more of a lack of interest/feedback. Perhaps we need to consider ways to generate more self contained questions within a thread like that which doesn't require someone parse which other comments are and aren't relevant to the question at hand.

[jbondc]

Fair enough, great answer. What could help is a list of 'next design priority' items as part of the roadmap.
Is #1003 design priority 6 months away, 1 year?
Is #311 design priority 6 months away, 1 year, more of a priority than #1003 ?
Maybe allowing the community to rank/vote on them since github doesn't allow it or use uservoice.com.

👍 to all the amazing progress on ES6

ps: async,traits,string-number enums are high on my list.

[danquirk]

Yeah, I really wish GitHub would add some sort of upvote facility so that people don't have to make noisy +1 comments and we don't have to use some other site for the functionality. I know GitHub has received that request numerous times... (including from me/us).

Assessing the likelihood of certain features making it in in x months is obviously difficult. We do try to use the roadmap page for that to some degree but obviously it is imperfect and has less and less content as you move forward in time (safe to say there are going to be more than 3 items in 2.0). Although as you can see there we do have one item that is evaluating the top requested features (which we are constantly doing and trying to fit in where we can, ex protected). The problem of course is I could tell you that #1003 is a priority in x months but then when we dig into the design it turns out there are much harder problems than we first envisioned, or the design is ok but we can't get an implementation with performance characteristics we're happy with, or it was a priority but then a new, unforeseen opportunity arose that we decided is actually more important in the near term (ex Angular support).

One option would be to provide a stack ranked list of features so you can at least have a relative sense of how important we see doing feature x vs feature y. In some ways this is what UserVoice is except it's stack ranked by community votes and not the development teams' priorities/realities. But that still has issues where it doesn't guarantee that the top of the stack is actually the next feature we do if for example we realize that the next release could either deliver our top feature request (some very large, expensive feature that 20% of people will love) or top features 2 through 10 (all small, well understood changes that everyone will benefit from more often). That said, I think we do have a pretty good idea for the top ranked features people want, although maybe in a slightly less precise way than a clearly ordered list everyone can read top down.

I'll see about having a talk about options to try to surface future plans better when possible.