WrapWithCondition is wrapping injects

[KingContaria]

Hi,

it seems when using wrapwithcondition mixinextras will also wrap any injectors injecting at the statement that is being wrapped if the wrapwithcondition's mixin has a lower priority. I ran into this issue when using 0.2.0, but I've reproduced it with 0.2.1 in this repo. It contains three different mixins into MinecraftClient#setScreen:

• a mixin with default priority = 1000 containing an inject at MinecraftClient#updateWindowTitle
• a mixin with priority = 500 containing a wrapwithcondition targetting MinecraftClient#updateWindowTitle
• a mixin with priority = 1500 containing a wrapwithcondition targetting MinecraftClient#updateWindowTitle

When using mixin export, this is the result:

        if (this.wrapWithCondition$zjg000$modid$wrappingUpdateWindowTitleWithCondition_lowerPriority(this)) {
            this.handler$zjf000$modid$injectingInFrontOfUpdateWindowTitle((CallbackInfo)null);
            if (this.wrapWithCondition$zjh000$modid$wrappingUpdateWindowTitleWithCondition_higherPriority(this)) {
                this.updateWindowTitle();
            }
        }

The wrapwithcondition using a higher priority than the inject only wraps the MinecraftClient#updateWindowTitle call, however the wrapwithcondition using a lower priority wraps both the inject and MinecraftClient#updateWindowTitle.

[LlamaLad7]

This is known and intended. If you don’t want this behaviour, use WrapOperation instead.

[LlamaLad7]

To give more detail it’s not exactly intended but it’s unavoidable. In order for injectors like ModifyArg and Redirect to keep working, the instruction inside the if statement must remain targeted after the WrapWithCondition applies, and as such injects will inject at that instruction and therefore be wrapped. This is resolvable through priority on either mod’s end as you’ve observed.

[LlamaLad7]

On second thought I’ll leave this open as it’s probably worthwhile to make WrapWithCondition a late injector like WrapOperation is.

[KingContaria]

Oh i didnt know this was intended, might be worth atleast mentioning it in the wiki. Personally I kinda assumed this wouldn't happen since the wiki specifically mentions using WrapWithConition in favor of Redirects but Redirects do not have this behaviour.

Also, does this mean that in this situation with 2 mixins with default priority the behaviour depends on mod/mixin loading order? Would it be worth it to just always set the mixins priority to 1500 if it uses WrapWithCondition with no intent of skipping other injects (and if other mods want their injects skipped they can still increase priority on their end) ?

[LlamaLad7]

Also, does this mean that in this situation with 2 mixins with default priority the behaviour depends on mod/mixin loading order?

Yes, but so do many other mixin behaviours.

Would it be worth it to just always set the mixins priority to 1500 if it uses WrapWithCondition with no intent of skipping other injects

I would just use WrapOperation instead if this is causing issues. Long-term I will probably change this behaviour.

[LlamaLad7]

Fixed in 0.3.3

[LlamaLad7]

Some fabric mods were depending on the old behaviour so it's been reverted in floader for now. May as well re-open this while I work something out.

[Earthcomputer]

Fabric mixin checks the fabric loader version the mod was compiled against to decide which local capturing algorithm to use. You could possibly have a similar check

[LlamaLad7]

That’s what i’m planning.

[LlamaLad7]

Fixed with a new version of WrapWithCondition in 0.3.5.