SpongePowered / Mixin

Mixin is a trait/mixin and bytecode weaving framework for Java using ASM
MIT License
1.37k stars 185 forks source link

Clarify that ModifyArgs.index is the absolute index, not relative #666

Open Sollace opened 2 months ago

Sollace commented 2 months ago

If the target method has the argument (double a, double b, double c, float d, float e) an index of 1 targets the double b argument, not float e as one might assume from the second paragraph.

Geolykt commented 2 months ago

What exactly do you mean with relative here? Because I have the small feeling that using the word "absolute" without elaborating on it might be a bit confusing to some people.

Sollace commented 2 months ago

What exactly do you mean with relative here? Because I have the small feeling that using the word "absolute" without elaborating on it might be a bit confusing to some people.

As the commit description says:

If the target method has the argument `(double a, double b, double c, float d, float e)`
an index of 1 targets the `double b` argument, not `float e` as one might assume from the second paragraph.

Relative means the index starts with the first matching argument (same type). Absolute is index starts with the first argument.

If you look at the original doc:

 * <p>Gets the argument index on the target to set. It is not necessary to
     * set this value if there is only one argument of the modifier type in the
     * hooked method's signature. For example if the target method accepts a
     * boolean, an integer and a String, and the modifier method accepts and
     * returns an integer, then the integer parameter will be automatically
     * selected.</p>

This description doesn't make that clear, and if you read the second sentence you get the impression that the index is counted separately for each type (relative) which makes it extremely confusing when an inject like:

@ModifyArg(method = "attack", at = @At(value = "INVOKE", target = "setVelocity(DDDFF)"), index = 1)
private float modifyDivergence(float divergence) {

- doesn't work because from reading the docs you're led to expect 1 to point to the second float parameter, not the second double.

Sollace commented 2 months ago

I got the "absolute" vs "relative" descriptor from asking on the Fabric discord why the above injector doesn't work, and the answer I got was "it should be an absolute index".

Geolykt commented 2 months ago

While I understand the semantics of index and thus can infer your definition of absolute based on that knowledge, the problem I have is that someone that is unfamiliar with what "absolute" means will get tripped up and confused after reading that word and attribute it a meaning it doesn't have (e.g. them thinking that an index = 0 modifies the reciever, which isn't the case I think? I might have confused myself right now lol. Another possible pitfall that could stem from excessive thought is attributing types of computational type category 2 to be two indices long). The other scenario is that they completely ignore that word in which case it doesn't help anything. But perhaps you (or someone else) are the better judge of that.

Sollace commented 2 months ago

I am open to a better way to describe it. IMO I still think it's better to have it than to not.

Earthcomputer commented 2 months ago

I don't understand how you could possibly get confused with the existing documentation. Why on Earth would you assume that the index is relative? Generally indexes are absolute unless otherwise specified.

Earthcomputer commented 2 months ago

I think there is a very simple typo in the original documentation. on -> of. Then any possible confusion is cleared.

Mumfrey commented 2 months ago

@Sollace wrote: Relative means the index starts with the first matching argument (same type). Absolute is index starts with the first argument.

Except that Mixin is already consistent in its use of ordinal to descibe "index within a group" and index to mean "index within scope". Personally I think the language of the document is fine and if anything it's just the example which could be tweaked to include a disambiguating circumstance, such as adding an differently-typed argument at the start of the example candidate method which shows that it is included in the consideration for index.

@Earthcomputer wrote I think there is a very simple typo in the original documentation. on -> of. Then any possible confusion is cleared.

Sure, I'll buy that for a dollar.