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

[jvm] Field annotations not exported correctly with RententionPolicy.RUNTIME #11370

Closed
EliteMasterEric opened this issue Nov 11, 2023 · 14 comments · Fixed by #11398
Closed

[jvm] Field annotations not exported correctly with RententionPolicy.RUNTIME #11370

EliteMasterEric opened this issue Nov 11, 2023 · 14 comments · Fixed by #11398
Assignees

Comments

@EliteMasterEric
Copy link
Contributor

EliteMasterEric commented Nov 11, 2023

Use Case

I am working with a library which utilizes annotations on a class's static fields to manage a configuration, with the benefit being that you can access the config value by querying the static field.

Example

// Haxe
import test.CoolClass.CoolClass_Comment;
import test.CoolClass.CoolClass_Entry;

class Main {
    public static function main():Void {
        var clazz:java.lang.Class = Lib.toNativeType(MyClass);
        TestClass.processClass(clazz);
    }
}

class MyClass {
	@:meta(CoolClass_Entry(name = "Outer"))
	public var outer:String;

	@:meta(CoolClass_Entry(name = "Inter"))
	public var inner:String;

	@:meta(CoolClass_Comment())
	public var comment:CoolClass_Comment;
}
// Java
public class CoolClass {
    @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) public @interface Entry {
        String category() default "default";
        String name() default "Entry";
    }
    @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.FIELD) public @interface Comment {
        String category() default "default";
    }
}

Using field.isAnnotationPresent() on the fields of MyClass returns false, when it returns true on a pure Java implementation.

Minimal Reproduction

https://github.com/EliteMasterEric/Issue-11370

@EliteMasterEric EliteMasterEric changed the title [jvm] Inner class annotations not exported correctly [jvm] Field annotations not exported correctly Nov 11, 2023
@EliteMasterEric
Copy link
Contributor Author

This could be related to any number of things; the fact the annotation is an inner class, the fact the annotation is on a field, the fact that the annotation is marked as RententionPolicy.RUNTIME...

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Nov 13, 2023

Pretty sure it's just the RetentionPolicy.RUNTIME because I just encountered the error again with a class annotation that was not an inner class.

I did notice while messing around with it that the metadatas were in the constant pool on the correct version and not on the Haxe exported version so maybe that's related.

@EliteMasterEric EliteMasterEric changed the title [jvm] Field annotations not exported correctly [jvm] Field annotations not exported correctly with RententionPolicy.RUNTIME Nov 13, 2023
@Simn
Copy link
Member

Simn commented Nov 13, 2023

The output for the Java and Haxe versions use the same annotations:

jclasslib_wb3HoSGmF0

However, the path of the annotation differs:

jclasslib_ASImC0MXgG

I'm pretty sure that that's the actual issue here. I don't know why this loses the test package and ends up in haxe.root, will have to see how path management is supposed to work here.

Could you transform the test case into something that avoids inner classes? That way we can avoid chasing some red herrings here.

@Simn
Copy link
Member

Simn commented Nov 13, 2023

The problem here is that @:meta doesn't respect imports or anything else, because our metadata is untyped. The JVM generator tries to be helpful by turning @:meta(CoolClass_Entry(name = "Outer")) into @:meta(haxe.root.CoolClass_Entry(name = "Outer")), which is obviously not what we want. If we instead use @:meta(test.CoolClass_Entry(name = "Outer")) then no rewriting takes place, but the path is still incorrect because the _ doesn't become $ in the output, and we can't use the latter in the path.

I don't have a good solution (other than typed metadata) off the top of my head, other than allowing something like @:meta("test.CoolClass$Entry"(name = "Outer")). That would feel pretty hacky though, and still require users to be aware of the problem. Interested in opinions!

@EliteMasterEric
Copy link
Contributor Author

Interesting, I will look into modifying the test case to see if non-inner annotations work properly, and whether they work if fully qualified. If they do, that fixes some (but not all) of my use cases.

@Simn
Copy link
Member

Simn commented Nov 13, 2023

I just remembered that @:strict is a thing. I always forget how it works, but using it in your example makes a difference:

	@:strict(CoolClass_Entry({name: "Outer"}))
	public var outer:String;

	@:strict(CoolClass_Entry({name: "Inter"}))
	public var inner:String;

	@:strict(CoolClass_Comment())
	public var comment:CoolClass_Comment;
Field: outer
  Comment: false
  Entry: true (Outer), (default)
Field: inner
  Comment: false
  Entry: true (Inter), (default)
Field: comment
  Comment: true (default)
  Entry: false

@EliteMasterEric
Copy link
Contributor Author

@:strict seems to work in one of the simple cases I tested (this was the one that uses inner annotations).

However, with the other annotation I tested with, I'm having issues getting it to compile.

In this example case:

@:strict(org.spongepowered.asm.mixin.Mixin({
    value: net.minecraft.client.renderer.entity.player.PlayerRenderer,
    priority: 1000,
}))

I receive these messages:

characters 67-72: org.spongepowered.asm.mixin.Mixin has no field value
characters 11-19 : org.spongepowered.asm.mixin.Mixin has no field priority

Mixin definitely has those fields however.

Also, when I tried to make value an array, I received this error:

characters 12-72 : This expression is too complex to be a strict metadata argument

@EliteMasterEric
Copy link
Contributor Author

Also what tool is that you're using?

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Nov 14, 2023

Of note, @:strict(org.spongepowered.asm.mixin.Mixin()) does not get recognized at all.

The only difference that could be found in the compiled JAR is that in the correct JAR, the annotation is in RuntimeInvisibleAnnotations, and in the JAR generated by Haxe, the annotation is in RuntimeVisibleAnnotations.

Mixin has a retention policy of @Retention(RetentionPolicy.CLASS) but the other annotation I tested had a retention policy of RetentionPolicy.RUNTIME.

@Simn
Copy link
Member

Simn commented Nov 14, 2023

  • I'm using https://github.com/ingokegel/jclasslib
  • The array case is just not implemented. Should be easy to fix, but a test case would be good.
  • The "has no field" errors I'm not sure, could be some typing pass problem. I'll need a test case to investigate that.
  • The other Mixin problem confuses me. Are you saying that the one with the invisible annotation is seen but the one with the visible one isn't? That sounds wild... also I still don't know what this RetentionPolicy business is about and what should be used here.

@EliteMasterEric
Copy link
Contributor Author

RetentionPolicy is an annotation which dictates whether and how annotations are visible to the application via reflection.

https://stackoverflow.com/a/23013020/5583560

The fact that the Mixin annotation needs to be invisible under the specific category of annotations is not particularly surprising to me. If you aren't aware, SpongePowered/Mixin is a library which applies traits via ASM/bytecode manipulation; this is extremely useful if, say, you need to modify the behavior of a compiled JAR file. I would not be surprised if it was reading bytecode to detect annotations rather than the standard method of reflection.

It may help to experiment with some more minimal reproductions but I'm not sure if those need to be done with the Mixin JAR or the standard example.

@Simn
Copy link
Member

Simn commented Nov 14, 2023

Ah, so if we have RetentionPolicy.CLASS it ends up as RuntimeInvisibleAnnotation and if it's RetentionPolicy.RUNTIME it becomes RuntimeVisibleAnnotation? I never made the connection between the two but spelling it out like this makes some sense.

I'm happy to have test cases using the Mixin JAR if we can figure out a way to make this work on our CI. Ideally without having to install large amounts of Java tooling... and the fact that https://files.minecraftforge.net/maven/ currently gives me a blank page with a 404 resource error in the console isn't very encouraging.

@EliteMasterEric
Copy link
Contributor Author

I'd recommend using Fabric's Maven for it. https://maven.fabricmc.net/net/fabricmc/sponge-mixin/0.12.5%2Bmixin.0.8.5/

@EliteMasterEric
Copy link
Contributor Author

EliteMasterEric commented Nov 25, 2023

Not sure if it's still useful but I was able to create a minimal (or as minimal as feasible) project which includes:

  • A Hello World application
  • A mod loader application which loads mod JARs into the classpath and executes their Mixins (literally the Fabric Minecraft mod loader but targeting Hello World instead of Minecraft)
  • An example mod which uses a mixin to modify Hello World. My goal is to be able to create a Haxe project which exports a JAR which functions identically to this.

https://github.com/EliteMasterEric/HelloWorldFabric

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 a pull request may close this issue.

2 participants