Skip to content

Conversation

@NebelNidas
Copy link
Member

@NebelNidas NebelNidas commented Sep 24, 2022

@NebelNidas NebelNidas force-pushed the prevent-enum-and-constructor-renames branch from b3e3bf0 to 906a2f2 Compare September 24, 2022 15:11
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Idk what this test class is doing?

@NebelNidas NebelNidas force-pushed the prevent-enum-and-constructor-renames branch from 3dcc479 to f811927 Compare September 24, 2022 20:59
@NebelNidas
Copy link
Member Author

Sorry, this should have remained a draft since there were some issues in my cherry-picking. Force-pushed the rewritten commits now so that we don't have the tests in yet, I'll add those under the correct packages in a later PR

@NebelNidas
Copy link
Member Author

@liach What do you dislike about this PR? 🤔

@liach
Copy link
Contributor

liach commented Sep 26, 2022

Such preventions are quite pointless; we do't prevent renaming equals, enum entries, record components, or other overrides either

@NebelNidas
Copy link
Member Author

Shouldn't Enigma do this though?

@liach
Copy link
Contributor

liach commented Sep 26, 2022

We have cases where we rename an override chain inferable through record component names. IMO marking these gray through name proposal service would be better, especially that these names can inadvertently cause troubles due to proguard works and we wish to rename in such cases

@NebelNidas
Copy link
Member Author

OK, I'll close this PR then and commit the constructor remap fix in a new one

@NebelNidas
Copy link
Member Author

NebelNidas commented Oct 6, 2022

... we do't prevent renaming equals ...

What does this do then?

} else if (name.equals("equals") && sig.equals("(Ljava/lang/Object;)Z")) {
return false;

Also, while it's not explicitly disallowed to rename values and valueOf yet, Enigma doesn't support it anyway either... So adding the check to isRenamable doesn't restrict anyone, instead it would accurately report if Enigma supports renaming the passed element or not

@NebelNidas NebelNidas changed the title Prevent enum and constructor renames Make enum methods values and valueOf unmappable Oct 6, 2022
Copy link
Member

@modmuss50 modmuss50 left a comment

Choose a reason for hiding this comment

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

Change is fine, this could maybe refactored to not allow remapping stuff from Java? Or maybe a larger change where we have a classpath of none re nameable libraries im not sure. Needs some discussion first if we want to do it.

@modmuss50 modmuss50 merged commit df86dab into FabricMC:master Dec 7, 2022
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.

Enum methods are marked as deobfuscated and can be renamed

4 participants