Prepend modid to accessor tag

[ekulxam]

Companion PR to #181 but for accessors
Relevant conversation: https://discord.com/channels/507304429255393322/807617284129423370/1379959874292678760

[apple502j]

Mod ID should not be prepended to accessors

[cputnam-a11y]

@apple502j , why not?

[ekulxam]

https://discord.com/channels/507304429255393322/807617700734042122/1246229871810707610 case where prepending modid could have prevented an issue

[sylv256]

https://discord.com/channels/507304429255393322/807617700734042122/1246229871810707610 case where prepending modid could have prevented an issue

That's an edge case caused by bad practice. Really what should be discouraged is non-interface-based accessors.

[Earthcomputer]

why not?

because Mixin merges accessors properly unlike other methods.

https://discord.com/channels/507304429255393322/807617700734042122/1246229871810707610 case where prepending modid could have prevented an issue

This looks like a Mixin bug to me, Mixin should be selecting the weakest access modifier when merging accessors

[sylv256]

This should probably be brought up in a Fabric Mixin issue since it really shouldn't be preferring private over public accessors.

[cputnam-a11y]

why not?

because Mixin merges accessors properly unlike other methods.

I disagree. There was a discussion on discord about this, generally ending around llama saying:

accessors and invokers should absolutely be prefixed with your modid

here

[Earthcomputer]

The conversation in fact didn't end there, and prefixing accessors by mod id is generally not what people do. Given that accessors don't clash with each other, I think rather than recommending every mod do the incredibly annoying thing of prefixing all their accessors, we put a few extra safeguards in Mixin to make sure that accessors can't overwrite other methods, maybe behind the overwrites.requireAnnotations flag (which will soon be enabled by default in the fabric mod template).

[Gamebuster19901]

Jumping in from FabricMC/Mixin#171

The issues described to me look like a bug in mixin not applying visibility correctly when there are multiple visibilities. The prefix doesn't seem to have much to do with it.

Some people stated that there are some cases in obfuscated environments where modid may be required to avoid conflicts... but is that the case for unobfuscated environments?

Are there examples for either of these cases? I can't think of any.

[cputnam-a11y]

Some people stated that there are some cases in obfuscated environments where modid may be required to avoid conflicts... but is that the case for unobfuscated environments?

fabric doesn't cover those cases.

[cputnam-a11y]

The conversation in fact didn't end there

this is fair. I'm not trying to imply that it has ended in any way. it's just very spread out through multiple issues, and multiple mediums, so I'm trying to keep everyone up to date. I obviously have an opinion, but please disregard that, I can write whatever I want in my own mixins, regardless of what the small help tag says

[Gamebuster19901]

Some people stated that there are some cases in obfuscated environments where modid may be required to avoid conflicts... but is that the case for unobfuscated environments?

fabric doesn't cover those cases.

That's not correct. Fabric's fork of mixin is used in fabric loader. Fabric loader is a general purpose mod loader for any java game. It can be used on obfuscated and unobfuscated games.

[cputnam-a11y]

That's not correct. Fabric's fork of mixin is used in fabric loader. Fabric loader is a general purpose mod loader for any java game. It can be used on obfuscated and unobfuscated games.

I'm sorry I was misleading then. I wasn't implying that fabric mixin doesn't cover those cases.

[Earthcomputer]

I've just tested it, looks like accidental overwriting via an accessor is already prevented if you have overwrites.requireAnnotations set to true, so that's basically the only problem with them solved.

[LlamaLad7]

so that's basically the only problem with them solved

if you don't consider the game crashing to be a problem then I suppose so

[Gamebuster19901]

Other than with the visibility bug, how do you get a game crash?

I think maybe if two mods try to name an accessor the same thing, but for some reason target different fields would really be the only way, right?

Something like:

//mod 1
@Accessor
public Foo getFoo();

//mod 2
@Accessor("bar")
public Bar getFoo();

[Earthcomputer]

There are two ways to get a game crash (only happens with overwrites.requireAnnotations):

// mod
@Accessor
Foo getFoo();

where the target class adds a getter for foo in an updated version (a porting bug).

Or:

// mod 1
@Accessor
Foo getFoo();

// mod 2, not an accessor
public Foo getFoo();

in the latter case, mod 2 should have either marked their method as @Unique or prefixed it with their modid, so the fault is on mod 2 rather than mod 1 not prefixing its accessors.

Your example causes no conflict as accessors can be merged. misread, technically your example causes a conflict too but that looks extremely unlikely

[Gamebuster19901]

I've thought it over for a while and this is my opinion.

I agree that edge cases do exist. However the ones we've identified so far are either extremely contrived (like two mods defining accessors with the same name but targeting different fields), easily preventable with existing tooling (@Unique, overwrites.requireAnnotations), or are just expected side effects when the target class changes upstream.

I'm totally fine with mods choosing to prefix their accessors if that works for them. Personally, I use suffixes (e.g. getFooWF()) so my IDE can more easily suggest the method, and so it doesn't break natural java naming patterns. I find that to be easier and less intrusive.

That said, I don’t think mixin or related tooling should try to automatically handle modid prefixes/suffixes UNLESS it can be done in a way that guarantees no breakage or unexpected behavior. (see FabricMC/Mixin#171)

[ekulxam]

Okay, so from my understanding, these are the conclusions so far:

1. Crashes/incompatibilities do exist; however, they seem to be less likely than most other mixin conflicts.
2. We have no consensus on proper accessor naming. Some people say prefix, some people say suffix, some people say leave it as is.
3. We don't have a way to enforce a nonexistent consensus.

That being said, I still would prefer my mixins not to conflict with others' mixins at all, even if the solution can be enabled by a simple flag. I try to write for preemptive compatibility and not, "Oh my mod broke because of this, time to make a patch."

Also, I probably should not have created #183 after this was closed, but my intention was to bring more attention to the original PR (and it seems as though I have succeeded somewhat). Apologies for that.

[ekulxam]

Additionally (extra thought that could be completely useless), we somewhat assumed that the conflicts that we found were the only conflicts that could have occurred due to not naming accessors/invokers differently. I haven't been around for a long time, but there might be more common cases that we haven't seen yet (maybe this PR is too early).