[Internals] The method used to access private variables is inconsistent
altrisi opened this issue ยท 5 comments
This is now an issue to discuss about what should be the standard way of retrieving or setting private fields from Carpet. Currently, the great majority uses interface classes from the fakes
package, but there are also some using Mixin's @Accessor
and @Invoker
annotations.
(I) Won't include comments of other pros/cons in here, discussion available below.
In this first comment I'll try to point pros and cons for both, but since I'll include my view, I'm slightly more in favour to @Accessor
s, just because of their simplicity. Feel free to disagree, since this is just a suggestion.
Old introduction
When looking at PR #540, I was about to comment about using `@Accessor` instead of going through the hassle of creating a fake interface, implementing it, shadowing the variable, and creating the get/set/call method.However, while doing that, I saw that Carpet does that with quite a lot more things.
So I propose using @Accessor
in places where it could be useful. Would require a bit of refactor, but may make things simpler.
What is @Accessor
@Accessor
is an annotation from Mixin that, well, creates a function to access or set a variable in a target class, usually private. There is also @Invoker
, that does basically the same, but in order to call functions instead.
Using @Accessor
removes the hassle of creating an interface with the method, creating a mixin implementing it, shadowing the variables and setting the methods, and also converts what would be two classes into a single class (interface).
TL;DR It does mostly the same Carpet does in two classes with multiple lines in just two lines in a single class.
Usage
In a Mixin interface (that is, an interface annotated with @Mixin(class)
, with class being the class to access), simply add the @Accessor
or @Invoker
annotation above a function like ServerWorldProperties getWorldProperties()
. That's it. Then, to call it from the code, cast the object into the interface and call the method.
Something like (example applied to #540):
@Mixin(ServerWorld.class)
public interface ServerWorldAccessor_scarpetEventsMixin {
@Accessor
ServerWorldProperties getWorldProperties();
}
ServerWorldProperties worldProperties = ((ServerWorldAccessor_scarpetEventsMixin) world).getWorldProperties();
You can also pass a string to the annotation (@Accessor("field_name")
) to choose a specific field to access. If it's not specified, it will assume the name by removing get
/set
/is
and decapitalizing the first letter. Similar for @Invoker
.
You can see more docs in Fabric's wiki page about Accessors, including static accessors and invokers. The javadocs also contain some extra info about them.
What could be affected by this
Most of the fakes
package, except for some that also provide some extra code apart from calling a function/getting the field's content.
Am I #600?
Accessors
Those are annotations from Mixin that allow to, in a single interface, declare the accessor to the method, automatically creating the setter/getter.
Pros
- Two mixins (mods) adding the same accessor with the same name to a class won't collide (according to some empirical testing done by adding two equal mixins with different names and calling both).
- Simplifies the accessor into a single class, removes unnecessary shadows and function declarations.
- Shouldn't ever need access transformers, according to their javadocs
Cons
- Requires their class to be an interface, so they can't live together with some other mixin code (must be in a separate class). However, that wouldn't really change many things, since they are already two classes. This already happens for TNT rules
- Would make the places where it is called a bit more convoluted because of the longer mixin names, except if their classes are named in a shorter way (like
ClassNameAccessor
, used already inExplosionAccessor
). Not sure if those are remapped when upgrading mappings (do the current "accessors"/They seem to be according to 40ab2dd#diff-324a163f669fb5679ca12251f2129777524e8ec9ad3531d8cda8a3ecc46f261c@Shadow
s get automatically remapped? Maybe neither)
Implementing Carpet's interfaces in mixins
This is the current implementation used in Carpet. It consists in an interface with the getter or setter methods that lives outside of the mixins package, and then they are implemented in Carpet's mixins by defining their functions manually.
Pros
- The accessor in the Mixin classes can live together with other mixin code (i.e.
@Inject
s) - Separates the code calling the getters/setters from the mixins package
- Provides a more granular control over what should the getter do, like pre-processing the variable, converting it, etc.
Cons
- Since they are methods defined into the class, they may clash with other mixins (needs confirm). That can be workarounded by adding a custom suffix to them, although the solution may not be ideal
- In some (rare?) cases, they may require access-wideners (according to the
@Accessor
javadoc) - They require two java files (interface plus mixins), and, if not knowing about Carpet, may make methods in the
fakes
seem unrelated to anything.
Discussion below.
Lol - I know what accessors are and the only real benefit for me is #1 in the pros section. I also DO recognize their drawbacks which you pointed in Cons. Main reason I don't use them is the fact that with interfaces that separates mixins (which is like its own shady area - not really part of Java, from interfaces which organize in one place additional contracts that we have added to the vanilla classes). So even if mixins are separated for each feature, each interface encapsulates all added public behaviours per original class.
Look for example on the BlockPredicateInterface which joins two similar interfaces of two internally private classes, allowing to skip on access wideners for these. Otherwise - these classes would need to be targeted directly which would require a widener, which are nasty.
Another thing I don't like is importing classes from 'mixin' area - looks like mixing nice and clean owned code with a hack. The only drawback for me is potentially clashing with other mods, that's why I started to use more unique names, often containing CM to minimize a chance of that happening.
Lol, its not a big deal for me tbh. If you are firm on accessors, that's fine. I prefer interfaces, like I prefer Egyptian braces
I think I'm not explaining myself, sorry for my english. I'm just trying to ask/decide whether to use one or the other in the future if I ever commit a mixin to Carpet, and if current different ones to the other should be changed to match whatever is decided.
It's neither a big deal for me which method is used, it's just to not have a mix of some accessors, some interfaces, and some of any other methods that may appear, and know that if someone is asking for "how do you get variable x" you always know "it's from an implemented interface" or "it's from an accessor", instead of saying "well, it may be from an implemented interface, but make sure to look if there's an accessor just in case". That's what I meant with consistency, sorry.