SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.43k stars 193 forks source link

Kotlin Support #245

Open phase opened 6 years ago

phase commented 6 years ago

This isn't a high priority at all, but projects outside of Sponge that use Mixins may want to use Kotlin. The only issue I see is with static functions, and Kotlin's lack of them.

Let's say I want to overwrite Minecraft.main(). In Java, it's simple:

@Mixin(Minecraft.class)
public abstract class MixinMinecraft implements Runnable {

    @Overwrite
    public static void main(String[] args) {
        System.out.println("Minecraft.main() has been OVERWRITTEN");
    }

}

In Kotlin, there are no static functions. In place of them are Companion Objects.

@Mixin(Minecraft::class)
abstract class MixinMinecraft : Runnable {

    companion object {
        @Overwrite
        fun main(args: Array<String>) {
            println("Minecraft.main() has been overwritten by a Kotlin Mixin")
        }
    }

}

Instead of generating a static function, this creates a Companion field that can be used.

// Decompiled class from above

@Metadata(mv={1, 1, 9}, bv={1, 0, 2}, k=1, d1={"snipped"}, d2={"Lio/jadon/pigeon/mixin/MixinMinecraft;", "Ljava/lang/Runnable;", "()V", "Companion", "launcher"})
@Mixin({Minecraft.class})
public abstract class MixinMinecraft
  implements Runnable
{
  public static final Companion Companion = new Companion(null);

  @Metadata(mv={1, 1, 9}, bv={1, 0, 2}, k=1, d1={"snipped"}, d2={"Lio/jadon/pigeon/mixin/MixinMinecraft$Companion;", "", "()V", "main", "", "args", "", "", "([Ljava/lang/String;)V", "launcher"})
  public static final class Companion
  {
    @Overwrite
    public final void main(@NotNull String[] args)
    {
      Intrinsics.checkParameterIsNotNull(args, "args");
      String str = "Minecraft.main() has been overwritten by a Kotlin Mixin";
      System.out.println(str);
    }
  }
}

Kotlin does provide us with an @JvmStatic annotation to put on companion functions.

@Mixin(Minecraft::class)
abstract class MixinMinecraft : Runnable {

    companion object {
        @JvmStatic
        @Overwrite
        fun main(args: Array<String>) {
            println("Minecraft.main() has been overwritten by a Kotlin Mixin")
        }
    }

}

This generates slightly different bytecode, giving us the static function we want be leaving in the companion object.

// Decompiled class from above

@Metadata(mv={1, 1, 9}, bv={1, 0, 2}, k=1, d1={"snipped"}, d2={"Lio/jadon/pigeon/mixin/MixinMinecraft;", "Ljava/lang/Runnable;", "()V", "Companion", "launcher"})
@Mixin({Minecraft.class})
public abstract class MixinMinecraft
  implements Runnable
{
  public static final Companion Companion = new Companion(null);

  @JvmStatic
  @Overwrite
  public static final void main(@NotNull String[] args)
  {
    Intrinsics.checkParameterIsNotNull(args, "args");
    Companion.main(args);
  }

  @Metadata(mv={1, 1, 9}, bv={1, 0, 2}, k=1, d1={"snipped"}, d2={"Lio/jadon/pigeon/mixin/MixinMinecraft$Companion;", "", "()V", "main", "", "args", "", "", "([Ljava/lang/String;)V", "launcher"})
  public static final class Companion
  {
    @JvmStatic
    @Overwrite
    public final void main(@NotNull String[] args)
    {
      Intrinsics.checkParameterIsNotNull(args, "args");
      String str = "Minecraft.main() has been overwritten by a Kotlin Mixin";
      System.out.println(str);
    }
  }
}

An ideal solution would be to:

  1. Detect the kotlin.Metadata annotation on classes and recognize that we're dealing with a Kotlin class.
  2. Ignore any field named Companion that has the type MixinClass$Companion.
  3. Ignore any subclasses called Companion with the kotlin.Metadata annotation.
  4. Detect the kotlin.jvm.JvmStatic annotation on methods and replace its bytecode with the bytecode of the method it calls. (The original method should only null checks and a direct call to the function in the companion object, so copying the bytecode should be fine.)

I might take a crack at this. If this isn't wanted in the main repo then I'll just maintain a fork.


As a side note, making a private companion object will generate the same Companion field with an @Deprecated:

@Deprecated
public static final Companion Companion = new Companion(null);
DenWav commented 6 years ago

This should be fun.

Mumfrey commented 6 years ago

Whilst I don't have a problem with the functionality in principle, I know next to nothing about Kotlin besides what Kyle has mentioned to me over the last couple of years. The biggest issue is not how easy it is to work around minor issues but what this means for the longer term.

I'm not the kind of person to add something half-baked, so the choices from my point of view are either "Kotlin is supported" or "Kotlin is not supported, use it at your own risk". Adding concessions for individual cases tacitly implies the first so what you're actually asking for here is Kotlin support. That's not something I see as particularly off the wall but I absolutely wouldn't be prepared to introduce that support until I know what the full implications are and what else would need to be supported.

I definitely don't have time to look into this now because it requires learning a new language, but "other language support" would definitely be a nice long-term goal. Obviously Kotlin is a little niche at the moment but it does seem to be growing, though obviously more established JVM languages like Groovy and Scala might be more worthwhile pursuing since they are far more mature, though from my understanding people are already using those languages to write mixins without problems, so potentially we can chalk that up as done already.

The rebuttal to this statement, notwithstanding everything else:

This isn't a high priority at all, but projects outside of Sponge that use Mixins may want to use Kotlin

is of course "why?". Mixins aren't Java classes as much as they are expressions of a transformation to a target class which happens to use Java as a domain-specific language. Why not just write the mixins in Java and have them call your Kotlin code?

The tl;dr of the above is basically: "I'm happy to add Kotlin support as a whole, but not prepared to add any 'band-aid' solutions which imply full support for Kotlin because that's not how I roll".

I will try to find some time to s/learn/get Kyle to teach me/ Kotlin so I can investigate the bytecode generated. Mixin is of course tightly-coupled to the bytecode it ingests so before I'm comfortable declaring that Kotlin is supported I need to be able to understand how it works. Some considerations that are already fully-covered in the Java case but would need to be investigated in the Kotlin case are things like

I will look into the issue and see how simple/hard it is to accomodate.

DenWav commented 6 years ago

Why not just write the mixins in Java and have them call your Kotlin code?

This is what I would recommend in general with using Mixins with Kotlin for the time being, until it's "officially supported". Simply treat the Mixin file as a header file, and the Kotlin file as the implementation. Unfortunately you'll need to add some "Java glue" in there to really make that work, but it's certainly possible.

As far as supporting Kotlin officially, Kotlin has kapt, which I know almost nothing about, but I believe it's an annotation processor which allows more flexibility, such as allowing code generation using proper APIs rather than a loop-hole in the compiler. Or it could just be a clone of the Java annotation processor, I'm not sure.

Anyways, it may be possible to use kapt to transform Kotlin Mixins into Java Mixins, but I'm really spit balling hard here, so take it with a grain of salt. If that is actually feasible, then you could theoretically have the kapt project completely separate as a third party thing (which may or may not be officially supported) without Mixins having to change at all.

Mumfrey commented 6 years ago

This is what I would recommend in general with using Mixins with Kotlin for the time being, until it's "officially supported". Simply treat the Mixin file as a header file, and the Kotlin file as the implementation. Unfortunately you'll need to add some "Java glue" in there to really make that work, but it's certainly possible.

I'd agree with that, as I mentioned above. It doesn't seem overly taxing to write mixins in Java for the moment, and leverage other languages for business logic. As developers we already write shell scripts for automation, groovy or xml for build scripts, etc. etc. I don't see Java as a DSL for mixins being that outrageous, and if I can support other languages for that down the line then it makes sense to employ them once they're fully supported rather than trying to shoe-horn them to fit when Mixin can't fully cope with that.

As far as supporting Kotlin officially, Kotlin has kapt, which I know almost nothing about, but I believe it's an annotation processor which allows more flexibility, such as allowing code generation using proper APIs rather than a loop-hole in the compiler. Or it could just be a clone of the Java annotation processor, I'm not sure.

Anyways, it may be possible to use kapt to transform Kotlin Mixins into Java Mixins, but I'm really spit balling hard here, so take it with a grain of salt. If that is actually feasible, then you could theoretically have the kapt project completely separate as a third party thing (which may or may not be officially supported) without Mixins having to change at all.

All of which just seems to be trying to bash a round yellow peg into a square blue hole for the sake of it. Sure, if the peg drops in neatly then there's no detriment to using it. But if you have to complicate your toolchain so much just for the arguable benefit of using a nascent language it seems counter-productive. After all surely the use-case of kotlin is writing code which is more concise and expressive. If you have to make everything twice as verbose as the Java equivalent then the approach has backfired. If the complexity ends up in the toolchain you've only shifted the issue not really addressed it.

It's why I think the correct, sensible way to go is to investigate first-class suppport and if that turns out to be unachievable then settle on no support. Some half-baked solution for shoe-horning one thing into another doesn't really help anyone in the long run.

DenWav commented 6 years ago

I agree, I'm mostly talking to @phase with that comment, saying he might be able to build a tool like that himself if proper Mixin support falls through.

DenWav commented 6 years ago

I took a stroll through the issues again and came across this one and realized this issue might be slightly flawed.

It's a common misconception that Kotlin doesn't have static functions because companion objects take their place. companion objects take the place for class level state things, and obviously your example shows that @JvmStatic allows generating bridge methods to the generated singleton, which is what objects (companion or not) actually are in Kotlin.

But the real reason Kotlin doesn't have static functions is because it doesn't need them, it has top level members to begin with. But, of course, These are all language level constructs, and the only thing Mixin actually cares about is the bytecode that it is told it needs to process. So it should come as no surprise how top level functions are actually implemented in terms of the JVM compilation target for Kotlin: static methods!

So let's take a look and see what that would actually look like in Kotlin:

@file:JvmName("MixinMinecraft")
@file:Mixin(MinecraftServer::class)

package org.spongepowered.server.mixin.core

import net.minecraft.server.MinecraftServer
import org.spongepowered.asm.mixin.Mixin
import org.spongepowered.asm.mixin.Overwrite

@Overwrite
fun main(args: Array<String>) {
    println("Minecraft.main() has been overwritten by a Kotlin Mixin")
}

Few notes:

Now here's what that code generates, decompiled back to Java:

package org.spongepowered.server.mixin.core;

import kotlin.Metadata;
import kotlin.jvm.JvmName;
import kotlin.jvm.internal.Intrinsics;
import net.minecraft.server.MinecraftServer;
import org.jetbrains.annotations.NotNull;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Overwrite;

@Metadata(
   mv = {1, 1, 9},
   bv = {1, 0, 2},
   k = 2,
   d1 = {"\u0000\u0014\n\u0000\n\u0002\u0010\u0002\n\u0000\n\u0002\u0010\u0011\n\u0002\u0010\u000e\n\u0002\b\u0002\u001a\u001b\u0010\u0000\u001a\u00020\u00012\f\u0010\u0002\u001a\b\u0012\u0004\u0012\u00020\u00040\u0003H\u0007¢\u0006\u0002\u0010\u0005¨\u0006\u0006"},
   d2 = {"main", "", "args", "", "", "([Ljava/lang/String;)V", "production sources for module SpongeVanilla_main"}
)
@JvmName(
   name = "MixinMinecraft"
)
@Mixin({MinecraftServer.class})
public final class MixinMinecraft {
   @Overwrite
   public static final void main(@NotNull String[] args) {
      Intrinsics.checkParameterIsNotNull(args, "args");
      String var1 = "Minecraft.main() has been overwritten by a Kotlin Mixin";
      System.out.println(var1);
   }
}

Certainly not as pretty, but we are talking about compilation output here. I don't know if the extra @Metadata or @JvmName annotations on the class would cause an issue for Mixin or not, but I assume it would make no difference. It seems to me like this would actually load into Mixin properly and would accomplish what you want.


The downside for this is you'd need to declare two separate Mixins anytime you want to modify both static and non-static members of a target class. You can do this in the same file, but two Java classes would be generated. It would look like this (sorry this is getting big):

@file:JvmName("MixinStaticMinecraft")
@file:Mixin(MinecraftServer::class)

package org.spongepowered.server.mixin.core

import net.minecraft.server.MinecraftServer
import org.spongepowered.asm.mixin.Mixin
import org.spongepowered.asm.mixin.Overwrite
import org.spongepowered.asm.mixin.Shadow
import java.io.IOException

@Overwrite
fun main(args: Array<String>) {
    println("Minecraft.main() has been overwritten by a Kotlin Mixin")
}

@Mixin(MinecraftServer::class)
class MixinMinecraft {
    @field:Shadow
    private var serverPort: Int = 0

    @Overwrite
    @Throws(IOException::class)
    fun init() = serverPort % 2 == 0
}

Notice there are two @Mixin declarations. The @file:Mixin applies to the class Kotlin generates to hold all of the top-level members of this file, and the @Mixin on the actual MixinMinecraft class applies to that class. Notice I had to change the name in @file:JvmName as they would clash otherwise. But, if we look at the output code decompiled back to Java:

// MixinStaticMinecraft.java
package org.spongepowered.server.mixin.core;

import kotlin.Metadata;
import kotlin.jvm.JvmName;
import kotlin.jvm.internal.Intrinsics;
import net.minecraft.server.MinecraftServer;
import org.jetbrains.annotations.NotNull;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Overwrite;

@Metadata(
   mv = {1, 1, 9},
   bv = {1, 0, 2},
   k = 2,
   d1 = {"\u0000\u0014\n\u0000\n\u0002\u0010\u0002\n\u0000\n\u0002\u0010\u0011\n\u0002\u0010\u000e\n\u0002\b\u0002\u001a\u001b\u0010\u0000\u001a\u00020\u00012\f\u0010\u0002\u001a\b\u0012\u0004\u0012\u00020\u00040\u0003H\u0007¢\u0006\u0002\u0010\u0005¨\u0006\u0006"},
   d2 = {"main", "", "args", "", "", "([Ljava/lang/String;)V", "production sources for module SpongeVanilla_main"}
)
@JvmName(
   name = "MixinStaticMinecraft"
)
@Mixin({MinecraftServer.class})
public final class MixinStaticMinecraft {
   @Overwrite
   public static final void main(@NotNull String[] args) {
      Intrinsics.checkParameterIsNotNull(args, "args");
      String var1 = "Minecraft.main() has been overwritten by a Kotlin Mixin";
      System.out.println(var1);
   }
}
// MixinMinecraft.java
package org.spongepowered.server.mixin.core;

import java.io.IOException;
import kotlin.Metadata;
import net.minecraft.server.MinecraftServer;
import org.spongepowered.asm.mixin.Mixin;
import org.spongepowered.asm.mixin.Overwrite;
import org.spongepowered.asm.mixin.Shadow;

@Metadata(
   mv = {1, 1, 9},
   bv = {1, 0, 2},
   k = 1,
   d1 = {"\u0000\u0018\n\u0002\u0018\u0002\n\u0002\u0010\u0000\n\u0002\b\u0002\n\u0002\u0010\b\n\u0000\n\u0002\u0010\u000b\n\u0000\b\u0007\u0018\u00002\u00020\u0001B\u0005¢\u0006\u0002\u0010\u0002J\b\u0010\u0005\u001a\u00020\u0006H\u0007R\u0012\u0010\u0003\u001a\u00020\u00048\u0002@\u0002X\u0083\u000e¢\u0006\u0002\n\u0000¨\u0006\u0007"},
   d2 = {"Lorg/spongepowered/server/mixin/core/MixinMinecraft;", "", "()V", "serverPort", "", "init", "", "production sources for module SpongeVanilla_main"}
)
@Mixin({MinecraftServer.class})
public final class MixinMinecraft {
   @Shadow
   private int serverPort;

   @Overwrite
   public final boolean init() throws IOException {
      return this.serverPort % 2 == 0;
   }
}

That looks pretty good to me. The biggest issue I see that I don't know the solution to is accessing static members (either added by the MixinStaticMinecraft Mixin, or already present in MinecraftServer) since that would run into precisely the same problem this issue was originally hoping to address.

An issue in general that I see is declaring properties (for @Shadows, I mean) with any visibility other than private. private properties in Kotlin don't generate getter and setter methods for obvious reasons, but any other visibility does. @field:Shadow would still place the annotation on the field, but now you've got getters and setters you never wanted in your Mixin class.


At the end of the day, I can't and won't recommend writing Mixins in Kotlin for a couple reasons.

  1. Due to the nature of what Kotlin is trying to accomplish, it frequently generates things behind the scenes which are normally really quite nice, but in this situation with Mixins, the output bytecode really is important, and Java being a (mostly) 1-to-1 representation of what you're expecting will be in the class is just easier to work with. If Mixin had first class support this would be less of an issue, but prepare for a biased second reason.
  2. If you're using IntelliJ, and let's be honest, if you're using Kotlin you probably are, then you are hopefully using my plugin which provides a good set of features which makes writing Mixins a hell of a lot easier. And just writing these stupid small examples reminded me how painful it is to write Mixins without any of these tools available. It just seems like a better idea in principle to write Mixins in Java, with the help of the extra tooling that allows, and the logic in Kotlin.
clarfonthey commented 2 years ago

Note that there is an open discussion on the Kotlin issue tracker about optimising out companion objects, so, that might be relevant too: https://youtrack.jetbrains.com/issue/KT-15595

Eveeifyeve commented 8 months ago

I feel like this should be mentioned again cause people are building Minecraft clients in Kotlin and there should be full support for Kotlin to have mixin support I just ran into this as I was compiling and I am so pissed I can't fully have a client in KOTLIN!!!!

AlgorithmLX commented 5 months ago

In my opinion, Mixins for Kotlin has only one problem - and that is the lack of generating a refmap file. In the sense that it is not generated at all. For me, there are no other limitations. Yes, and to be honest, in Java it is not particularly eagerly generated either...

LlamaLad7 commented 5 months ago

No, that is not correct. The Kotlin compiler generates a bunch of synthetic members which will cause issues when merged into the target class. Notably using a Kotlin mixin on a class at all immediately breaks KReflect on that class due to the @Metadata being merged. There is no reason to write mixins in Kotlin and no one should do so.