Fix HarmonyMethodRefParametersAnalyzer incorrectly marking member accesses as warning

[xiaoxiao921]

Fix incorrect warnings like these

[Alexejhero]

Bump

[ratijas]

You link has expired. FYI "githubusercontent" links are temporary.

I found this issue/PR because I was looking into calls like Method(out var stuff) being erroneously reported by diagnostic Harmony003.

[xiaoxiao921]

I don't have the screenshot anymore, wasn't aware GitHub self hosted files would expire on their own website.

[ratijas]

The problem is you reused a link from elsewhere. Should have pasted the image directly, I guess. Regardless, it wouldn't be usable when reading git history. Moreover, if it fixes a warning, you should just copy the warning text; it doesn't require a png (:

[xiaoxiao921]

Turns out chatgpt can one shot find it by just pasting the url.
This was the image, hosted from discord, which I just copy pasted into a discord bot DM.

[ratijas]

Here is a minimal reproducible example, in a text format, searchable, indexable, does not expire, doesn't have anything to do with any Discord bots.

using HarmonyLib;

class OriginalCode
{
    public void Test() {}
}

[HarmonyPatch(typeof(OriginalCode))]
static class MyPatch
{
    [HarmonyPostfix]
    [HarmonyPatch(nameof(OriginalCode.Test))]
    static void PropertyAccess((int, int) __state)
    {
        var _ = __state.Item1;
    }

    [HarmonyPostfix]
    [HarmonyPatch(nameof(OriginalCode.Test))]
    static void MethodCall(int __state)
    {
        __state.ToString();
    }
}

The code above produces the following diagnostics:

SourceFile.cs(15,17): warning Harmony003: Harmony non-ref patch parameter __state.Item1 modified. This assignment have no effect.
SourceFile.cs(22,9): warning Harmony003: Harmony non-ref patch parameter __state.ToString modified. This assignment have no effect.

So, apparently, accessing (reading) Item1 on a tuple or even calling ToString confuses the analyzer, which is needlessly restrictive for read-only operations (but there is no language support to enforce or tell the compiler that it is truly a read-only operation).

Furthermore, the warning is only issued for value types (like int or tuple), but not for reference types:

using HarmonyLib;

class OriginalCode
{
    public void Test() {}
}

class State
{
    public int data;
}

[HarmonyPatch(typeof(OriginalCode))]
static class MyPatch
{
    [HarmonyPostfix]
    [HarmonyPatch(nameof(OriginalCode.Test))]
    static void CustomClass(State __state)
    {
        // no warning
        var _ = __state.data;
        __state.data = 42;
    }
}

Finally, the warning can be mitigated by copying a value typed state or another argument to a local variable.

    [HarmonyPostfix]
    static void CustomClass((int, int) __state)
    {
        // no warning
        var alias = __state;
        var _ = alias.Item1;
    }

It's worth nothing that the official Harmony docs are clearly biased towards reference type states, so the warning does not affect the official Harmony tutorial on prefix patching.

---

Overall, I think the warning makes sense to prevent silly mistakes, but an argument named __state specifically should be treated differently than the rest.

[xiaoxiao921]

The PR was not even about the parameter being a __state one, it just happened that screenshot was one IIRC. It's fair though that __state = ...; should probably not get flagged in a suffix, as the user might just be using as a (local) variable. That makes me think though that a warning if the user forget to put the ref param for a __state param in a prefix would be nice though! (didn't check if the project has already one or not)

[ratijas]

Yea, I was trying to analyze your solution, and conduct my own investigation as well. It was a fun ride!

Also, you are right: the analyzer isn't flagging a __state without out or ref in prefix, but it should. I tried it, and to my surprise, BepInEx/HarmonyX didn't complain at all, even at runtime, but the value of __state was default-initialized — Send = 0 for NetworkBehaviour.__RpcExecStage enum in particular.