Inheritance of `remap` is inadequately documented

[magneticflux-]

Okay, so this is a little confusing since it was completely unexpected to me, but in short: the behavior changes if you supply remap=true to a certain mixin even though the default value for the annotation is true anyway. I find this extremely counterintuitive.

My situation was that I was trying to target a remapped method inside of a non-remapped method. The outer method was marked remap=false. What I assumed was that remap=true is the default always so the inner annotation would not need to have remap=true explicitly stated. What actually happens though is the remap value is set for the "context" and so the default for the inner annotation becomes remap=false with no warning.

Either this (potentially useful, but also potentially confusing) quirk should be clearly documented in each remap Javadoc or the value should always default to remap=true regardless of "parent" annotations.

[Earthcomputer]

This is intended behaviour, although I agree that it's poorly documented in the Javadocs

[Mumfrey]

As EarthComputer says this is the intended behaviour, I think the confusion actually arises from the fact that Java has a weird contract for "default" values of annotations which is used by the Mixin AP to tristate the value, in that there's a difference between a value being specified as the "default" and not being specified at all.

The tristate is:

• value not specified: inherit remap from parent
• true: explicitly override parent if necessary, remap this element and children
• false explicitly override parent if necessary, do not remap this element or children

While the tristate characteristic could be documented better, I chose this behaviour as the most intuitive since in general if a thing is being decorated with remap=false then it should mean "this thing and all the things it contains", e.g. putting remap=false on a @Mixin means you don't need to decorate every single injector, @At and @Overwrite inside that mixin with remap=false as well, which would be annoying and noisy. But an explicit remap=true lets you override that behaviour on an element-by-element basis.

What actually happens though is the remap value is set for the "context" and so the default for the inner annotation becomes remap=false with no warning.

I disagree that there should be a "warning" in this case. You told it the element was remappable so the AP complaining that you told it something wouldn't be very helpful as then you'd need a @SuppressWarnings for the desired behaviour. The AP did what you told it to - it didn't remap the injector - so a warning would be inappropriate.

Either this (potentially useful, but also potentially confusing) quirk should be clearly documented in each remap Javadoc or the value should always default to remap=true regardless of "parent" annotations.

I feel like calling this a "quirk" is kind of rude, this is designed behaviour which is intended to make remap=false more intuitive ("don't remap this thing") and it only becomes quirky if you bypass the javadoc (which explains the behaviour) and look at the code, which because of Java conventions the "default" value has to be supplied, even though this is treated as a tristate in practice.

The javadoc for remap says

This can be done on an individual member basis by setting remap to false on the individual annotations, or disabled for the entire mixin by setting the value here to false. Doing so will cause the annotation processor to skip all annotations in this mixin when building the obfuscation table unless the individual annotation is explicitly decorated with remap = true.

Which in my mind clearly articulates that the remap behaviour will be inherited unless explicitly overridden. However since some confusion has clearly crept in anyway, I will attempt to clarify further and maybe just write a more comprehensive documentation article on remapping in general.

[magneticflux-]

I chose this behaviour as the most intuitive since in general if a thing is being decorated with remap=false then it should mean "this thing and all the things it contains"

I agree, it would have been completely intuitive if I didn't know anything about the JVM. As it is, I know just enough to be wrong: when I looked at the code I just assumed that there was no way to determine a "default"/null value for a primitive annotation field and so this system would be impossible to implement. Clearly the Java reflection APIs work in mysterious ways when it comes to primitive values!

only becomes quirky if you bypass the javadoc (which explains the behaviour) and look at the code, which because of Java conventions the "default" value has to be supplied, even though this is treated as a tristate in practice.

I agree, I only went to the code because in this particular instance (in the @Redirect, @Inject, and @At annotations), the remap javadocs don't mention the tristate / defaulting property like in the @Mixin javadocs.

Which in my mind clearly articulates that the remap behaviour will be inherited unless explicitly overridden.

I agree, if I had seen these docs from the @Mixin annotation I would have realized what was going on. If the note about explicitly decorating with remap = true is copied from the @Mixin javadocs to the other places where remap has this behavior (or a similar note about it being a tristate), I think that would greatly reduce potential for confusion.

[Mumfrey]

I agree, it would have been completely intuitive if I didn't know anything about the JVM. As it is, I know just enough to be wrong: when I looked at the code I just assumed that there was no way to determine a "default"/null value for a primitive annotation field and so this system would be impossible to implement. Clearly the Java reflection APIs work in mysterious ways when it comes to primitive values!

Annotation values as they are actually stored in the class are actually all just converted to string, if the value is omitted in the source code then it's simply not written into the compiled class at all, this is to save space. Mixin never deals with the source, it only ever deals with the partially-compiled AST in the AP (via Mirror) and at runtime via the compiled bytecode. Since both of these methods just work with the "raw" annotation data, it's trivial to see if a value is missing or specified. I don't know if regular reflection supports this, but reflection is not used anywhere in Mixin to access the annotation data.

I agree, I only went to the code because in this particular instance (in the @Redirect, @Inject, and @At annotations), the remap javadocs don't mention the tristate / defaulting property like in the @Mixin javadocs.

This is why I agreed the documentation needs expanding upon, but the behaviour itself is intended.

I agree, if I had seen these docs from the @Mixin annotation I would have realized what was going on. If the note about explicitly decorating with remap = true is copied from the @Mixin javadocs to the other places where remap has this behavior (or a similar note about it being a tristate), I think that would greatly reduce potential for confusion.

This is likely the approach I will take as I wouldn't use the term tristate in the docs, just describe the behaviour of omitting the setting vs. specifying it in the same way I do in the root annotation javadoc.