Fix typescript types for typescript 2.4

[sandersn]

Typescript does a lot more analysis than previous versions. This (1) exposed some bugs in the types and (2) caused the compiler to run out of memory and crash. The crash, and many of the new checks, can be avoided by passing --noStrictGenericChecks but users of immutable shouldn't be required to do that.

Improved Checks

1. The return type of Stack.flatMap's mapper was incorrectly M but should be Iterable<M>.
2. Set and related types should extend Collection<never, T> not Collection<T, T>. That's because Collection.map (and flatMap and filter) assert that the returned collection has the same key type and a different value type. But Set.map returns Set<T>, which, if it extends Collection<T, T> changes both the key type and value type. Changing to Set<T> extends Collection<never, T> means that the key type never changes.

Workarounds

1. Map and related types have a flatMap method that is not a valid Collection.flatMap implementation because it changes the key type and value type, much like Set (see flatMap typescript type for Keyed, [Ordered]Map are not substitutable for Collection.flatMap #1283). However, here the implementation and the type are wrong, so I worked around the problem by letting Map.flatMap return Map<any, any> instead of Map<KM, VM> (the previous type) or Map<K, M> (a valid subtype of Collection.flatMap).
2. (Fixed in Typescript 2.5.3) In Typescript 2.4, the compiler spends too much time and memory checking the extends relationships between Collection and its subtypes. For example, OrderedMap extends Map extends Collection.Keyed extends Collection. The worst offender is toSeq since it refers to a subtype, Seq, which is then extended multiple times by further subtypes. This makes checking goes around in circles multiple times for each subtype. I chose to rely on upcoming optimisations in Typescript 2.5.3 that will short-circuit most of these cycles.

Both workarounds have multiple possible fixes; for example, workaround (1) could instead declare two overloads of flatMap, one with the Collection-compatible type and one with the current type. Workaround (2) could delete toSeq entirely from Collection, or change the return type of toSeq to any in Collection, or change the return type of toSeq in subtypes of Collection.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[samwgoldman]

We noticed the issue with Set and map, flatMap as well. (See #879)

[sandersn]

Looks like some doc-generation tooling doesn't understand never yet, so I changed Set to extend Collection<void, T> instead.

[sandersn]

The failing tests points out an additional bug with the definition of Set.has, which for other collections checks the key but for sets checks for the value.

I'm still working on running tests locally, but I don't know how to copy type-definitions/Immutable.d.ts to dist/immutable-nonambient.d.ts, which is what the tests refer to. I poked around the gulpfile and didn't find anything? Is this manual?

[sandersn]

Never mind, I eventually got it by running through the tasks in package.json

[emmanueltouzery]

this could be totally the wrong place to report this (or maybe not?) but I tried to port my immutables‑using typescript app to typescript@2.6.0-dev.20170826 and the immutables code from this pull request, and I get an unexpected behaviour.

See my commit:
emmanueltouzery/ng-typeview@86ab9e6

I would have thought the extra cast I added to make it build wouldn't be needed? Without it, it appears to infer type {} for Map<string,string>.get(string), instead of string|undefined and I have no idea why.

That cast was not needed with tsc 2.3 and the previous immutables TS bindings (tsc goes out of memory on this code with both 2.4.2 and 2.5RC).

[sandersn]

@emmanueltouzery the problem is that controllerNameToFilename: Map<{}, {}>. If you manually annotate it as Map<string, string>, the compiler error goes away. I'll check why the call to the map constructor doesn't infer the right type arguments.

[sandersn]

I tracked the problem back to the preceding declaration:

    const controllerNameToFilename =
        Map(
            viewInfos
                .filter(vi => vi.controllerName.isSome())
			          // JS files are not going to have a scope interface
			          // definition so they're not helpful. Also, we can
			          // get twice the same file: original TS & compiled JS.
			          // => keep only the original TS in that case.
			          .filter(vi => vi.fileName.toLowerCase().endsWith(".ts"))
                .map(vi => [vi.controllerName.some(), vi.fileName] as [string, string]));

Notice that I added as [string, string] there at the end. Without it, the return type is inferred as string[], and the wrong overload of map gets chosen, and controllerNameToFilename gets the type Map<{}, {}> instead of Map<string, string>. I'm not sure why this worked in previous versions of immutable, though.

[leebyron]

Happy to see work here, but some feedback:

• I think all of the changes with key: never aren't correct where previously they were correct.
• Ideally we can avoid returning any types. What we did for better flow support was just add explicit overrides of methods on the final concrete collection types.

[sandersn]

Here's an example of where code goes wrong without key: never:

let nset = Immutable.Set.of(1,2,3,4)
let ncol: Immutable.Collection<number, number> = nset;
nset.map(v => v.toString()).map((v, k) => k.length); 
ncol.map(v => v.toString()).map((v, k) => k.length); 

The last two lines produces Set { 1 } at runtime, but the last line has a type error because map(v => v.toString()) produces a Collection<number, string> not Collection<string, string>, so the types say that k: number in map((v, k) => k.length)

With key: never you are prevented from using the key with sets. You have to use the value instead. Both lines have a type error and have to change the second map to map(v => v.length).

I understand this is a break for Typescript 2.3 code that uses the keys of a Set today, so if you have guidance I would appreciate it. I'm still thinking about the second case; there might be a way to avoid breaks there.

[leebyron]

@sandersn thanks for the example, I think perhaps this would be better solved by a more specific type for map() that's aware of a new key type as well on Set (and SeqSet) rather than blocking the use of keys

[leebyron]

.

[sandersn]

OK, I restored the original behaviour for subclasses. I had to do this by adding overloads in the base classes (Seq, Collection, etc) to account for the inconsistent types coming from Set and Dictionary. Instead of a single more specific type, this allows overrides to override one of two different types.

Most of these overloads are reasonable, except for Collection.map, because Set.map has identical parameter types to Collection.map, just a different return type. So the compiler can't distinguish them and always chooses the first one. There I used a hack suggested by @DanielRosenwasser, which is a variadic function (...args: never[]): any. Anything is assignable to this, but it won't be called by mistake because values with type never are so rare.

[sandersn]

Also, overloads are not inherited, so only uses of the base Collection interface will see the bogus overload. Most people will only ever see the methods that go with the collection type they're using.

[leebyron]

Thanks for your help on this. I amended a few changes in the process of merging this, please let me know if any follow ups are necessary

[sandersn]

Nope, looks good. I forgot about the test changes for Set that no longer needed to be changed.