Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.x: compose() generics #4950

Closed
ZacSweers opened this issue Jan 3, 2017 · 4 comments
Closed

2.x: compose() generics #4950

ZacSweers opened this issue Jan 3, 2017 · 4 comments

Comments

@ZacSweers
Copy link
Contributor

ZacSweers commented Jan 3, 2017

Migrating some of our code to RxJava 2, we've run into some issues around transformers due to the following signature change:

// 1
public <R> Observable<R> compose(Transformer<? super T, ? extends R> transformer) {
    
    return ((Transformer<T, R>) transformer).call(this);

}

// 2
public final <R> Observable<R> compose(ObservableTransformer<T, R> composer) {
    
    return wrap(composer.apply(this));

}

Was this intentionally narrowed? If not, open to a PR to add back the ? super semantics?

@akarnokd
Copy link
Member

akarnokd commented Jan 3, 2017

Do you have an example that doesn't compile for you without ? super? We have to be careful with such variance changes because it may break current users.

@ZacSweers
Copy link
Contributor Author

Whoops just saw your response. I'll put together a sample later tonight

@ZacSweers
Copy link
Contributor Author

ZacSweers commented Jan 8, 2017

So here's an example where an observable emits A instances down the stream and transforms them into B instances.

interface A<T, R> {}
interface B<T> {}

// RxJava 1
static <T> rx.Observable.Transformer<A<T, ?>, B<T>> oldStyle() {
    return new rx.Observable.Transformer<A<T, ?>, B<T>>() {
        @Override
        public rx.Observable<B<T>> call(rx.Observable<A<T, ?>> a) {
            return rx.Observable.empty();
        }
    };
}

// RxJava 2
static <T> ObservableTransformer<A<T, ?>, B<T>> newStyle() {
    return new ObservableTransformer<A<T, ?>, B<T>>() {
        @Override
        public ObservableSource<B<T>> apply(Observable<A<T, ?>> a) {
            return Observable.empty();
        }
    };
}

void test() {
    
    A<String, Integer> a = new A<String, Integer>() { };

    // RxJava 1
    rx.Observable.just(a)
            .compose(TransformersTest.<String>oldStyle())  // Yay, because Integer is irrelevant here
            .subscribe(new Action1<B<String>>() {
                @Override
                public void call(B<String> stringB) {

                }
            });

    // RxJava 2
    Observable.just(a)
            .compose(TransformersTest.<String>newStyle())   // This doesn't compile
            .subscribe(new Consumer<B<String>>() {
                @Override
                public void accept(B<String> tB) throws Exception {

                }
            });
}

In the RxJava 2 example, the compose() line there doesn't compile because of generics issues.

screen shot 2017-01-07 at 11 55 01 pm

In order to make it compile, the transformer signature needs to be changed to be like the following:

static <T, R> ObservableTransformer<A<T, R>, B<T>> newStyle() {
    return new ObservableTransformer<A<T, R>, B<T>>() {
        @Override
        public ObservableSource<B<T>> apply(Observable<A<T, R>> a) {
            return Observable.empty();
        }
    };
}

And imposes some boilerplate on the consumer in that they now have to specify the second type even though it's irrelevant.

Observable.just(a)
        .compose(TransformersTest.<String, Integer>newStyle())  // :(
        .subscribe(new Consumer<B<String>>() {
            @Override
            public void accept(B<String> tB) throws Exception {

            }
        });

@akarnokd
Copy link
Member

akarnokd commented Jan 8, 2017

Okay, please verify that with the variance changes it compiles with Java 6, 7 and 8 targets.

ZacSweers added a commit to ZacSweers/RxJava that referenced this issue Jan 8, 2017
akarnokd pushed a commit that referenced this issue Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants