Mixin method remap: fix desc being dropped & support wildcards#164
Mixin method remap: fix desc being dropped & support wildcards#164Moulberry wants to merge 8 commits intoFabricMC:masterfrom
Conversation
| byte[] data; | ||
| private ClassInstance mrjOrigin; | ||
| private final Map<String, MemberInstance> members = new HashMap<>(); // methods and fields are distinct due to their different desc separators | ||
| private final Map<String, MemberInstance> members = new LinkedHashMap<>(); // methods and fields are distinct due to their different desc separators |
There was a problem hiding this comment.
Should I sort based on TrMethod#getIndex or is making this a LinkedHashMap sufficient?
| String quantifier = info.getQuantifier(); | ||
| if (quantifier.equals("*") || quantifier.startsWith("{0,")) { | ||
| quantifier = ""; | ||
| } | ||
| list.add(new MemberInfo(mappedOwner, mappedName, quantifier, mappedDesc)); |
There was a problem hiding this comment.
Can you elaborate on the goal behind these lines
There was a problem hiding this comment.
Regarding quantifiers in this case, if the quantifier has a minimum of 1 or more, we cannot replicate this behaviour when emitting multiple name + desc pairs, so we should probably error
In all other cases we should just be able to keep the original quantifier
There was a problem hiding this comment.
The quantifier is stripped to match the behaviour from this PR before the rewrite (2e1a11b)
so if we know it's going to be * that we're inserting, we may as well just not
If we're passing a desc I probably wouldn't use a quantifier
I don't even really know what the point of the minimum is inside mixins itself, so I don't even know what behaviour we would need to be replicating here
Okay yeah all it does is throw an error: https://github.com/FabricMC/Mixin/blob/main/src/main/java/org/spongepowered/asm/mixin/injection/selectors/TargetSelectors.java#L248
If there's a quantifier of 1 or more I'm not sure if it's better to show a warning, remove the quantifier and continue or error and not remap at all. I'd lean towards the first option
There was a problem hiding this comment.
I could take or leave the quantifier stripping given we're now handling all kinds of quantifier, but if we're going to keep it presumably we want to handle startsWith("{,") too
There was a problem hiding this comment.
True yes startsWith("{,") should also be included, technically { followed by any whitespace followed by , also works
There was a problem hiding this comment.
Actually I just realized that, because of how the code works, using a max specifier like method{0,4} will always just try to replace it with the first 4 methods w/ descriptors. canInject() needs to be made smarter so it knows it can ignore methods if their match index is >4
2 participants
Fixed descriptor being dropped when input had descriptor
Descriptor should be used in output if and only if either:
Added support for resolving multiple targets when using wildcard
See remapSeparateRemappedName test for example
After the rewrite commit, this addresses a lot of additional problems and edge cases brought up by LlamaLad7 on discord