Merging upstream

[kohanis]

Merging up to v2.3.1.1 of upstream + some fixes

[kohanis]

Okay, there is one thing. core 3.1 sdk does not support file-scoped namespaces even with preview. There are already file-scoped namespaces in master. Should I convert them to block-scoped namespaces or drop 3.1 sdk support?

[kohanis]

Fair, removed it for now. As for 79, I don't see any incompatibilities, but better check later. I was actually planning to do a full merge to 2.3-prerelease.3 later, but I was finding some regressions in my tests with 2.3, so haven't yet I'd prefer to move this to draft for now, as it's something to think about. There's a reason why original harmony doesn't distribute standard versions (or rather now they do, but only ref ones), might have to do the same here. Because of net5+ directives. It will probably need to be split into net5.0, netcoreapp3.1 and net45

[kohanis]

Now it should be okay. Used same method as in harmony. Main targets are now net35;net45;netcoreapp3.0;net5.0, and netstandard2.0 is now ref only

Edit: It might be worth adding mention of "ref only" netstandard2.0 to nuget with the next release

[ManlyMarco]

I merged the previous PR, looks like it created some conflicts. I'll merge this one as well if you can resolve them.

Edit: The PR added net7.0 target but actions seem to be set up for net6.0 max. It should be fine if you pick your net5.0 change instead when merging, assuming it still builds fine.

[kohanis]

I'll wait a bit for discussion on 79

[ManlyMarco]

@kohanis Everything's merged now so it should be fine to revisit this.

[kohanis]

Just noting that I currently have everything synchronized with master of harmony, but I'm waiting for 2.3, which I understand should be out soon https://github.com/pardeike/Harmony/commit/ff82ff4f85bb077c632fae6aa5e8c81b7b03aa7f

[kohanis]

Well, it's basically done, maybe just add github workflows later on

Binary incompatibilities(kind of): HarmonyLib.InlineSignature removed (moved to internal in harmony, not used at all in harmonyx) HarmonyLib.MethodType.Async / HarmonyLib.AccessTools.AsyncMoveNext hidden from net35 build

Comments: ValueTuple shims removed - they are now in MonoMod.Backports __args now unpacks itself back as in harmony netstandard2.0 build is now ref only

// Also, harmony now embeds pdb into the release dll(for thin version, but harmonyx doesn't have fat version anyway). Should it be here?

[ManlyMarco]

On a glance it seems fine, but looks like this will need some testing in different heavily modded games before a merge.

ValueTuple shims removed - they are now in MonoMod.Backports

Is there a chance those shims were used by plugins, which will now break?

Also, harmony now embeds pdb into the release dll(for thin version, but harmonyx doesn't have fat version anyway). Should it be here?

Embedding debug symbols in release is fine, yes.

[kohanis]

Is there a chance those shims were used by plugins, which will now break?

Oh, those were internal, I missed it. So, no

Embedding debug symbols in release is fine, yes.

Well, it's not embedded in 2.3.1 now) Will merge up to 2.3.1.1 soon, only changes for project files, not sources

[kohanis]

Merged up to 2.3.1.1. Also fixed StackTraceFixes(hopefully) But it is now split into 2 packages, HarmonyX and HarmonyX.Ref, alike Lib.Harmony and Lib.Harmony.Ref. And since there is no HarmonyX.Ref yet, you cannot build netstandard2.0 not in a ReleaseRef configuration yet

[kohanis]

@Meivyn you can try this. Here is different approach to GetExecutingAssembly, and one another master fix. Might help. Based on the log, I doubt it, but maybe.

[Meivyn]

That does indeed not fix the issue. I can't tell if your replacement works how it's intended to because I'm not sure to understand what that's even fixing. But it fixes the Unity crash as well.

If you can look at the native patcher while you're here, it's also broken for the same reason (#107).

[kohanis]

I believe last push should fix #107 And @Meivyn, if your reverse patch target is extern(and I think it is, there was a regression in #79), that should help too

Yes, it should be calli and not a generated shenanigans, but there are currently some issues with managed calli with DynamicMethodDefinition. EDIT: actually, calli is not supported by the current transpiler anyway, so NativeHookOriginalProxy will be needed anyway. But calli inside it will still be better. Currently I only use delegate because otherwise mono inline throw before the actual alt entry hook

[Meivyn]

The target method is not an extern method, it's a random method I choose to test the reverse patch:

I also confirm that your latest push fixes the issue with the native methods patches for me.

[Meivyn]

Using a reverse patch on my own method doesn't throw but also doesn't work.

[kohanis]

It looks like inlining. Try calling through a delegate to check, instead of directly

[Meivyn]

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). That didn't solve the exception when patching though.

[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?

[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