API Design Review: Varargs

[benjchristensen]

There is dislike of the multiple arities of from... especially how the 10-arity option always comes up and autocompletes first.

For history, changes from vararg to multiple arity overloads was done here: #361 and #359

An alternative is just accept from(Iterable<T>) and people call it as from(Arrays.asList(1, 2, 3, 4, 5)) which doesn't give the vararg warnings, but this is verbose.

On the flip side, the arities from 4-10 seem unnecessary and for "demo code" only.

Perhaps we deprecate the from(T, T, T) and higher arities? For higher we would use the from(Iterable<T>) solution?

Are there better ways?

/cc @headinthebox and @jhusain

NOTE: This affects from, merge, mergeDelayError and concat.

[akarnokd]

Maybe we should only keep from(T) and deprecate the higher arities and just as well.

[headinthebox]

Java 8 streams use of(T t) and of(T... values). Not sure if we mustopen the can of worms of varargs, but the Oracle folks don't seem to shy away from it.

[akarnokd]

The SafeVarargs annotation is on their side.

[benjchristensen]

The @SafeVarargs definitely solves the issue. Need to figure out if we can adopt @SafeVarargs and build with Java 7 just for that but leave everything else as Java 6 so Android support is not affected.

[benjchristensen]

I just learned that @SafeVarargs doesn't solve this problem when nested generics are involved.

In this screenshot, note the first one is still causing warnings whereas the second is not. The Observable having generics causes the problem despite Arrays.asList using @SafeVarargs.

[benjchristensen]

It looks like we have 3 choices:

1. Leave code as is with the 9 arity overloads for from, merge and concat.
2. Remove all the arity overloads and make developers do something like Arrays.asList to pass into a single Iterable overload. Devs end up with the same vararg warnings.
3. Use varargs and just deal with the warnings (the same as if we go with option 2)

[mttkay]

If these are the only options, I'd vote for 3, purely because I generally dislike approach 1 (violates the zero-one-infinity rule [1]) and because 2 adds nothing over 3 except verbosity.

[1] http://www.catb.org/jargon/html/Z/Zero-One-Infinity-Rule.html

[benjchristensen]

I too vote for 3 as it makes the API far cleaner (especially if we end up needing to provide Scheduler overloads for merge and mergeDelayError as a result of this: #998 (comment))

[benjchristensen]

@samuelgruetter @akarnokd @zsxwing What are your opinions on this matter?

[akarnokd]

Use the builder pattern instead of varargs.

Observable.<TCommon>fromBuilder().add(v1).add(v2).add(v3).build().map().etc.

[benjchristensen]

I'd rather keep what we have right now than that.

Observable.from(v1, v2, v3).map()

[samuelgruetter]

Two more ideas:

• Introduce an add (or append or endWith) method, so that we could use something like

Observable.<TCommon>just(v1).add(v2).add(v3).map().etc.

This is similar to @akarnokd's idea above, but a bit less verbose, and a bit less efficient. And if v1, v2, ... all have the same type, we can even omit the <TCommon>.
Or if we keep the low arity from, this could be the escape if we still need higher arities:

Observable.from(v1, v2, v3).add(v4).add(v5).map().etc.

• Separate methods for low arities and varargs for high arities:

public final static <T> Observable<T> from(T t1)
public final static <T> Observable<T> from(T t1, T t2)
public final static <T> Observable<T> from(T t1, T t2, T t3)    
public final static <T> Observable<T> from(T t1, T t2, T t3, T t4, T... ts)

This covers everything except cases with 4 or more arguments of generic type, which should be quite rare.

[headinthebox]

By the time you are at such high arity it is easier to put the values in an array and convert that. This kind of code is only used in small samples. from(1, 2) or anything you can enumerate statically is not a very interesting "asynchronous data stream".

(and so far we have been able to keep the builder pattern out,let's keep it that way).

[headinthebox]

So let's go for option 3.

[benjchristensen]

So @headinthebox you think we should go completely vararg and just deal with the warnings that drove us to the current code? Or that we should keep the 1, 2 and 3 arity overloads and then 4+ is with varargs?

[benjchristensen]

By the way, this isn't just for from, the common use cases are for merge, concat, mergeDelayError etc.

[headinthebox]

you think we should go completely vararg

Yes.

[samuelgruetter]

Also go completely vararg for concat? In concat, the arguments are always generic, so users will always get a warning (see #359).

[headinthebox]

Annoying yes, but the Java type system is a freaking mess anyway (and just a mirage because of erasure).

BTW, can we put @SuppressWarnings("unchecked") in the library source to suppress warnings for users?

[benjchristensen]

No, the SuppressWarnings is for the call site, not the function definition.

The SafeVarargs annotation is a Java 7 feature for the function definition, and it works for simple types like T but not nested generics (which Java also has issues with) such as Observable<T> which is what concat and others require. Therefore, even if we could adopt Java 7, SafeVarargs is helpful for from but not concat, merge, etc.

[benjchristensen]

My comment above at #686 (comment) shows when I found out that SafeVarargs wouldn't solve the issue.

[headinthebox]

What a mess. So the alternative is many mechanical overloads with separate type parameters f<A,....Z>(....)?

[benjchristensen]

The alternative is what we have right now ... manually creating all of the overloads. It makes it so it doesn't give warnings, but it means we have lots of overloads and painful autocomplete in IDEs.

So we either have warnings in the IDE, or autocomplete mess. We have to pick a poison it seems.

I don't know which is worse, it's probably a personal opinion.

[headinthebox]

Agreed. I guess many people treat "warnings as errors" we should probably go for autocomplete mess. It is unfortunate that there (seems to be) no way in the Java IDEs to control autocomplete. In Visual Studio you can decorate your API to control what pops up.

[benjchristensen]

I'm going to close this out and choose to leave the code as is, opting to avoid warnings and leave the autocomplete mess.