Merging upstream

[kohanis]

Merging up to v2.3.1.1 of upstream + some fixes

[CptMoore]

Looks like @kohanis fixes to native detour patcher indeed fixes issue #107, thanks.
Could you also use
<MonoModRuntimeDetour>25.1.0</MonoModRuntimeDetour>
instead of the prelease version, as it includes a vital unity+linux fix.

[kohanis]

That indeed did the trick. Though why is that required now? It was working fine in the previous version of Harmony (and it breaks existing patches).

Frankly I don't see any reason why mono wouldn't inline a purely throwing method without [MethodImpl(MethodImplOptions.NoInlining)].
Is the usage exactly the same? If method with reverse patch call is called at least once before patching itself, this behaviour is not surprising
// But the fact that it is called before patching is, imho, misuse of it

[Meivyn]

Watch out, bumping the MonoMod.RuntimeDetour version requires Harmony to target net452 instead of net45, otherwise it won't use the correct version of Mono.Cecil.

Is the usage exactly the same? If method with reverse patch call is called at least once before patching itself, this behaviour is not surprising

Yes, the usage is the same, as I said it was breaking patches that did work before and it breaks my test one as well, and as you can see that method isn't called before patching. Patching happens in Init which is always called before OnEnable as it's the constructor.

[Plugin(RuntimeOptions.DynamicInit)]
public class Plugin
{
    internal static IPALogger Log { get; private set; } = null!;

    [Init]
    public Plugin(IPALogger logger, PluginMetadata metadata)
    {
        Log = logger;
        Harmony.CreateAndPatchAll(metadata.Assembly, "com.meivyn.BeatSaber.BSPlugin5");
    }

    [OnEnable]
    public void OnEnable()
    {
        Patch.ReversePatch();
    }

    [OnDisable]
    public void OnDisable()
    {
    }

    public void TestMethod()
    {
        Log.Notice(nameof(TestMethod));
    }
}


[HarmonyPatch(typeof(Plugin), nameof(Plugin.TestMethod))]
internal class Patch
{
    [HarmonyReversePatch]
    [MethodImpl(MethodImplOptions.NoInlining)] // breaks without this
    public static void ReversePatch()
    {
        throw new NotImplementedException("Reverse patch has not been executed.");

        IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
        {
            return new CodeMatcher(instructions)
                .MatchStartForward(new CodeMatch(OpCodes.Ldstr))
                .SetOperandAndAdvance("Reverse patched")
                .InstructionEnumeration();
        }
    }
}

[kohanis]

Yes, the usage is the same, as I said it was breaking patches that did work before and it breaks my test one as well, and as you can see that method isn't called before patching. Patching happens in Init which is always called before OnEnable as it's the constructor.

Cannot confirm it. Used latest BSIPA 4.3.2 with your sample without MethodImplOptions.NoInlining on PlateUp, got NotImplementedException

[Meivyn]

Yeah, I actually cannot reproduce the same behavior with my patch as well. It seems to happen only to this patch: https://github.com/Aeroluna/Heck/blob/master/Heck/HarmonyPatches/PlayViewInterrupter.cs#L76, at least from my testing. Using Harmony 2.12.0 broke this patch, and it was not throwing before that. Reverting to Harmony 2.10.2 fixes it.

[kohanis]

Okay, reverse patches are broken in general in 1.11.0+, affirmative. GC collects them. I'll see what I can come up with

[kohanis]

Far from perfect, but let's call it a hotfix for now
// btw, it wasn't previously implemented correctly either, it's just that ILHook didn't have a finalizer previously

[Meivyn]

That doesn't fix the inline issue with my test patch, however it seems to have fixed the broken patch I mentioned earlier. Guess I'm only missing a fix for the IL error that happens when reverse patching some methods before using this PR in BSIPA.

I highly doubt those are the only bugs we are gonna encounter, but I guess it is usable enough. Thanks for your fixes.

[kohanis]

Restructured. Last push in this PR, unless fixes related to it are needed. I'm planning some structural changes of project based on this PR, but as separate one

[kohanis]

Pardon, that fix was important. Also, friendly reminder that without HarmonyX.Ref package project will not be built as is, and CI is also not adapted for this

[kohanis]

As far as I can tell it should work the same way, but with two packages. Not sure about integration with jenkins though

[CptMoore]

whats missing for this to be merged? CI? HarmonyX.Ref?

[ManlyMarco]

Sorry it took me so long to review, most of the changes are just syntax fluff which makes it hard to spot and review any material changes.

As far as I can see this is good to merge and shouldn't break binary backwards compatibility. There may be some minor source code breaks but that's fine.
It might require a major version bump still, but that's for later.

My main worry is workflows and dealing with the ref/full situation. Ideally builds and packages would stay in the same format as they are now, in my opinion at least.

If no one else raises any issues with this PR in the meantime I'll merge this later next week.

[kohanis]

Theoretically I can merge netstandard references into main package, will check.

[kohanis]

Okay, there's a reason for separate package (harmony's discord).
But, after rethinking, I realized that the restriction on netstandard does not apply to harmonyx. Harmony's problem with it is ilmerge.
So, this should work. I plan to later squish this into the first commit
EDIT: it might make sense to make separate target for netcoreapp3.(0/1) tho. To avoid dragging json