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

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

Open
Su5eD opened this issue Jun 17, 2023 · 5 comments
Open

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

Su5eD opened this issue Jun 17, 2023 · 5 comments

Comments

@Su5eD
Copy link
Contributor

Su5eD commented Jun 17, 2023

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.

for (IMappingFile.IField fld : cls.getFields()) {
String name = obf.remapField(fld.getMapped());
if (name.startsWith("field_") || name.startsWith("f_"))
sfields.put(name, fld.getOriginal());
}
for (IMappingFile.IMethod mtd : cls.getMethods()) {
String name = obf.remapMethod(mtd.getMapped(), mtd.getMappedDescriptor());
if (name.startsWith("func_") || name.startsWith("m_"))
smethods.put(name, mtd.getOriginal());
}

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

cpw commented Jun 18, 2023

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
Copy link
Contributor Author

Su5eD commented Jun 18, 2023

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

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

cpw commented Jun 19, 2023

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

@LexManos
Copy link
Member

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.

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