pardeike / Harmony

A library for patching, replacing and decorating .NET and Mono methods during runtime
https://www.patreon.com/pardeike
MIT License
5.29k stars 493 forks source link

Harmony AccessTools.MakeDeepCopy fails when target contains a constant or static readonly field #619

Open xADDBx opened 2 months ago

xADDBx commented 2 months ago

Describe the bug When using AccessTools.MakeDeepCopy on a class which contains a constant field, Harmony will cause a System.FieldAccessException: Cannot set a constant field.

To Reproduce

  1. Create an executable program
  2. Create another class with a constant field in it
  3. In the Main method, create an instance of the class and call MakeDeepCopy on it
  4. System.FieldAccessException: Cannot set a constant field.
    at System.Reflection.MdFieldInfo.SetValue(Object obj, Object value, BindingFlags invokeAttr, Binder binder, CultureInfo culture)
    at HarmonyLib.Traverse.SetValue(Object value)
    at HarmonyLib.AccessTools.<>c__DisplayClass102_0.<MakeDeepCopy>b__0(String name, Traverse src, Traverse dst)
    at HarmonyLib.Traverse.<>c__DisplayClass39_0.<IterateFields>b__0(String f)
    at System.Collections.Generic.List`1.ForEach(Action`1 action)
    at HarmonyLib.Traverse.IterateFields(Object source, Object target, Action`3 action)
    at HarmonyLib.AccessTools.MakeDeepCopy(Object source, Type resultType, Func`4 processor, String pathRoot)
    at HarmonyLib.AccessTools.MakeDeepCopy[T](Object source)
    at Harmonyissue.Program.Main(String[] args) in D:\Programming\Projects\Modding\HarmonyStuff\Harmonyissue\Harmonyissue\Program.cs:line 16

Expected behavior Const fields should be ignored when creating a DeepCopy

Screenshots / Code grafik grafik

Runtime environment (please complete the following information):

xADDBx commented 2 months ago

I just tried testing the different possible combinations of IsLiteral, IsInitOnly and IsStatic. At the same time I noticed that static readonly fields will also cause an exception: grafik

FieldModifiers IsLiteral IsInitOnly IsStatic Causes Exception
False False False No
readonly False True False No
static False False True No
static readonly False True True Yes
const True False True Yes

Updated Test class:

public class Test {
    public const string MyConstField = "Don't set me";
    public string MyField = "Set me";
    public static string MyStaticField = "Set me";
    public readonly string MyReadonlyField = "Set me";
    public static readonly string MyStaticReadonlyField = "Don't set me";
}

MakeDeepCopy will throw for both MyStaticReadonlyFieldand MyConstField. Changing the Traverse.SetValue to the following seemed to work for my test case:

[HarmonyPatch]
public static class Patches {
    [HarmonyPatch(typeof(Traverse), nameof(Traverse.SetValue)), HarmonyPrefix]
    public static bool SetValue(object value, ref Traverse __result, Traverse __instance) {
        var infoField = AccessTools.Field(typeof(Traverse), "_info");
        var methodField = AccessTools.Field(typeof(Traverse), "_method");
        var rootField = AccessTools.Field(typeof(Traverse), "_root");
        var paramsField = AccessTools.Field(typeof(Traverse), "_params");
        MemberInfo _info = infoField.GetValue(__instance) as MemberInfo;
        MethodBase _method = methodField.GetValue(__instance) as MethodBase;
        object[] _params = paramsField.GetValue(__instance) as object[];
        object _root = rootField.GetValue(__instance);

        if (_info is FieldInfo fi) {
            bool isConst = fi.IsLiteral && !fi.IsInitOnly && fi.IsStatic;
            bool isStaticReadonly = !fi.IsLiteral && fi.IsInitOnly && fi.IsStatic;
            if (!isConst && !isStaticReadonly)
                ((FieldInfo)_info).SetValue(_root, value, AccessTools.all, null, CultureInfo.CurrentCulture);
        }
        if (_info is PropertyInfo)
            ((PropertyInfo)_info).SetValue(_root, value, AccessTools.all, null, _params, CultureInfo.CurrentCulture);
        if (_method is not null)
            throw new Exception($"cannot set value of method {_method.FullDescription()}");
        __result = __instance;
        return false;
    }
}

Actually, is there a reason it tries to set static Fields at all? While it works on normal static fields, does the method really need to set static fields which should be shared across instances anyway?

In total I see three different approaches:

  1. Check for static readonly or const as shown above and don't set those fields
  2. Check for any static field and don't set those (depending on where Traverse.SetValue is used, this might not be a good idea)?
  3. try/except System.FieldAccessException around the FieldInfo.SetValue call (this might be too broad in what it catches?)

Edit: To make sure this doesn't only apply to public string, I also tested for private int fields, which showed exactly the same behavior.