3 ms·

it's not a trick. Yes, it can be casted, but you should be explicit about it. AFAIK, the signature for the concat function is dependent on the array you're usin

it's not a trick. Yes, it can be casted, but you should be explicit about it. AFAIK, the signature for the concat function is dependent on the array you're using it, so in this case, it's Array<Leaves>. Even though concat returns a new array, it is expecting the arguments to be of similar type. That's a "safe" behavior and something I would expect. Auto-casting to the return type (in this case) is something that I would not expect to happen, as it could be the source of errors. Doing [].concat(Array<A>, Array<B>) is fine, because you did not specify the type of the array for the literal [], it's not a trick, in that case the type should be Array<any>, but it's casted to Array<MemsqlNode>.

funkaster 8y ago
Looking at the type definition, it is expecting the same type: /** * Combines two or more arrays. * @param items Additional items to add to the end of array1. */ concat(...items: ConcatArray<T>[]): T[]; /** * Combines two or more arrays. * @param items Additional items to add to the end of array1. */ concat(...items: (T | ConcatArray<T>)[]): T[];
- dunham 8y ago
  That's typescript's type definition. Flow recognizes that concat returns a new array, so it specifies that calling concat on an Array<T> with an Array<S> as an argument returns an Array<T|S>. Their actual definition is: declare class $ReadOnlyArray<+T> { // concat creates a new array concat<S, Item: $ReadOnlyArray<S> | S>(...items: Array<Item>): Array<T | S>; Personally, I'm ok with that and think it is useful. Although the return value could be an array of any type more general than T|S and the types of the argument arrays could vary. (i.e. Array<T>.concat(Array<S1>,Array<S2>,...): Array<? extends (T|S1|S2)>) but I don't know if that can be expressed in flow. I don't think either gets it perfect, but flow is trying to at least capture the fact that the return value can't be an array of a type that is disjoint from T|S. In practice, I've been impressed by the level of detail of information that is captured and propagated by flow's type checker.
munchor 8y ago
Right but isn't that effectively changing the runtime code (thus making it a trick) in order to fix a type error? I don't know if the performance of `a.concat(b)` is the same as `[].concat(a, b)`.
- funkaster 8y ago
  It's not a type error. I think the error is in your definition of a function and types like that. I would've defined your types like in this example[0]. However, I don't know enough about your codebase or why you're doing this. [0]: http://www.typescriptlang.org/play/#src=type%20NodeType%20%3D%20%22LEAF%22%20%7C%20%22AGGREGATOR%22%3B%0D%0A%0D%0Atype%20MemsqlNode%20%3D%20%7B%0D%0A%20%20host%3A%20string%3B%0D%0A%20%20port%3A%20number%3B%0D%0A%20%20type%3A%20NodeType%3B%0D%0A%7D%0D%0A%0D%0Atype%20Leaf%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22LEAF%22%3B%0D%0A%7D%0D%0A%0D%0Atype%20Aggregator%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22AGGREGATOR%22%3B%0D%0A%7D%0D%0A%0D%0Afunction%20f(leaves%3A%20Array%3CLeaf%3E%2C%20aggregators%3A%20Array%3CAggregator%3E)%3A%20Array%3CMemsqlNode%3E%20%7B%0D%0A%20%20return%20(leaves%20as%20Array%3CMemsqlNode%3E).concat(aggregators)%3B%0D%0A%7D%0D%0A http://www.typescriptlang.org/play/#src=type%20NodeType%20%3...
- WorldMaker 8y ago
  The argument is that you have a potential type error in `a.concat(b)` that you hadn't considered before in that code: If `a` is defined everywhere as Array<Leaf> and you push() something to it that is not a Leaf you potentially broke one of your own invariants somewhere else that uses `a`. In JS concat happens in place and is a mass push, so the same general concept holds.
  - dunham 8y ago
    `concat` doesn't happen in place, it builds a new array: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat
    - WorldMaker 8y ago
      Interesting. Then that should be a fixable bug in TS' lib files then.

funkaster 8y ago

Looking at the type definition, it is expecting the same type: /** * Combines two or more arrays. * @param items Additional items to add to the end of array1. */ concat(...items: ConcatArray<T>[]): T[]; /** * Combines two or more arrays. * @param items Additional items to add to the end of array1. */ concat(...items: (T | ConcatArray<T>)[]): T[];

dunham 8y ago
That's typescript's type definition. Flow recognizes that concat returns a new array, so it specifies that calling concat on an Array<T> with an Array<S> as an argument returns an Array<T|S>. Their actual definition is: declare class $ReadOnlyArray<+T> { // concat creates a new array concat<S, Item: $ReadOnlyArray<S> | S>(...items: Array<Item>): Array<T | S>; Personally, I'm ok with that and think it is useful. Although the return value could be an array of any type more general than T|S and the types of the argument arrays could vary. (i.e. Array<T>.concat(Array<S1>,Array<S2>,...): Array<? extends (T|S1|S2)>) but I don't know if that can be expressed in flow. I don't think either gets it perfect, but flow is trying to at least capture the fact that the return value can't be an array of a type that is disjoint from T|S. In practice, I've been impressed by the level of detail of information that is captured and propagated by flow's type checker.

munchor 8y ago

Right but isn't that effectively changing the runtime code (thus making it a trick) in order to fix a type error? I don't know if the performance of `a.concat(b)` is the same as `[].concat(a, b)`.

funkaster 8y ago
It's not a type error. I think the error is in your definition of a function and types like that. I would've defined your types like in this example[0]. However, I don't know enough about your codebase or why you're doing this. [0]: http://www.typescriptlang.org/play/#src=type%20NodeType%20%3D%20%22LEAF%22%20%7C%20%22AGGREGATOR%22%3B%0D%0A%0D%0Atype%20MemsqlNode%20%3D%20%7B%0D%0A%20%20host%3A%20string%3B%0D%0A%20%20port%3A%20number%3B%0D%0A%20%20type%3A%20NodeType%3B%0D%0A%7D%0D%0A%0D%0Atype%20Leaf%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22LEAF%22%3B%0D%0A%7D%0D%0A%0D%0Atype%20Aggregator%20%3D%20MemsqlNode%20%26%20%7B%0D%0A%20%20type%3A%20%22AGGREGATOR%22%3B%0D%0A%7D%0D%0A%0D%0Afunction%20f(leaves%3A%20Array%3CLeaf%3E%2C%20aggregators%3A%20Array%3CAggregator%3E)%3A%20Array%3CMemsqlNode%3E%20%7B%0D%0A%20%20return%20(leaves%20as%20Array%3CMemsqlNode%3E).concat(aggregators)%3B%0D%0A%7D%0D%0A http://www.typescriptlang.org/play/#src=type%20NodeType%20%3...
WorldMaker 8y ago
The argument is that you have a potential type error in `a.concat(b)` that you hadn't considered before in that code: If `a` is defined everywhere as Array<Leaf> and you push() something to it that is not a Leaf you potentially broke one of your own invariants somewhere else that uses `a`. In JS concat happens in place and is a mass push, so the same general concept holds.
- dunham 8y ago
  `concat` doesn't happen in place, it builds a new array: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat
  - WorldMaker 8y ago
    Interesting. Then that should be a fixable bug in TS' lib files then.