Skip to content

Use actual parameter names instead of generic 'argN' names#191

Merged
LlamaLad7 merged 6 commits intoFabricMC:mainfrom
zOnlyKroks:main
Dec 22, 2025
Merged

Use actual parameter names instead of generic 'argN' names#191
LlamaLad7 merged 6 commits intoFabricMC:mainfrom
zOnlyKroks:main

Conversation

@zOnlyKroks
Copy link
Member

When LVT is unavailable at injection point, extract parameter names from method's local variable table as fallback instead of generating generic names like 'arg0', 'arg1', etc.
Hopefully fixes #189

Copy link

@Earthcomputer Earthcomputer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three problems:

  • Locals.getLocalsAt also needs to be changed.
  • Even though the this parameter is going to be named this in the LVT by any Java compiler, it would be good to fetch this one from the LVT if it is there as well. Maybe some compilers from some other JVM languages might name it differently.
  • This needs to be gated behind a Fabric compatibility version so it doesn't break anyone that is somehow relying on the old names. See FabricUtil and Locals.getLocalsAt.

Copy link

@Earthcomputer Earthcomputer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot of duplicated code in this PR now, the duplicates could easily be merged into one method.

Inside LocalVariableDiscriminator, the compatibility version isn't checked, and the this name still isn't taken into account.

@zOnlyKroks
Copy link
Member Author

There's a lot of duplicated code in this PR now, the duplicates could easily be merged into one method.

Inside LocalVariableDiscriminator, the compatibility version isn't checked, and the this name still isn't taken into account.

seems my commit got banished into the shadow realm that resolved that

@zOnlyKroks zOnlyKroks requested a review from LlamaLad7 December 19, 2025 15:31
Copy link
Member

@LlamaLad7 LlamaLad7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things left. Also happy to finish it up myself but you're more than welcome to.

Copy link
Member

@LlamaLad7 LlamaLad7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will test thoroughly either later today or tomorrow morning, this is the last thing I can see for now.

@LlamaLad7 LlamaLad7 merged commit 41a0ead into FabricMC:main Dec 22, 2025
1 check passed
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 this pull request may close these issues.

Local variable capture does not support capturing by name for arguments

3 participants