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

Don't use default interface method bytecode optimization for target 1.8 #495

Merged
merged 2 commits into from
Mar 30, 2022

Conversation

oliviernotteghem
Copy link
Contributor

@oliviernotteghem oliviernotteghem commented Mar 25, 2022

Description:
Generating default methods in interfaces (with lambda expression in their body compiled as bridge methods) for the Java 8 target result in bytecode that fails to be de-sugared by Bazel. This will be presumably fixed in future version of bazel, for the time being, disable this optimization and rely on the bytecode generation used in older kotlin compiler, it relies on generating a separate static class and works around the issue with lambda/bridge.

More technical info :

@tyvsmith
Copy link
Member

For a java consumer, how will their usage change hitting this API?

@jbarr21 jbarr21 self-requested a review March 25, 2022 17:26
@jbarr21
Copy link
Contributor

jbarr21 commented Mar 25, 2022

this will not work for java consumers. in rib repo add:
private static final class FooActivityDelegate implements ActivityDelegate { }
to
RIBs/android/demos/memory-leaks/src/main/java/com/uber/rib/SampleApplication.java
and you’ll see a build failure for
Class 'FooActivityDelegate' must either be declared abstract or implement abstract method 'onCreate(Bundle)' in 'ActivityDelegate'

Copy link
Contributor

@jbarr21 jbarr21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this breaks java consumers

@oliviernotteghem
Copy link
Contributor Author

oliviernotteghem commented Mar 28, 2022

@jbarr21 @tyvsmith : Got it, we need to preserve Java consumers too, it indeed breaks. Didn't understand at first how java compiles against kotlin interface default methods (for the record, this is a very good resource : https://www.zacsweers.dev/jvmdefault-more-useful/ )

We have a few options, none of them are ideal given Bazel doesn't support yet bytecode output with jvm-default=**all** newer option (which is available only in Gradle BTW) when used by generics in interface/lamdbas (which, presumably, triggers type erasure and the generation of the problematic bridge method). Note these options are temporary, until this issue is fixed in Bazel Lambda desugaring code - I could generate a plain vanilla Bazel project, consuming a dummy library (built with Gradle, since, again the jvm-default all option is not supported in Bazel kotlin rules yet) with default interface using generics, and repro the problem, so I'll file a github issue on Bazel project and hopefully this will be looked at soon.

So our options are :

  1. nuke the 2 default interface that uses generics (and move them to helpers methods, or whatever that removes the lambdas out of the body of default method interface)
  2. revert to using jvm-default=enable and tag all default interface in our codebase (@JvmDefault). This is deprecated with newer compiler, but avoid the Bazel issue and preserve java consumers.
  3. Maybe a kotlin trick I am not aware of.

Again, no ideal solution I could find, but I think #2 is probably the lesser of the evils. Once I hear from you guys, I will update the PR with #2 unless you have a better solution. We should also add a test to cover the java consumer scenario.

EDIT : github issue filed at bazelbuild/bazel#15144

@oliviernotteghem
Copy link
Contributor Author

Discussed offline with @jbarr21 and will temporarily impl solution #2 until bazelbuild/bazel#15144 is solved.

@jbarr21 jbarr21 self-requested a review March 30, 2022 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants