Type parameter constraint in patched class breaks on newer Mono.Cecil version about cecil HOT 7 CLOSED

Closing this in favor of the better-informed #953

from cecil.

Could you extract the offending code in a small dll that we could write a test around? Thanks!

from cecil.

I will look into it, but unfortunately this is quite a complex project and I'm not sure I'll be able to craft a small standalone version that exhibits this bug.

from cecil.

That would however provide the best chances at this being diagnosed and fixed :)

from cecil.

I did manage to use git bisect to determine that the issue is introduced by commit c4cfe16.

from cecil.

Specifically, readding the metadata.RemoveGenericConstraintMapping (generic_parameter); line to MetadataReader.ReadGenericConstraints (and readding that method of course) fixes the issue.

from cecil.

So, I've found something, or at least how to trigger this or make it go away.

Some of it is part of MonoMod though. Also I'm very much at my wits end, but I'll just write down what it is that I found.

So, Everest's installer patches Celeste.exe with MonoMod.

Within MonoMod you at some point end up in MonoMod.MonoModder.PatchRefsInType(TypeDefinition type) where you got this

            foreach (MethodDefinition method in type.Methods)
                PatchRefsInMethod(method);

And then in PatchRefsInMethod(MethodDefinition method) when on the Celeste.UserIO type's method which looks like this inside Celeste

public static bool Save<T>(string path, byte[] data) where T : class
{
[...]
}

if you now simply just call the getter of method.CustomDebugInformations or even just method.HasCustomDebugInformations -- you will now end up with a new Constraint, specifically I've been checking method.GenericParameters[0].Constraints.Count

But also the extra fun thing is if you touch the getter of Constraints on that first generic parameter first, it will also be fine and not get the extra constraint.

The part within MonoModder that triggers it actually looks like this, but you only need to "get" method.CustomDebugInformations, the collection it return is empty on this method, so this loop is irrelevant.

            foreach (CustomDebugInformation dbgInfo in method.CustomDebugInformations)
                if (dbgInfo is AsyncMethodBodyDebugInformation asyncDbgInfo)
                    for (var i = 0; i < asyncDbgInfo.ResumeMethods.Count; i++)
                        asyncDbgInfo.ResumeMethods[i] = ((MethodReference) asyncDbgInfo.ResumeMethods[i].Relink(Relinker, method)).Resolve();

I just tried to see if I could trigger this without MonoMod, by just putting this in place of the MonoModder execution:

                if (module.Name == "Celeste.exe") {

                    var method = module.Types.First(t => t.FullName.Contains("Celeste.UserIO")).Methods.First(m => m.FullName.Contains("Save("));

                    // if I comment this out, I should somehow get "1" on the second log statement, otherwise "0" on both...
                    //logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}");

                    foreach (var cdi in method.CustomDebugInformations)
                        logVerboseCb($"{cdi}");

                    logVerboseCb($"[UserIOLogger] Count = {method.GenericParameters[0].Constraints.Count}");

                }

But it doesn't want to trigger the bug at all. Must be something else that MonoMod is doing.

The place in MonoMod is here
https://github.com/MonoMod/MonoMod/blob/878928553ef775bd7814ae5d208771f0d4ec29be/src/MonoMod.Patcher/MonoModder.cs#L1949

although I was working inside Everest where it subclasses MonoModder and I copied the base implementation of void PatchRefsInMethod(MethodDefinition method) into an override and worked there because that was easier than (re-)building MonoMod all the time. 😅

https://github.com/EverestAPI/Everest/blob/464dea2c34aac90d0cbb4d7c733c88e2f6bf9d68/NETCoreifier/NetFrameworkModder.cs#L13

I suppose I could try chipping away at MonoMod to actually reproduce the bug without MonoMod... anyways this doesn't make any sense :)

PS: What even is Mono.Cecil.Mixin.Read

from cecil.