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

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

Open
DenWav opened this issue Dec 29, 2020 · 3 comments

Comments

@DenWav
Copy link
Contributor

DenWav commented Dec 29, 2020

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
Copy link
Member

ichttt commented Dec 29, 2020

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

@DenWav
Copy link
Contributor Author

DenWav commented Dec 29, 2020

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

@DenWav
Copy link
Contributor Author

DenWav commented Dec 31, 2020

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.

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

2 participants