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

(When AGP 3.0 is released) Java 8 and assume desugar for android, or target Java 7? #10

Closed
ZacSweers opened this issue Dec 23, 2016 · 13 comments
Assignees
Milestone

Comments

@ZacSweers
Copy link
Collaborator

Need to make a call on this based on our integration and see how it fairs using this in Java 7 source.

@ZacSweers ZacSweers added this to the 0.1.0 milestone Dec 23, 2016
@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Dec 26, 2016

Been tinkering with this more. Copying over my notes shared elsewhere:

The generated lambdas do have to be decompiled by the IDE in the debugger, but they're pretty simple and easy to read once you understand the structure. Still not as nice as stepping through source though. Regular sources do show up in java 8 syntax. You even get the option to set a breakpoint on the lambda innards like normal java 8 code, except those breakpoints never actually get hit (which seems like a big minus).

The lack of being able to set a meaningful breakpoint seems pretty bad. Easy to trace back, once you hit one, but it's pretty much impossible to find the generated class that corresponds to a lambda without a breakpoint farther down the line. In light of that, I'm leaning toward not doing retrolambda.

For example: the following snippet

@Override
  public final void onError(Throwable e) {
    if (!isDisposed()) {
      dispose();
      try {
        onError.accept(e);  // Where onError was a method reference of a SingleObserver's onError
      } catch (Exception e1) {
        Exceptions.throwIfFatal(e1);
        RxJavaPlugins.onError(new CompositeException(e, e1));
      }
    }
  }

corresponds to this in the debugger when decompiled, where this is the actual backing implementation of the above onError Consumer object in byte code:

// $FF: synthetic class
final class AutoDispose$AutoDisposingSingleObserverCreator$$Lambda$4 implements Consumer {
    private final SingleObserver arg$1;

    private AutoDispose$AutoDisposingSingleObserverCreator$$Lambda$4(SingleObserver var1) {
        this.arg$1 = var1;
    }

    @Hidden
    public void accept(Object var1) {
        this.arg$1.onError((Throwable)var1);
    }

    public static Consumer lambdaFactory$(SingleObserver var0) {
        return new AutoDispose$AutoDisposingSingleObserverCreator$$Lambda$4(var0);
    }
}

Thoughts @tonycosentini @vinc3m1 @LukeStClair?

@tonycosentini
Copy link
Contributor

We should prioritize debugging and using this over development IMO. It's not a huge thing - Java 7 isn't a big deal to me.

@ZacSweers
Copy link
Collaborator Author

With desugar coming to the android toolchain, should we move to Java 8 and assume people will use desugar?

@ZacSweers ZacSweers self-assigned this Apr 29, 2017
@ZacSweers ZacSweers changed the title Java 8 + Retrolambda, or target Java 7? Java 8 and assume desugar for android, or target Java 7? Apr 29, 2017
@ZacSweers ZacSweers removed this from the 0.1.0 milestone Apr 29, 2017
@vinc3m1
Copy link

vinc3m1 commented Apr 30, 2017

I would wait until stable release, but after that sure.

@tonycosentini
Copy link
Contributor

What benefit does it give the library?

It seems like for consumers it adds a bit of friction for using this, no?

@ZacSweers
Copy link
Collaborator Author

It makes the library easier to read/write in by using the new syntax features. From a java library distribution standpoint - everyone writing a pure java project these days should really be on Java 8. On Android, with desugar, it should be zero friction since it all gets done at dexing.

@LukeStClair
Copy link
Contributor

LukeStClair commented May 1, 2017 via email

@ZacSweers
Copy link
Collaborator Author

I should have clarified - *when android gradle plugin 2.4 stable is released, not immediately.

@digitalbuddha
Copy link

I'm not sure if you can assume that everyone will upgrade to 2.4 stable as soon as it is released. What is the downside of using Retrolambda until better adoption of newer tools?

@ZacSweers
Copy link
Collaborator Author

In testing with this, I found that consuming retrolambda'd libraries in Java 7 projects (retrolambda'd or not) actually added quite a bit of friction. Due to how the weaving works, you get no sources (only decompiled class files), and trying to step through them in the debugger is pretty hit or miss. Not to mention you can't actually open said generated classes due to not having sources, so you can't just go open them and set a breakpoint manually either :|.

The desugar approach they're taking with the gradle plugin actually works around this by treating the entire project as java 8 until you compile, at which point it will also desugar any Java 8-targeting libraries compiled with it.

@ZacSweers ZacSweers changed the title Java 8 and assume desugar for android, or target Java 7? (When AGP 3.0 is released) Java 8 and assume desugar for android, or target Java 7? Jul 16, 2017
@ZacSweers ZacSweers modified the milestone: 1.0.0 Sep 10, 2017
@ZacSweers
Copy link
Collaborator Author

Another potential benefit: LifecycleScopeProvider could be moved to a separate (optional) artifact if we support default methods. It could just extend ScopeProvider and the default requestScope() method would do the swizzling with a default implementation.

This works fine for android gradle projects, but could incur a nontrivial cost for buck/bazel builds where incremental compilation can't work some magic.

@ZacSweers
Copy link
Collaborator Author

Opened an issue on bazel to better understand how they avoid clean build churn - bazelbuild/bazel#4186

@ZacSweers
Copy link
Collaborator Author

Closing in favor of #198

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

5 participants