dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

JIT throws MissingMethodException compiling dynamic method that invokes `init` property setter declared in generic class #45102

Closed AArnott closed 3 years ago

AArnott commented 3 years ago

Description

Very strangely, when a property setter is defined as init in C# 9, and that property is declared in a generic type, then dynamic generated code throws when calling it.

Repro

#nullable enable

using System;
using System.Reflection;
using System.Reflection.Emit;

var milk = new Product<int> { Name = "milk" };

MethodInfo setter = milk.GetType().GetProperty(nameof(Product<int>.Name))!.SetMethod!;
setter.Invoke(milk, new object?[] { "bread" });
Console.WriteLine(milk);

var assemblyBuilder = AssemblyBuilder.DefineDynamicAssembly(new AssemblyName("test"), AssemblyBuilderAccess.Run);
var moduleBuilder = assemblyBuilder.DefineDynamicModule("test.dll");
var typeBuilder = moduleBuilder.DefineType("SomeClass", TypeAttributes.Public | TypeAttributes.Sealed | TypeAttributes.Abstract, typeof(object));

var dynamicMethod = typeBuilder.DefineMethod("SetName", MethodAttributes.Public | MethodAttributes.Static, null, new Type[] { typeof(Product<int>), typeof(string) });
var il = dynamicMethod.GetILGenerator();
il.Emit(OpCodes.Ldarg_0);
il.Emit(OpCodes.Ldarg_1);
il.Emit(OpCodes.Call, setter);
il.Emit(OpCodes.Ret);

Type builtType = typeBuilder.CreateType()!;
MethodInfo method = builtType.GetMethod("SetName", BindingFlags.Public | BindingFlags.Static)!;

var del = method.CreateDelegate<Action<Product<int>, string>>();
del(milk, "toast");
Console.WriteLine(milk);

public class Product<T> {
    public string Name { get; init; }
}

Configuration

.NET 5.0. Repros on .NET Framework 4.8 as well. x64

Regression?

Not a regression, since the repro depends on init property setters which was only introduced in .NET 5.0.

Other information

See https://github.com/neuecc/MessagePack-CSharp/issues/1134 which tracks this within the MessagePack repo.

Why does .NET 5.0 throw at runtime as if the property setter was not defined? And why only when the class is generic?

If I change from init to set (or from generic class to non-generic), the problem goes away, even though the emitted IL is identical. The init setter and set setter are nearly identical too. There's just one small bit of metadata on an init setter that's different (that messagepack doesn't care about):

+instance void modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) set_Name (
-instance void set_Name (

The problem does not repro when using DynamicMethod. It only fails when using MethodBuilder.

Dotnet-GitSync-Bot commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

AArnott commented 3 years ago

Based on this doc it seems likely that our IL code gen has to be updated to include modreq([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) in the called method signature somehow. It seems all C# compiler generated calls to init methods require this, and it's evident in decompilers but not in our code-generated calls to it.

As to why it only fails in generic classes may be a bug/detail of the .NET 5 runtime.

Is this actually a bug in reflection or ILGenerator? Should it be including the modreq return type modifier in the call IL instruction but is leaving it out?

AArnott commented 3 years ago

Confirmed: when the class is not generic, ILGenerator produces this:

image

But when the class is generic, ILGenerator only produces this:

image

So the modreq is missing in the generic class case.

wzchua commented 3 years ago

This is related to #25958.

40587 fixed this and #44452 fix a bug I had in the fix

AArnott commented 3 years ago

@steveharter advocated for #40587 to be merged into 6.0 due to risk, it seems. Now that it's there, can we port to 5.0? While the bug has been around for a while, the C# 9 init property accessor and positional records features make hitting this much more likely and is breaking not just mocking frameworks but deserializers.

AArnott commented 3 years ago

@wzchua does [this](https://github.com/dotnet/runtime/issues/25958#issuecomment-543403476] describe a workaround that I could possibly implement to fix-up the generated IL so I can run on current versions of runtimes? Does this bug happen on .NET Framework as well?

AArnott commented 3 years ago

Ya, .NET Framework has this bug as well.

I tried using SignatureHelper.GetMethodSigHelper to workaround the problem by building my own signature, but the public APIs on that type are woefully deficient it seems. There's no way to set required modifiers on the return type, and even adding arguments throws NRE when calling it in what seems like a reasonable way. But I'm guessing because I can't find any good documentation for this class or sample uses of it outside of dotnet/runtime where everything used is internal.

wzchua commented 3 years ago

@AArnott no. That describes why the previous attempt at fixing the issue broke something.

I thought SignatureHelper would work but it can't. There's a step where the class token is operated with the method signature to generate some token. It calls into the unmanaged part of the runtime.

But I think setting the backing field directly should work but it's not the same as calling the setter.

AArnott commented 3 years ago

I think setting the backing field directly should work but it's not the same as calling the setter.

That would require some way to associate the backing field with the property setter, assuming there even is a backing field that setting would be equivalent.

We should also consider adding API to SignatureHelper and ILGenerator to allow folks to do this sort of thing themselves, IMO. In the meantime, since DynamicMethod works (albeit it takes a slower path in MessagePack to do so) I'll see what I can do to automatically "fail over" to that path when init property setters are encountered.

wzchua commented 3 years ago

This is probably super fragile but I extracted the code path a generic class + non-generic methodInfo takes for token generation https://gist.github.com/wzchua/a72cfea67ad514234364c10da92fe08f

steveharter commented 3 years ago

@AArnott does this still repro in 6.0? If not, I assume you want #40587 and #44452 ported to 5.0?

Changed this to API suggestion for now, since SignatureHelper should have a way to specify the modifiers.

AArnott commented 3 years ago

Thanks for the workaround, @wzchua. Ya, reflecting into internals sounds quite fragile, as you say.

@wzchua : Is there a (safer?) way to simply detect if your fix is present in a given runtime? Right now I'm changing MessagePack to not even try to support the scenario since it's broken, but I'd like it to "light up" properly when running on a fixed runtime (whether that's 6.0 or 5.0 after it's fixed).

@steveharter : it turns out at least for VS's own purposes, we use messagepack settings that lead to it using DynamicMethod internally so this isn't impacting VS. And VS isn't using .NET 5.0 anyway, so no, I won't push hard to port the fix to .NET 5.0, although I would support it since I expect it will impact other messagepack customers before .NET 6 becomes mainstream.

wzchua commented 3 years ago

@AArnott, there is really no other way other than generating a method, invoking it and catching the exception.

steveharter commented 3 years ago

Closing per feedback:

this isn't impacting VS. And VS isn't using .NET 5.0 anyway, so no, I won't push hard to port the fix to .NET 5.0,