add the ability to remap mixin annotation as an extension of tiny-remapper

[Logiquo]

Currently WIP. finished

Here is the list of annotation need to be remapped. Please add more if I forget something.

• @Mixin
• @Shadow
• @Overrite
• @Accessor
• @Invoker
• @Implements
• @Inject
• @ModifyArg
• @ModifyArgs
• @Redirect
• @ModifyVaribale
• @ModifyConstant
• @Desc
• @At
• @Slice
• @Descriptors

[liach]

Don't think tiny remapper should add this. This should go standalone as tiny remapper shouldn't have any relationship to mixin.

[Logiquo]

It can be standalone once the pre/post visitor API is introduced. I carefully separated the code so it can be a matter of copy & paste if we want to separate them. (i.e. this will act as an extension of tiny-remapper).

[liach]

For mixin to run under multiple namespaces, it is supposed to just carry the mapping data in the jars. Also this doesnt' solve all bytecode transformation issue as this doesn't handle access wideners.

Imo the remapping mixin thing should go into a separate project.

[Logiquo]

It supposes to be a replacement of AP that can solves

1. can remap even the binary is compiled already. This would ease the process if you want to compile some common code against the different mapping. Currently Mixin AP would require multiple compiles for this (which makes it hard to write Gradle build script).
2. it solves the problem of imaginary classes, whose refmap cannot be generated because it does not exist at compile time.

it is supposed to just carry the mapping data in the jars

Why does it suppose to carry the mapping data in the jars? Can we just remap the binary instead?

Also this doesn't solve all bytecode transformation issues as this doesn't handle access wideners.

The AP also does not handle access wideners. And access widener is handled by loom, why do we want to handle it here?

The idea is that, if I choose to do it in a separate project, then many logical need to be copied from tiny-remapper. If it acts like an extension of tiny-remapper, then many things will be easier.

[liach]

This should be part of the loader. The main logic of tiny remapper is handling bytecodes, while mixin's storage format itself does not touch bytecodes.

Why does it suppose to carry the mapping data in the jars? Can we just remap the binary instead?

This is what mixin is doing with the compile extension right now.

The AP also does not handle access wideners. And access widener is handled by loom, why do we want to handle it here?

I don't see why in one situation we only want to remap mixin but not access wideners.

if I choose to do it in a separate project, then many logical need to be copied from tiny-remapper

No. Mixin compile extensions does remapping, and it doesn't use remapper at all. You probably mean you want to use mapping parser or the mapping io library instead.

[Logiquo]

I don't see why in one situation we only want to remap mixin but not access wideners.

aw is already handled by loom, why do we want to do it again? And also *.accesswinder is a plain text file, what do you want to do with it?

No. Mixin compile extensions do remapping, and it doesn't use remapper at all. You probably mean you want to use mapping parser or the mapping io library instead.

Using tiny-remapper is the easiest place to see how remap mixin will go. We can talk about its appreciated place once this thing works.

This is what mixin is doing with the compile extension right now.

The idea is to completely remove the need for refmap. We just remap the string, shadow, etc all to the correct namespace. In the end, the resulting jar should run without any refmap.

[liach]

aw is already handled by loom

Same, mixin is already handled by loom and has both intermediary/named format, while aw is always intermediary.

Using tiny-remapper is the easiest place to see how remap mixin will go

It is out of the remapper's scope. You can always use any part of the remapper in the loader, where mixin actually exist, than introducing another unneeded and error-prone dependency to the remapper. Also there is no proof for your argument as what you have in the pr is just a few data classes.

The idea is to completely remove the need for refmap. We just remap the string, shadow, etc all to the correct namespace. In the end, the resulting jar should run without any refmap.

It's not possible. For shadows etc, you still need refmap so you know what member should this mixin member change with. This is actually a current shortcoming with mixin refmap, as it only shows the name to remap without referencing the member it actually tracks in one mapping.

[Logiquo]

You can always use any part of the remapper in the loader, where mixin actually exist, than introducing another unneeded and error-prone dependency to the remapper.

I do not want to argue this one with you anymore, since the whole thing is still in a very early stage. And if anything goes for plans, no new dependency should be introduced.

Same, mixin is already handled by loom

The loom uses mixin AP to handle this, which has limitations as I explained above. I don't see any limitation for aw processing.

It's not possible. For shadows etc

It's possible, you just remap the field name based on your knowledge of the current class & target class annotated by @Shadow.

[Player3324]

TR is in a much better position to remap the data than Mixin's annotation processor with MCE, it has the better model, doesn't incur runtime overhead, can modify the data to remap directly and doesn't have all the issues MCE tries to work around. It is in line with the regular remapping purpose of making the Jar behave as if it was written against the target namespace in the first place. PR 62 can provide the isolation necessary to leave TR core untouched beyond activating the feature.

[liach]

Agree that #62 would be the best approach.

[liach]

@LogicFan I wonder if it's feasible to create only 1 custom annotation node class that handles remapping, while the remapping configurations (e.g. what field has what type) goes to a separate class (the updated data will be applied to the annotation node itself).

With only the mixin and shadow annotations, this is currently fine, but mixin has quite a few more annotations, and many of those annotations have string fields of effectively the same type (many are also descriptors, etc.)

[Logiquo]

So there are 3 things that need to be remapped for a mixin.

1. The actual reference to the Minecraft class (this is handled by tiny-remapper).
2. Anything that is a string represents a class/method/field/etc (the mixin ap use refmap for this).
3. Anything that is not a string, but represents a class/method/field (e.g. shadow, soft-implement, etc) (the mixin ap remap them directly).

The only thing needs to be considered here is 2 and 3. I would like to talk about some limitations of the existing mixin AP approach.

1. Because it does not load Minecraft when processing the data, it does not understand the parent-child relationship of the class, thus it cannot properly remap shadow, override, etc, if it comes from the target's parent and you do not properly extend from it.
2. Mixin AP cannot detects lambda or anonymous class without additional annotation.

My solution will be:

1. remap all soft-target when the process is based on the data of tiny-remapper, it can fix the problem of 1 because tiny-remapper actually understands the relationship of Minecraft. It can also fix 2 becasue ASM is more powerful than Java AP.
2. generate a mapping and let another tiny-remapper remap the hard-target.

Notice that the reason for 2 need another pass is that remapping a hard-target could affect things out of the current class instance, and hence cannot be done within this pass.

[Player3324]

Needing another pass doesn't sound right, it'd have to influence the propagation/analysis step instead of we want to go there (not sure it's worth it..)

[Logiquo]

Needing another pass doesn't sound right, it'd have to influence the propagation/analysis step instead of we want to go there (not sure it's worth it..)

Suppose there is a @Shadow public int abc, then if another class reference it, you need to also re-map that one somehow. This cannot be done by the current class propergation.

But, the good news is that it won't affect any propagation/analysis step. Because all of these things are done by another tiny-remapper, with the newly generated mapping.

Say if we want to use it in the loom, then loom will use the first tiny-remapper as before, but with a extra visitor to remap to soft-target and generate mapping for hard-target. Then loom will create another tiny remapper to remap the jar outputted by the first tiny-remapper based on the mapping generated by the previous extra visitor.

named -- tiny-remapper-1 --> obf, but not remap `@Shadow`, `@Override`, etc -- tiny-remapper-2 --> the jar we want.

[liach]

Hmm, if we need another pass, can we just collect the needed hard mappings in the previsit? I don't think that those hard-targets can only be discovered after one round of remap is completed.

[Logiquo]

Hmm, if we need another pass, can we just collect the needed hard mappings in the previsit? I don't think that those hard targets can only be discovered after one round of remapping is completed.

If you are talking about extraAnalyzerVisitor, then.

This can be done and is what has been done by the mixin AP. But the problem is either

1. either I need to duplicate the logic of how tiny-remapper resolves the parent-child relationship; or
2. or it does not recognize the parent-child relationship (similar to what currently mixin AP does).

Thus, I think a second pass is better. Create another tiny-remapper inside loom does not sound too problematic. In the end, it just a chain of remapping.

If you are talking about another pre-visitor, then it propergate only on the class level, not the entire classpath level, so it won't work.

[Logiquo]

Based on my experiment in @Shadow, 2 pass seems works pretty well.

[Logiquo]

I think an extra analysis pass is needed for both incomplete MemberInfo and @Implements.

[Logiquo]

This one is ready for review. (pending for #62 )

[Logiquo]

Since #62 is merged, this one is ready to review. Also, I think this one can be in its own repo similar to https://github.com/FabricMC/fabric-mixin-compile-extensions . The benefit would be to have two independent version numbers, so we can bump one's version without affects the other in the loom build.

[Player3324]

Looks good, thanks!