Carpet

Carpet

2M Downloads

[Internals] The method used to access private variables is inconsistent

altrisi opened this issue ยท 5 comments

commented

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 @Accessors, 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 in ExplosionAccessor).
  • Not sure if those are remapped when upgrading mappings (do the current "accessors"/@Shadows get automatically remapped? Maybe neither) They seem to be according to 40ab2dd#diff-324a163f669fb5679ca12251f2129777524e8ec9ad3531d8cda8a3ecc46f261c

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. @Injects)
  • 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.

commented

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.

commented

Thoughts?

commented

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

commented

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.

commented

I myself prefer interfaces. And since there are two working branches right now - I would refrain of any not-needed refactors.