MinecraftForge / ForgeGradle

Minecraft mod development framework used by Forge and FML for the gradle build system
GNU Lesser General Public License v2.1
526 stars 446 forks source link

Generated methods CSV doesn't support record method names #922

Open Su5eD opened 1 year ago

Su5eD commented 1 year ago

Currently, the generator for method and field CSV files, OfficialChannelProvider, distinguishes methods and fields by their prefix, which is func_ / m_ for methods and field_ / f_ for fields. However, in records, the names of accessor methods are the same as the field they're accessing. As an example, see NoiseGeneratorSettings.surfaceRule.

https://github.com/MinecraftForge/ForgeGradle/blob/651e43fd7c68068c835548583c5eb924e13d9251/src/mcp/java/net/minecraftforge/gradle/mcp/OfficialChannelProvider.java#L99-L108

This leads to all record method names being filtered out as fields by the generator.
When the mappings are later used at runtime by modlauncher for remapping class member names, record methods will never be remapped due to this issue.

cpw commented 1 year ago

The record field accessor methods are likely marked as synthetic by the compiler, which likely means they should not even be in the decompile.

Are you trying to reflect them at runtime and hitting an error, or is it a "they seem to be missing"? Because I would generally expect them to be missing. Note that at runtime deobf, they probably deobf to their field name because they're synthetic.

Su5eD commented 1 year ago

Runtime deobf doesn't work at all because the default modlauncher naming service loads its method mappings from methods.csv. And since record method names are filtered out from that file due to this bug, remap calls will always fall back to the obfuscated name.

All the required method names are already present in the SRG input of OfficialChannelProvider, they just get filtered out because they begin with the field name prefix f_.

LexManos commented 1 year ago

To make this clear, it has nothing to do with things being synthetic or not. This is a bug in FML, or at least a misunderstanding of design. The SRG names are meant to be unique and treated as a lookup in a single dictionary Having them split by FML means that record names do not get the field names as they are intended.

The fix would be to update FML's Naming Service, but it can also be fixed on our end by duplicating the data into both csv files. So feel free to PR FG to add "f_" to the list of methods so it's retroactive.

cpw commented 1 year ago

Fixing name service is quite doable. We'd need to identify the special case where the method is a field name somehow..

LexManos commented 1 year ago

The point is they shouldn't be special cased. It'd just need to load both the methods and fields csv into a single dictionary.