Covariant Generics Issues

[benjchristensen]

After the recent super/extends additions there are some areas where it doesn't quite feel right in how it's working.

This issue has come up at #336 (comment) as well as work I'm doing related to #359

I want to use this issue to track issues and discuss solutions.

[benjchristensen]

One issue is with concat:

public static <T> Observable<T> concat(Observable<Observable<? extends T>> observables)

That works with this:

    public void testConcatCovariance() {
        Observable<Media> o1 = Observable.from(new HorrorMovie(), new Movie());
        Observable<Media> o2 = Observable.from(new Media(), new HorrorMovie());

        Observable<Observable<Media>> os = Observable.from(o1, o2);

        List<Media> values = Observable.concat(os).toList().toBlockingObservable().single();
    }

but it doesn't work with this:

    public void testConcatCovariance2() {
        Observable<Media> o1 = Observable.from(new HorrorMovie(), new Movie(), new Media());
        Observable<Media> o2 = Observable.from(new Media(), new HorrorMovie());

        Observable<Observable<Media>> os = Observable.from(o1, o2);

        List<Media> values = Observable.concat(os).toList().toBlockingObservable().single();
    }

When both types are Media the Observable ends up being Observable<Observable<Media>> instead of Observable<Observable<? super Media>> and then concat fails. This is not at all ideal, especially since it's more common to be concatenating multiple Observables of the same type.

If I change concat to this and remove the ? extends:

public static <T> Observable<T> concat(Observable<Observable<T>> observables)

... the testConcatCovariance2 example works and testConcatCovariance1 fails unless I change this line:

Observable<Media> o1 = Observable.<Media>from(new HorrorMovie(), new Movie());

Personally I prefer doing that when dealing with super types than making concat be forced to deal with it all the time.

If you skip to Observable.from(o1, o2) step which confuses the generics and go directly to concat then it's even cleaner.

This uses:

public static <T> Observable<T> concat(Observable<? extends T> t1, Observable<? extends T> t2)

Both of these work:

    @Test
    public void testConcatCovariance3() {
        Observable<Movie> o1 = Observable.from(new HorrorMovie(), new Movie());
        Observable<Media> o2 = Observable.from(new Media(), new HorrorMovie());

        List<Media> values = Observable.concat(o1, o2).toList().toBlockingObservable().single();
    }

    @Test
    public void testConcatCovariance4() {

        Observable<Movie> o1 = Observable.create(new OnSubscribeFunc<Movie>() {

            @Override
            public Subscription onSubscribe(Observer<? super Movie> o) {
                o.onNext(new HorrorMovie());
                o.onNext(new Movie());
                //                o.onNext(new Media()); // correctly doesn't compile
                o.onCompleted();
                return Subscriptions.empty();
            }
        });

        Observable<Media> o2 = Observable.from(new Media(), new HorrorMovie());

        List<Media> values = Observable.concat(o1, o2).toList().toBlockingObservable().single();
    }

In short, it seems the Observable<Observable<? extends T>> does not work well.

[benjchristensen]

See benjchristensen@1a0fcdc for the changes made to Observable.concat due to the above finding in relation to #359. If someone can provide a better solution I'd appreciate it.

[benjchristensen]

Looks like this makes it work:

public static <T> Observable<T> concat(Observable<? extends Observable<? extends T>> observables) 

[benjchristensen]

See benjchristensen@97b51eb for this change.

[benjchristensen]

I think concat is fine as of #361

Need to look at reduce as reported at #336 (comment)

[benjchristensen]

The issues with reduce are shown at https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/test/java/rx/ReduceTests.java#L58 and https://github.com/Netflix/RxJava/blob/master/rxjava-core/src/test/java/rx/ReduceTests.java#L83

[benjchristensen]

I have not yet figured out a solution to reduce that retains ? super on the instance method version. I would appreciate someone else taking a look.

[benjchristensen]

@jmhofer can you give an example where the instance reduce method actually benefits from the wildcards?

public Observable<T> reduce(Func2<? super T, ? super T, ? extends T> accumulator)
... vs ...
public Observable<T> reduce(Func2<T, T, T> accumulator)

I can get covariance to work on it as a static method, but not as an instance method.

[benjchristensen]

Here is example code separate from Rx:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import rx.util.functions.Func1;

public class CovariancePlayground {

    public static class TestContainer<T> {

        private final List<T> items;

        public TestContainer(T... items) {
            this(Arrays.asList(items));
        }

        public TestContainer(List<T> items) {
            this.items = items;
        }

        public TestContainer<T> doSomethingToIt(Func1<? super T, ? extends T> function) {
            ArrayList<T> newItems = new ArrayList<T>();
            for (T t : items) {
                newItems.add(function.call(t));
            }
            return new TestContainer<T>(newItems);
        }

        public <R> TestContainer<R> transformIt(Func1<? super T, ? extends R> function) {
            ArrayList<R> newItems = new ArrayList<R>();
            for (T t : items) {
                newItems.add(function.call(t));
            }
            return new TestContainer<R>(newItems);
        }
    }

    public static class UnitTest {

        public void test() {
            TestContainer<Movie> movies = new TestContainer<Movie>(new ActionMovie(), new HorrorMovie());
            TestContainer<Media> media = new TestContainer<Media>(new Album(), new Movie(), new TVSeason());

            movies.doSomethingToIt(movieFunction);
            movies.doSomethingToIt(mediaFunction); // doesn't compile

            //            media.doSomethingToIt(movieFunction); // shouldn't be possible
            media.doSomethingToIt(mediaFunction);

            movies.transformIt(mediaToString);
            media.transformIt(mediaToString);
        }

        Func1<Movie, Movie> movieFunction = new Func1<Movie, Movie>() {

            @Override
            public Movie call(Movie t1) {
                return t1;
            }

        };

        Func1<Media, Media> mediaFunction = new Func1<Media, Media>() {

            @Override
            public Media call(Media t1) {
                return t1;
            }

        };

        Func1<Media, String> mediaToString = new Func1<Media, String>() {

            @Override
            public String call(Media t1) {
                return t1.getClass().getName();
            }

        };
    }

    static class Media {
    }

    static class Movie extends Media {
    }

    static class ActionMovie extends Movie {
    }

    static class HorrorMovie extends Movie {
    }

    static class Album extends Media {
    }

    static class TVSeason extends Media {
    }
}

[abersnaze]

I remember reading in effective java that the only type that can satisfy the super T and extends T is just T.

[benjchristensen]

I think the reduce instance method fits this quote from Effective Java 2nd Edition page 136:

If an input parameter is both a producer and a consumer, then wildcard types will do you no good: you
need an exact type match, which is what you get without any wildcards.

I don't think we can get it to work since the publisher and consumer are the same type. Thus I think we should change it to just be the following:

public Observable<T> reduce(Func2<T, T, T> accumulator)

[benjchristensen]

Yes George, I think I just read that very quote and pasted it. Funny we both got to the same thing! Thanks for confirming my thoughts on this.

[benjchristensen]

Can someone else take a look at #369 and let me know if my understanding of generics are correct here?

I have changed reduce, aggregate and scan back to Func2<T, T, T> without ? super and ? extends because these functions produce and consume the same type, thus they must match and the wildcards serve no value.