kotlin_rules and Ijars: Update the Ijar tooling to do the right thing.

[hsyed]

I have been working on a new set of kotlin rules and they are currently being migrated to bazelbuild/rules_kotlin.

One goal with the design of the Kotlin JVM ruleset is to stay as close to the native java rules and infrastructure as possible. The automatic Ijars generation is problematic as the java rules auto create ijars. Ijars generated for Kotlin work in a lot of cases, but inlined methods break (might be other cases).

Proposal: Adapt Ijar to do the right thing for Kotlin Jars.

I propose making a change to the behaviour of the ijar tool so that it does the right thing when it comes to Kotlin metadata. Adding this change to Bazel core simplifies the Kotlin rules and allows more reuse out of java common infrastructure. I propose the ijar tool be updated to:

1. Initially to detect a Kotlin jar and ignore it for processing -- perhaps just symlink the ijar to the full jar ? This should be straight forward a Kotlin jar has some markers that can be identified without processing bytecode.
2. Adapt the tool to generate valid Kotlin Ijars. Afaik this means retaining certain bytecode elements and marker files.

Motivating example 1: dep management tooling / java_import.

I have just added a kotlin_import rule to:

1. Get around the automatic ijar generation by java_import, and
2. Ensure the Jar has a KotlinInfo provider so that full_compile_jars are always selected when compiling Kotlin.

Any fancy java dependency management tooling that we develop will likely work via orchestrations of java_import, java_library and java_import_external it's a shame to have to add a layer for Kotlin when it's barely needed.

[meteorcloudy]

@iirina Can you take a look at this?

[iirina]

Can you explain why the automatic ijar generation in the java rules is problematic? Is it because you are using java_import to import kotlin jars, in which case ijar will fail (sometimes)? How does it impact java_library/java_import_external?

[hsyed]

I misrepresented the sometimes part. Most Kotlin libraries that a JVM project would use as an external dependency will have DSLs that make heavy use of inline. So some intermediate kotlin_ rule has to be used to:

• Add a KotlinInfo -- to enable rules in the workspace to ensure full_compile_jars are used when using the target as a dependency.
• Force selection of full_compile_jars by re-interpreting the JavaInfo from deps imported as plain Java Jars --e.g., using the //jar component of maven_jar.

If the Kotlin ABI elements were retained by the Ijar tool transparently then special handling would not be needed so Kotlin libraries could be imported by java_import, maven_jar, java_library.

Current dependency management strategies (pubref/rules_maven, bazel-dep, migration-tooling, this one) go beyond this. They generate BUILD file entries which range from from simply listing runtime_deps to modelling the transitive deps in fine grained detail (exports etc).

Current and future versions of these tools could work for Kotlin transparently as long as the ijar tool doesn't strip off the Kotlin ABI elements.

This benefits of this feature go beyond just the motivational example I gave above. Some other reasons for why updating the ijar tool:

• The intellij plugin model has to have conditional checks for the intermediate rules --e.g., kotlin_import and kotlin_stdlib.

[cushon]

Adapt the tool to generate valid Kotlin Ijars. Afaik this means retaining certain bytecode elements and marker files.

Is there a reference for specifically how these ABI elements are represented in bytecode?

[hsyed]

Just had a closer look.

For marker files there is a META-INF/<module name>.kotlin_module. Might be more depending on the compilation modes / module types.

After some experimentation inlining metadata seems to be embedded in the Metadata annotation. Every class has a Metadata annotation.

Using the bytecode tool included in the Intellij Kotlin plugin to inspect the following test file:

package test

fun monkey(x: () -> Unit) {
    x()
}

inline fun donkey(y: () -> Unit): Int {
    y()
    return 1
}

fun lonkey(z: () -> Unit): Int {
    z()
}

d2 entry in metadata -- notice the inlined function includes the param type.

d2={"donkey", "", "y", "Lkotlin/Function0;", "", "lonkey", "z", "monkey", "x", "production sources for module .workspace"})

full output

// ================test/TestKt.class =================
// class version 50.0 (50)
// access flags 0x31
public final class test/TestKt {


  // access flags 0x19
  // signature (Lkotlin/jvm/functions/Function0<Lkotlin/Unit;>;)V
  // declaration: void monkey(kotlin.jvm.functions.Function0<kotlin.Unit>)
  public final static monkey(Lkotlin/jvm/functions/Function0;)V
    @Lorg/jetbrains/annotations/NotNull;() // invisible, parameter 0
   L0
    ALOAD 0
    LDC "x"
    INVOKESTATIC kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull (Ljava/lang/Object;Ljava/lang/String;)V
   L1
    LINENUMBER 4 L1
    ALOAD 0
    INVOKEINTERFACE kotlin/jvm/functions/Function0.invoke ()Ljava/lang/Object;
    POP
   L2
    LINENUMBER 5 L2
    RETURN
   L3
    LOCALVARIABLE x Lkotlin/jvm/functions/Function0; L0 L3 0
    MAXSTACK = 2
    MAXLOCALS = 1

  // access flags 0x19
  // signature (Lkotlin/jvm/functions/Function0<Lkotlin/Unit;>;)I
  // declaration: int donkey(kotlin.jvm.functions.Function0<kotlin.Unit>)
  public final static donkey(Lkotlin/jvm/functions/Function0;)I
    @Lorg/jetbrains/annotations/NotNull;() // invisible, parameter 0
   L0
    ALOAD 0
    LDC "y"
    INVOKESTATIC kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull (Ljava/lang/Object;Ljava/lang/String;)V
   L1
    LINENUMBER 8 L1
    ALOAD 0
    INVOKEINTERFACE kotlin/jvm/functions/Function0.invoke ()Ljava/lang/Object;
    POP
   L2
    LINENUMBER 9 L2
    ICONST_1
    IRETURN
   L3
    LOCALVARIABLE y Lkotlin/jvm/functions/Function0; L0 L3 0
    LOCALVARIABLE $i$f$donkey I L0 L3 1
    MAXSTACK = 2
    MAXLOCALS = 2

  // access flags 0x19
  // signature (Lkotlin/jvm/functions/Function0<Lkotlin/Unit;>;)I
  // declaration: int lonkey(kotlin.jvm.functions.Function0<kotlin.Unit>)
  public final static lonkey(Lkotlin/jvm/functions/Function0;)I
    @Lorg/jetbrains/annotations/NotNull;() // invisible, parameter 0
   L0
    ALOAD 0
    LDC "z"
    INVOKESTATIC kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull (Ljava/lang/Object;Ljava/lang/String;)V
   L1
    LINENUMBER 13 L1
    ALOAD 0
    INVOKEINTERFACE kotlin/jvm/functions/Function0.invoke ()Ljava/lang/Object;
    POP
   L2
    LINENUMBER 14 L2
    ICONST_0
    IRETURN
   L3
    LOCALVARIABLE z Lkotlin/jvm/functions/Function0; L0 L3 0
    MAXSTACK = 2
    MAXLOCALS = 1

  @Lkotlin/Metadata;(mv={1, 1, 9}, bv={1, 0, 2}, k=2, d1={"\u0000\u0014\n\u0000\n\u0002\u0010\u0008\n\u0000\n\u0002\u0018\u0002\n\u0002\u0010\u0002\n\u0002\u0008\u0005\u001a\u0017\u0010\u0000\u001a\u00020\u00012\u000c\u0010\u0002\u001a\u0008\u0012\u0004\u0012\u00020\u00040\u0003H\u0086\u0008\u001a\u0014\u0010\u0005\u001a\u00020\u00012\u000c\u0010\u0006\u001a\u0008\u0012\u0004\u0012\u00020\u00040\u0003\u001a\u0014\u0010\u0007\u001a\u00020\u00042\u000c\u0010\u0008\u001a\u0008\u0012\u0004\u0012\u00020\u00040\u0003\u00a8\u0006\u0009"}, d2={"donkey", "", "y", "Lkotlin/Function0;", "", "lonkey", "z", "monkey", "x", "production sources for module .workspace"})
  // compiled from: test.kt
}


// ================META-INF/production sources for module .workspace.kotlin_module =================
���������������	
�
�test��TestKt

I think I read somewhere that the bytecode contained within the stub of an inlined function is used as well.

references:

understand every line of your codebase

kotlin-bytecode-generation-and-runtime-performance

[cushon]

ijar will remove non-class files including META-INF/<module name>.kotlin_module, but that would be easy to change.

It does not remove annotations, so @Metadata should already be preserved.

For inline methods, I assume the implementation needs to be visible across compilation boundaries? It might need a way to identify inline methods and to include their implementations in the interface jar.

[hsyed]

Yes I think thats right, the compiler seems to generate bytecode mimicking a regular JDK8 higher order function, the lambda invocation instructions must be used as the placeholders, nice design.

It seems like a simple enough change, visit classes with Metadata annotations, discover public and protected inline methods and preclude them from bytecode dropping.

I could gather a list of the file patterns relevant to the various module types -- as globs ?.

---

Since we are visiting the jars and the metadata I wonder if it would be possible to either:

1. Attach providers marking the Kotlin Jar type, along with some basic fields such as module name.
2. Or just attach a basic provider marking the Kotlin Jar type and write out a bazel proto file containing relevant metadata.

There are going to be quite a few rule and rule type variants. If we had option 1 it would really simplify the design as we can constraint the attributes.

[lberki]

In my uninformed opinion, I think it's fine to special-case Kotlin stuff in ijar a little bit; the above "don't drop bytecode from methods that are marked as inline" counts as such in my book.

I don't understand the latter half of @hsyed 's comment, though -- how is this new proposed provider relevant to changes to ijar?

[hsyed]

@iberki I've removed the second part from my previous comment. It's not relevant to this issue at all.

[davidstanke]

Great -- sounds like we're well aligned on approach. @hsyed do you have the bandwidth to implement this? @cushon any concerns about the approach described?

[lberki]

@hsyed -- excellent, I was afraid for a second that my understanding is that hazy :)

[hsyed]

@davidstanke I haven't looked at C++ in a long time and I just opened the JVM bytecode spec last week. I think it would be best implemented by someone who has already worked on the tool.

There are two parts to this feature:

1. Retaining metadata files.
2. Discovery and retention of method bodies of inline functions.

1 is quite straight forward and I could implement this, but if someone else does 2 might as well get 1 one done in that PR.

[hsyed]

How would we test the changes ? I can add a test case to rules_kotlin that tests if the inlined methods are retained via a java_import.

[lberki]

That's one option. You could also add a pre-compiled Kotlin .jar somewhere under

https://github.com/bazelbuild/bazel/tree/master/third_party/ijar/test

and add a test that checks what happens to the class files. Or both. Both is probably the best.

[hsyed]

I've added some test cases, these cover the inlining change and retention of the metadata.

Now onto the test. I can add a function to ijar_test.sh this could:

1. Run the ijar tool on the jar.
2. Use the jar tool to verify the metadata file has been retained.
3. Try to compile a kotlin client file that references the inline functions.

@lberki why do you think ?

[lberki]

I'd rather live without (3) because that would make the tests of ijar dependent on Kotlin. Other than that, awesome.

[hsyed]

rules_kotlin will be a part of the CI runs soon. It should catch kotlinc's opinion on the processed ijar.

For 3 we could assert function bodies still exist using javap -c ? --e.g.,

Compiled from "InlineCases.kt"
public final class ClassWithInline {
  public final void classInlineFun(kotlin.jvm.functions.Function0<kotlin.Unit>);
    Code:
       0: aload_1
       1: ldc           #9                  // String op
       3: invokestatic  #15                 // Method kotlin/jvm/internal/Intrinsics.checkParameterIsNotNull:(Ljava/lang/Object;Ljava/lang/String;)V
       6: aload_1
       7: invokeinterface #21,  1           // InterfaceMethod kotlin/jvm/functions/Function0.invoke:()Ljava/lang/Object;
      12: pop
      13: return

  public ClassWithInline();
    Code:
       0: aload_0
       1: invokespecial #30                 // Method java/lang/Object."<init>":()V
       4: return
}

asserting this fragment should be enough of a test:

  public final void classInlineFun(kotlin.jvm.functions.Function0<kotlin.Unit>);
    Code:

[lberki]

Yep, checking the output of javap -c works; what I'd like to avoid is that the tests of ijar have a dependency on Kotlin. If you think that test is necessary, you can put it in rules_kotlin.

[cushon]

The information in metadata about which methods are inlined appears to be a serialized proto: https://github.com/JetBrains/kotlin/blob/ee50aec7342ab375a105bb318e6501135e859028/core/deserialization/src/descriptors.proto. @lberki are we OK with added a dependency on proto to ijar? I think there were some concerns about making the c++ singlejar depend on proto, but I'm not sure where that landed.

[hsyed]

I've wired up the tests in the pr (#4632), the ijar step is currently just cp'ing so it can be merged in.

[lberki]

@hsyed : I assume you mean "the ijar step is currently just cp'ing so it cannot be merged in", right?

@cushon : unfortunately, I can't recall or find that discussion anywhere :( I think in Bazel, that is okay because proto_library doesn't have an implicit dependency on anything Java, so there won't be a dependency cycle, but the internal version does, so we need to do some tricks to make that work and the collateral damage would be that the internal version wouldn't support Kotlin. I think we could live with that.

[lberki]

Ugh, I was just made aware that the extra dependencies required by protocol buffers make the above solution impractical :( I can think of a few options:

• Write an "ijar-for-Kotlin" binary that strips out the Kotlin-specific parts from jars as a step to be run before or after ijar. We'd need to tweak the Skylark API for Java a bit, but I think that's doable? (paging @iirina for confirmation)
• Check in the source files generated by the proto compiler, thus cutting the dependency that way.

I'd prefer the former one, because then people who don't use Kotlin don't pay the price to support it.

Any more clever ideas?

[cpsauer]

Hey, all! Figured I'd toss up a quick PR that disables ijar in java_import by default. The goal is to unblock its usage for Kotlin JARs enable reunification around java_import if you folks wanted it.

So please see #14967. Would love it if you'd weigh in, lmk what you think, or add to it!

Backstory:
I'd been fixing some other things in java_import around Android/ProGuard and ran across this issue when I noticed that my fix wasn't automatically applying to rules_jvm_external. (See #14966 and bazel-contrib/rules_jvm_external#672.) From the volume of discussion and all the java_import re-implementations, it seems like there's a strong need here, so, like you, I'm hoping Bazel folks will up-priorize a real fix to ijar! But in the meantime, it seemed like disabling ijar with a togglable attribute was the right stopgap measure, as you all had proposed above. I'd just added a (private) attribute to java_import for my other PR, so I figured I might as well take a quick crack at this one. I'm not (currently) a Kotlin user or anything, but if felt like the right thing to do. So I could use your help making sure it works for you and integrating when the time comes!

[cpsauer]

For (future) readers of this issue: Plenty of additional discussion over in #14967.

To answer a dangling question above: It doesn't look like they bothered with ijar in aar_import.
Presumably that's for the same reasons as above, and that you all didn't, namely the build-caching optimization it conveys isn't that relevant to prebuilt code. (But edit: See @cushon's note below, describing a newly identified use case.)

[cushon]

To recap some of the reason ijar exists for java_import from (#14967 (comment), #14967 (comment)), it makes incremental builds faster when the jar changes but the ABI doesn't, and it makes the jars on the compile-time classpath much smaller. Prebuilt jars may not change very often, but java_import is also used with generated jars (e.g. together with genrules). Those jars can very large and change often. Disabling ijar by default for java_import doesn't look like a viable approach to me.

[cushon]

To expand on the previous comment, the option that @kevin1e100 brought up in #14967 (comment) of having ijar not strip classes annotated with @kotlin.Metadata seems like the most promising option that's been discussed to me.

We might also consider adding more validation to java_import, or example to detect mistakes like forgetting to add the kotlin standard library dep when importing kotlin jars, or detecting attempts to use java_import on jars that contain kotlin inline functions (until this bug is fixed).

[cpsauer]

@cushon and @kevin1e100, a question coming from a place of true ignorance about the state of a different alternative:

I'd read @kevin1e100's comment above (#4549 (comment)) about generating ABI jars (equivalent to ijars?) via an official kotlinc plugin. I'd also seen that Kevin had contributed a fix to the other blocking issue he'd linked (awesome). Do we know the status of that, or if it is a feasible alternative? That is, would it also work for Java and therefore be an ijar alternative that would work with Kotlin? (I got as far as seeing that there was maybe some experimental ABI jar support in rules_kotlin, maybe based on it.)

[kevin1e100]

@cpsauer the jvm-abi-gen plugin I'm guessing you're asking about produces ijar-like artifacts when compiling Kotlin sources. I don't believe isn't usable with Jar files the Kotlin compiler previously produced, so it's not helpful in the context of java_import afaik.

[cpsauer]

Oh! Got it. Bummer Thanks, @kevin1e100, for taking the time to reply.

[cushon]

I had a quick look at kevin1e100's @kotlin.Metadata idea (#4549 (comment)), and it seems feasible: https://bazel-review.googlesource.com/c/bazel/+/195202/1/third_party/ijar/classfile.cc

That will meant that ijar / java_import automatically don't strip Kotlin classes going forward, without needing any new attributes/configuration.

There's some possible future work around around more precise interface jars for Kotlin. To try to summarize, theoretically ijar could decode the proto in the @Metadata annotation and check for 'inline' modifiers:

• adding a protobuf dep to ijar might require some wrangling, I think we tried to add a protobuf dep to singlejar at one point and it caused problems, but I don't remember the details and it was a while ago
• ijar would need to preserve anonymous classes used in inline methods, I'm not sure if checking the EnclosingMethod attribute would be sufficient
• there was some speculation we might need internal heuristics

If someone wants to take a swing at that I think we'd want to try to accept that contribution, but it could be a bunch of work.

[cushon]

There's more that could be done here (see the note in #4549 (comment) about future work), but ijar / java_import now do something reasonable for Kotlin by default, by not applying stripping to classes annotated with @kotlin.Metadata.

[kevin1e100]

That's amazing, thank you so much!

[cpsauer]

Wow! That's awesome. Thanks @cushon for getting it done.
(I'll update some of the other folks I'd seen blocked on it.)

[Bencodes]

Thanks for making that happen @cushon!