bad class file for SPacketPlayerListItem$AddPlayerData due to bad RuntimeInvisibleParameterAnnotations attribute, when setupDecompWorkspace has not been run

[Pokechu22]

This has already been reported as MinecraftForge/MinecraftForge#3238, but it's still an issue, so I'm creating a new issue here to keep track of it.

C:\Pokechu22\Git_\SetupCITest\build\sources\main\java\com\mumfrey\DummyFile.java:31: error: cannot access AddPlayerData
    public final SPacketPlayerListItem.AddPlayerData getEntry() {
                                      ^
  bad class file: C:\Users\Pokechu22\.gradle\caches\minecraft\net\minecraft\minecraft\1.12.2\snapshot\20160517\minecraftBin-1.12.2.jar(net/minecraft/network/play/server/SPacketPlayerListItem$AddPlayerData.class)
    bad RuntimeInvisibleParameterAnnotations attribute: AddPlayerData(GameProfile,int,GameType,ITextComponent)
    Please remove or make sure it appears in the correct subdirectory of the classpath.

To reproduce, use https://github.com/Mumfrey/SetupCITest/ (edit the versions in it, though, to be 1.12.2 and FG 2.3), and run gradle build without setupDecompWorkspace. The above error should occur.

[LexManos]

As with last time, I am not able to reproduce this.

[liach]

you can produce it by building spongecommon

[Pokechu22]

As with last time, I am not able to reproduce this.

Was not aware of that aspect. I'm sure you've already checked, but to be sure you're running off of a clean cache; it's not using the cached decomp'd version?

I record an MD5 of 31c039eedcfdc4236a4485275d288fc0 for that class (using 1.12.2 and the 20160517 mappings that come with SetupCITest); is that the MD5 you're getting?

[Pokechu22]

Also... just found something somewhat sad, but it explains why the message happens even though there is only RuntimeVisibleParameterAnnotations, not RuntimeInvisibleParameterAnnotations:

First, the declaration for the message is here, and its translation key is compiler.misc.bad.runtime.invisible.param.annotations.

Just a quick aside. It was a HUGE pain to actually download the compiler source. For whatever reason, Mercurial hates me. It would constantly download around 50% of the project, then say "transaction abort!" and delete the whole thing. I eventually managed to get it to work by following these steps, except I only downloaded 1 revision the first time (after that, though, I could download ~500 at a time).

Ok, next. Here's the code that actually raises that error, with the helpful comment "Attach parameter annotations." Looks fairly reasonable. But, it's a bit of a troll. That code is used to read RuntimeInvisibleParameterAnnotations... but it's also used for RuntimeVisibleParameterAnnotations, with the same error message. Wow, thanks.

Anyways, that explains why the message seems to be lying about what's in the file — it is. I'll look into what the diff is between the files next.

[Pokechu22]

OK, here's a diff of the constructor to SPacketPlayerListItem, as given through javap -v. - is bin, + is src.

   public net.minecraft.network.play.server.SPacketPlayerListItem$AddPlayerData(net.minecraft.network.play.server.SPacketPlayerListItem, com.mojang.authlib.GameProfile, int, net.minecraft.world.GameType, net.minecraft.util.text.ITextComponent);
     descriptor: (Lnet/minecraft/network/play/server/SPacketPlayerListItem;Lcom/mojang/authlib/GameProfile;ILnet/minecraft/world/GameType;Lnet/minecraft/util/text/ITextComponent;)V
     flags: ACC_PUBLIC
     Code:
       stack=2, locals=6, args_size=6
          0: aload_0
          1: aload_1
-         2: putfield      #33                 // Field field_179968_a:Lnet/minecraft/network/play/server/SPacketPlayerListItem;
+         2: putfield      #1                  // Field this$0:Lnet/minecraft/network/play/server/SPacketPlayerListItem;
          5: aload_0
-         6: invokespecial #36                 // Method java/lang/Object."<init>":()V
+         6: invokespecial #2                  // Method java/lang/Object."<init>":()V
          9: aload_0
         10: aload_2
-        11: putfield      #38                 // Field profile:Lcom/mojang/authlib/GameProfile;
+        11: putfield      #3                  // Field profile:Lcom/mojang/authlib/GameProfile;
         14: aload_0
         15: iload_3
-        16: putfield      #40                 // Field ping:I
+        16: putfield      #4                  // Field ping:I
         19: aload_0
         20: aload         4
-        22: putfield      #42                 // Field gamemode:Lnet/minecraft/world/GameType;
+        22: putfield      #5                  // Field gamemode:Lnet/minecraft/world/GameType;
         25: aload_0
         26: aload         5
-        28: putfield      #44                 // Field displayName:Lnet/minecraft/util/text/ITextComponent;
+        28: putfield      #6                  // Field displayName:Lnet/minecraft/util/text/ITextComponent;
         31: return
+      LineNumberTable:
+        line 197: 0
+        line 198: 9
+        line 199: 14
+        line 200: 19
+        line 201: 25
+        line 202: 31
       LocalVariableTable:
         Start  Length  Slot  Name   Signature
             0      32     0  this   Lnet/minecraft/network/play/server/SPacketPlayerListItem$AddPlayerData;
-            0      32     1 p_i46663_1_   Lnet/minecraft/network/play/server/SPacketPlayerListItem;
-            0      32     2 p_i46663_2_   Lcom/mojang/authlib/GameProfile;
-            0      32     3 p_i46663_3_   I
-            0      32     4 p_i46663_4_   Lnet/minecraft/world/GameType;
-            0      32     5 p_i46663_5_   Lnet/minecraft/util/text/ITextComponent;
-      LineNumberTable:
-        line 187: 0
-        line 188: 9
-        line 189: 14
-        line 190: 19
-        line 191: 25
-        line 192: 31
+            0      32     1 this$0   Lnet/minecraft/network/play/server/SPacketPlayerListItem;
+            0      32     2 profileIn   Lcom/mojang/authlib/GameProfile;
+            0      32     3 latencyIn   I
+            0      32     4 gameModeIn   Lnet/minecraft/world/GameType;
+            0      32     5 displayNameIn   Lnet/minecraft/util/text/ITextComponent;
     RuntimeVisibleParameterAnnotations:
       parameter 0:
       parameter 1:
       parameter 2:
       parameter 3:
-        0: #31()
-      parameter 4:
-        0: #31()
+        0: #42()

Hm, yeah, that does look incorrect; bin has 5 parameters while src has 4. I do see 5 parameters on the method, but one of them is the synthetic this$0 parameter for the enclosing class. The JVMS says that "The value of the num_parameters item gives the number of formal parameters of the method represented by the method_info structure on which the annotation occurs." (and that "This duplicates information that could be extracted from the method descriptor.", which seems to be what the check I found before is doing). I'm not fully sure whether that synthetic thing counts as a formal parameter. I'll investigate that further.

Let's check the unobf'd version of the class, jp$b. ... it has it too.

class Why {
	public jp.b getAddPlayerData() {
		return null;
	}
}

Compile the above code with javac -cp 1.12.2.jar Why.java and you will get

Why.java:2: error: cannot access b
        public jp.b getAddPlayerData() {
                 ^
  bad class file: 1.12.2.jar(jp$b.class)
    bad RuntimeInvisibleParameterAnnotations attribute: b(GameProfile,int,ams,hh)
    Please remove or make sure it appears in the correct subdirectory of the classpath.

So, this isn't FG's fault. But I don't know where else to go from here. Only thing I know to do is to look into the formal parameters bit.

[Pokechu22]

In an attempt to reproduce this, I tried type to make use of the receiver parameter (SO). No dice:

import java.lang.annotation.*;

public class RVA {
	@Target({ElementType.TYPE_USE})
	@Retention(RetentionPolicy.RUNTIME)
	public @interface Foo {}
	public class Inner {
		public Inner(@Foo RVA RVA.this, @Foo RVA i) {
			System.out.println("Outer is " + RVA.this);
			System.out.println("i is " + i);
		}
	}
	public static void main(String[] args) {
		RVA rva = new RVA();
		System.out.println(rva.new Inner(null));
	}
}

This generates the following table:

    RuntimeVisibleTypeAnnotations:
      0: #24(): METHOD_RECEIVER
      1: #24(): METHOD_FORMAL_PARAMETER, param_index=0

which is perfectly valid.

And not using TYPE_USE disalows the receiver parameter:

import java.lang.annotation.*;

public class RVA2 {
	@Target({ElementType.PARAMETER})
	@Retention(RetentionPolicy.RUNTIME)
	public @interface Foo {}
	public class Inner {
		public Inner(RVA2 RVA2.this, @Foo RVA2 i) {
			System.out.println("Outer is " + RVA2.this);
			System.out.println("i is " + i);
		}
	}
	public static void main(String[] args) {
		RVA2 rva = new RVA2();
		System.out.println(rva.new Inner(null));
	}
}

and I get a correct table (which is probably why the recompiled jar works):

    RuntimeVisibleParameterAnnotations:
      parameter 0:
        0: #24()

So this is either some stupidly subtle compiler bug that only happens on Mojang's end, or a bug with the obfuscator.

[Pokechu22]

After some internal discussion, looks like this isn't a compiler bug. So instead it's probably an obfuscator issue.

Also, this is probably the same issue as ModCoderPack/MCPBot-Issues#257 - tentatively, my guess is that the synthetic this$0 parameter is causing annotations in inner class constructors to be shifted by 1, which creates that malformed result there.

[Pokechu22]

Oh wow. Looks like I'm not the only one who had issues; there's an OW2 commit that seems to go into detail about this too and before OW2 was marking things as synthetic.

And the spec has changed:

SE 8, §4.7.18

num_parameters

The value of the num_parameters item gives the number of formal parameters of the method represented by the method_info structure on which the annotation occurs.

This duplicates information that could be extracted from the method descriptor.

SE 9, §4.7.18

num_parameters

The value of the num_parameters item gives the number of run-time visible parameter annotations represented by this structure.

There is no assurance that this number is the same as the number of parameter descriptors in the method descriptor.

(those aren't the only changes!) And more interestingly, javac still gives the same (misleading) error message for malformed templates even on the latest JDK 10 prerelease. So this isn't fixed by updating javac, even though the spec seems to be more tolerant of it...

Still looking into how to properly recover from this, and for the moment I'm thinking of adding something to Exceptor. But this is pretty weird.

EDIT: I didn't mention it, but Exceptor does use a version of ASM before that change (it still is supposed to generate synthetic markers, though in this case it doesn't).

[LexManos]

So, just an update for those at home. We have identified the issue. ProGuard, the Obfuscator that Mojang uses is breaking the annotation attribute for some reason we can't seem to find the justification for.
https://sourceforge.net/p/proguard/code/ci/719eb1b7433dbd87973273b30cfbac0f1db9972d/tree/core/src/proguard/classfile/io/ProgramClassReader.java#l745
All compilers we've tested so far produce a RuntimeVisibleParameterAnnotations for all non-synthetic methods.
PG 'fixes' this by adding empty entries for the synthetic parameters.
Fernflower, as it should, indexes only the non-synthetic parameters. Hence what appears to be a 'truncation' or 'shift'.

Currently, we're thinking about writing a solution into Exceptor. But it's difficult to determine what arguments to a constructor is synthetic. Normal named inner classes is simple as it's only ever 1 (parent$this), but named local classes may have as many synthetic arguments as they want.

[Pokechu22]

I've created ModCoderPack/MCInjector#3 that should hopefully fix this. See there for more technical information about how I'm deciding whether an argument is synthetic.

I've also reported the misleading compiler output to OpenJDK; there is a ticket for that: JDK-8197558.

As Lex pointed out, this is where ProGuard adds the extra entries. Here's the ProGuard bug report that led them to add it, which in turn references JDK-6332964. Apparently back in Java 5, the reflection API would reject annotations without this fix, so ProGuard was sort of correct to fix it. Now though, the reflection code gives up and just doesn't validate the lengths. The spec doesn't make clear which version is correct (until Java 9, where it simply gives up and says that anything is correct).