JsArray.concat has incorrect generics

[niloc132]

In both JsArray and ReadonlyArray, concat() is specified as

  public native JsArray<T> concat(T... items);

In closure's externs however, it is defined as
https://github.com/google/closure-compiler/blob/39363c2c57dcdc365c739ec3f13d4e10ef9a204c/externs/es3.js#L830-L841

/**
 * Returns a new array comprised of this array joined with other array(s)
 * and/or value(s).
 *
 * @param {...*} var_args
 * @return {!Array<?>}
 * @this {*}
 * @nosideeffects
 * @override
 * @see http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat
 */
Array.prototype.concat = function(var_args) {};

This is an important loss of functionality, as concat should be able to take an array and iterate its contents, rather than only accepting a varargs of the same contents.

For example, given two gigantic arrays:

const oneMillionApples = "apples".repeat(1000000).slice(0,-1).split(",");
const oneMillionOranges = "orange".repeat(1000000).slice(0,-1).split(",");

In plain JS, you can concat these - the arrays are the same type, and we don't need to do something like array destructuring, which would blow the stack

const tooManyItems = oneMillionApples.concat(oneMillionOranges);

but given the current type signature of concat() in Java, this can't work - concat's args must be varargs of T. Trying to treat the JsArray as String[] will mean we have to translate java varargs semantics to js varargs semantics, and blow the stack.

Contrast this with the closure types, which seem to allow any args at all (since an array can be heterogeneous).

[jDramaix]

I agree we are loosing functionality here as you should be able to:

• concat two array of different types
• directly pass a JsArray to avoid array spreading.

So I believe we should emit these APIs for concat instead of the current one:

  public native <R,V> JsArray<R> concat(V... items);
  public native <R,V> JsArray<R> concat(JsArray<V> array);

This can be implemented in changing the code in BuiltInClosureTypeCleaner within the cleanArrayConcatMethod

WDT ?

[niloc132]

Technically I think your second method is unnecessary, since <V> isn't bound and that single JsArray would count as a single argument. It is more correct to allow multiple arrays too. That is, no array spreading should take place if V is some Object[] or JsArray<?>.

It does have the disadvantage of not restricting R to be a common supertype of T and V - but your pair of methods at least gives us the ability to resolve V as the items in the array, though it would obviously be better if we could take advantage of that.

If I'm not mistaken, Closure's types are vague enough that you can do something (in java) like

JsArray<Double> numbers = ...;
JsArray<JsDate> dates = ...;
JsArray<String> strings = numbers.concat(dates);

In a perfect world, if Java would let us define V as super T or something, we could easily get

JsArray<Object> both = numbers.concat(dates);

or even

JsArray<Object> both = numbers.concat(dates, strings);

but instead I think we might instead consider just avoiding unbound V and R and stick with T as V extends T like the current code does and return JsArray, so we at least can do

JsArray<Object> strings = ...;
JsArray<Number> numbers = ...;
JsArray<Object> result = strings.concat(numbers);

This is "wrong", but at least is a familiar problem to Java developers (Collection.addAll, Stream.concat), and gives a way to get to what they want.

Keeping both V and R means the developer needs to be more explicit in some cases... but the user can specify "V is JsArray and I want to pass two string arrays" and insist that out the other side R will just be String. Offering T/V/R seems to imply that it will be typesafe, but I'm not sure that can actually be achieved.

Its too bad that Array.prototype.concat doesn't recursively unwrap arrays, we could make the user be explicit about array creation to avoid the spread issue. It does make me think about encouraging multiple methods as you've done here - but Java generics could start to bite if we offer JsArray<T>... vs T.... This then in turn suggests renaming the java method to something else, and using @JsMethod to rename back to concat, strawman:

@JsMethod(name="concat")
public native <V extends T> JsArray<V> concatArray(V[]... arrays);
@JsMethod(name="concat")
public native <V extends T> JsArray<V> concatJsArray(JsArray<V>... arrays);
@JsMethod(name="concat")
public <V extends T> JsArray<V> concatItems(V... items);

Unambiguous, but awkward - existing concat() could still exist to avoid breaking code, but perhaps with javadoc and deprecation to encourage caution from surprise stackoverflows. Since V extends T, we can know that no one is going to stick arrays of arrays into a string array, and the last one could be an overlay method that passes the java varargs as an array (like Promise.all/allInternal).

I'm probably missing another path forward - for sure I don't love any of these enough to want to commit to one. Sorry that this rambles a bit.

[zbynek]

Maybe

public native <V extends T> JsArray<T> concat(JsArray<V>... array);
public native JsArray<T> concat(T single);

is enough?

Then the common usecases can be coded like this:

JsArray<String> apples, oranges, pears;
// ... initialize ...
JsArray<String> fruits = apples.concat(oranges, pears);
JsArray<String> with2bananas = apples.concat("banana").concat("banana");
String[] cherries = new String[]{"cherry", "cherry"};
JsArray<String> withCherries = apples.concat(JsArray.of(cherries));

One could also follow the pattern used by List.of where special cases are used for small number of arguments and do

public native <V extends T> JsArray<T> concat(JsArray<V>... array);
public native JsArray<T> concat(T t1);
public native JsArray<T> concat(T t1, T t2);
public native JsArray<T> concat(T t1, T t2, T t3);