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

Fields that shadow remappable fields get inadvertently remapped when referred to from a subclass #72

Open
quat1024 opened this issue Aug 21, 2021 · 2 comments

Comments

@quat1024
Copy link

Sry for the mouthful of a title, this is a pretty specific issue, also i produced this from inside ForgeGradle 3 and 5 and have been sent here MinecraftForge/ForgeGradle#821 !
gonna put the original issue text below Okay


forgegradle-based test case is at https://github.com/quat1024/bugtest if you wanna try it

Easiest to explain by example, so here's the scene:

public abstract class Superclass extends DirectionalBlock {
  public Superclass(Properties props) {
    super(props);
    
    registerDefaultState(defaultBlockState()
      .setValue(FACING, Direction.UP)
      .setValue(POWERED, false));
  }

  public static final EnumProperty<Direction> FACING = DirectionalBlock.FACING; //shadows a field from superclass (!)
  public static final BooleanProperty POWERED = BlockStateProperties.POWERED; //for comparison's sake
  
  @Override
  protected void createBlockStateDefinition(StateContainer.Builder<Block, BlockState> builder) {
    builder.add(FACING, POWERED);
  }
}

//...

public class Subclass extends Superclass {
  public Subclass(Properties props) {
    super(props);
  }
  
  @Nullable
  @Override
  public BlockState getStateForPlacement(BlockItemUseContext ctx) {
    return defaultBlockState()
      .setValue(FACING, ctx.getClickedFace().getOpposite())
      .setValue(POWERED, false);
  }
}

The important parts:

  • Superclass extends Minecraft's DirectionalBlock, and both classes have a field named FACING. Mine shadows theirs.
  • References to DirectionalBlock.FACING should be remapped to DirectionalBlock.field_176387_N, but references to Superclass.FACING should stay the same.
  • Subclass refers to that field.

I built a reobf jar using ForgeGradle's standard build task. One of its subtasks remaps the mod to SRG names using SpecialSource, and here's what i found:

  • Superclass correctly contains two fields named FACING and POWERED,
  • Superclass#createBlockStateDefinition correctly refers to the fields named FACING and POWERED,
  • Subclass#getStateForPlacement incorrectly refers to fields named field_176387_N and POWERED instead.

This fails at runtime with a NoSuchFieldError, see quat1024/incorporeal-2-forge#11.

👀 Only accesses from subclasses are incorrectly remapped. Referring to the public static field Superclass.FACING from an unrelated class uses the correct name of FACING

@md-5
Copy link
Owner

md-5 commented Aug 21, 2021

Are you using an inheritance map or live remap with Minecraft on the classpath? If you're not, that's probably why this happens as the bytecode for your classes does not tell us enough info to know how to remap.

This would be easiest to verify by doing a test case with the SpecialSource CLI as I'm not sure what Forgegradle does

@LexManos
Copy link
Contributor

Just for note, cuz I was curious. The end bytecode reference is:

#28 Fieldref           #1.#29         // Test$Subclass.POWERED:Lnet/minecraft/world/level/block/state/properties/BooleanProperty;
getstatic     #28                 // Field POWERED:Lnet/minecraft/world/level/block/state/properties/BooleanProperty;

So it's looking for the static field on the subclass itself, which is odd to say the least...

FG passes in the full classpath and --live
So it should be all that is needed.

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

No branches or pull requests

3 participants