search

MinecraftForge / ForgeFlower

Forge's modifications to FernFlower. Fixing various bugs/inconsistencies. Main Repo: https://github.com/MinecraftForge/FernFlower
Apache License 2.0
80 stars 44 forks source link

Local variable names in inner class methods aren't fixed when they conflict with outer names #86

Open DenWav opened 3 years ago

DenWav commented 3 years ago

This is due to https://github.com/MinecraftForge/ForgeFlower/blob/master/FernFlower-Patches/0037-Do-not-rebuild-variable-names-in-lambdas.patch

For background on what that code is doing (re: the patch message): That allows the decompiler to fix variable names in a nested class or lambda expression which clashes with the name of an outer variable.

What's happening is it's checking the names for the current method against the variable names defined in the outer scope. That's why setNewOuterNames is passed to VarNamesCollector, then checked for in VarNamesCollector.getFreeName() - the outer names are compared against the names for the nested method.

What FernFlower's code originally did was append x to local variable names when conflicts came up - but now it doesn't do that because of that patch. Considering that's the only thing that method is actually doing I don't know what the patch is actually trying to fix. The commit which added the patch said it fixes #88, but there is no issue 88 yet.

This comes up relatively infrequently, and I guess never in MCP due to how LVT is handled, but it causes an issue with my usage. This quick and dirty fix is because of this issue - previous versions of FernFlower would rename the inner i variable to ix. https://github.com/PaperMC/Paper/blob/mappings/mojang/Spigot-Server-Patches/0266-Optimize-BlockPosition-helper-methods.patch#L113-L134

ichttt commented 3 years ago

See the comment on the commit, he meant to reference #80

DenWav commented 3 years ago

Ah, so this is easy to fix just by excluding this and keeping the method call. I'll PR that tomorrow.

DenWav commented 3 years ago

Okay patch 9 totally changes how variables are named and so it actually makes patch 37 a little odd considering it's not doing anything. But I cannot follow the new system, and can't figure out how to backport this feature onto it. I'll just leave this issue here and hope someone smarter than me can figure it out.