Open magneticflux- opened 2 years ago
This is intended behaviour, although I agree that it's poorly documented in the Javadocs
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 parenttrue
: explicitly override parent if necessary, remap this element and childrenfalse
explicitly override parent if necessary, do not remap this element or childrenWhile 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 toremap=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.
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 withremap = 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.
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.
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), theremap
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 withremap = true
is copied from the@Mixin
javadocs to the other places whereremap
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.
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 istrue
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 thatremap=true
is the default always so the inner annotation would not need to haveremap=true
explicitly stated. What actually happens though is the remap value is set for the "context" and so the default for the inner annotation becomesremap=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 toremap=true
regardless of "parent" annotations.