Add missing compiler-generated attributes to corelib

[stephentoub]

Fixes #85612

Cherry-picked @ericstj's commit for adding these and then tweaked it a bit (e.g. added docs and tests).

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #85612

Cherry-picked @ericstj's commit for adding these and then tweaked it a bit (e.g. added docs and tests).

Author:	stephentoub
Assignees:	-
Labels:	area-System.Runtime.CompilerServices, new-api-needs-documentation
Milestone:	-

[ericstj]

Interesting - we had one test that expected to find NullableContextAttribute defined in System.Text.Json.dll:

runtime/src/libraries/System.Text.Json/tests/Common/UnsupportedTypesTests.cs

Lines 173 to 182 in 9fc5fcb

	public async Task TypeWithNullConstructorParameterName_ThrowsNotSupportedException()
	{
	// Regression test for https://github.com/dotnet/runtime/issues/58690
	Type type = Assembly.GetExecutingAssembly().GetType("System.Runtime.CompilerServices.NullableContextAttribute")!;
	ConstructorInfo ctorInfo = type.GetConstructor(new Type[] { typeof(byte) });
	Assert.True(string.IsNullOrEmpty(ctorInfo.GetParameters()[0].Name));
	object value = ctorInfo.Invoke(new object[] { (byte)0 });
	await Assert.ThrowsAnyAsync<NotSupportedException>(() => Serializer.SerializeWrapper(value));
	await Assert.ThrowsAnyAsync<NotSupportedException>(() => Serializer.DeserializeWrapper("{}", type));

Perhaps that test should fallback to looking in the core assembly for NullableContextAttribute.

type ??= typeof(object).Assembly.GetType("System.Runtime.CompilerServices.NullableContextAttribute")!;

cc @eiriktsarpalis

[eiriktsarpalis]

@ericstj Either that or conditioning the Assembly on the tested TFM.

[333fred]

I do want to hold off on this until we have a bit of time investigate on the roslyn-side about the correct ordering we should look for these attributes and making sure that we're not about to introduce a large metadata-breaking change as libraries update to .NET 8. I should have some time for that next week after we get collection literals merged. I don't expect to encounter unsolvable issues, but measure twice cut once.

[ericstj]

@ericstj Either that or conditioning the Assembly on the tested TFM.

I wasn't sure if that test was actually specific to that type, or you just happened to choose it because it had the characteristics you needed to serve as a repro. From your comment in the issue:

Root cause appears to be IL #73188 (comment) containing parameters with stripped names.

So maybe we could add some test type that is guaranteed to have stripped parameter names. I actually don't understand why the JsonSerializer would ever try to serialize an attribute, unless the user passed in a value of that attribute to serialize which seems really unusual.

[stephentoub]

I pushed a commit to disable the json test on netcoreapp.

There are other failures on mono in release, all related to moq / castle, I'm guessing something to do with trimming.

[eiriktsarpalis]

So maybe we could add some test type that is guaranteed to have stripped parameter names. I actually don't understand why the JsonSerializer would ever try to serialize an attribute, unless the user passed in a value of that attribute to serialize which seems really unusual.

The original customer reported issue was trying to serialize that attribute. I tried reproducing the problem with a test type but couldn't. IIRC it had something to do with how the compiler emitted IL for the particular compiler generated attribute.

[333fred]

I do want to hold off on this until we have a bit of time investigate on the roslyn-side about the correct ordering we should look for these attributes and making sure that we're not about to introduce a large metadata-breaking change as libraries update to .NET 8. I should have some time for that next week after we get collection literals merged. I don't expect to encounter unsolvable issues, but measure twice cut once.

I put my investigation notes into dotnet/roslyn#68697 (comment), but the TLDR is that our past selves were smart and thought about this, and we are good to proceed.

[stephentoub]

@lewing, suggestions on how to track down why this is failing? Seems likely it's related to trimming, given the mono legs that are failing.

[ericstj]

@stephentoub it looks like these failures are caused the bug fixed in castleproject/Core@979eb75. That was previously assuming all arrays in CustomAttributeTypedArgument were IList<CustomAttributeTypedArgument> representing the list of arguments - that's wrong. The first parameter to an attribute might be an array itself, as byte[] is for NullableAttribute. I think we just need to update Castle.Core to get that fix. Looks like it's in the latest public build 5.1.1, which happens to be referenced by the latest build of Moq - 4.18.4.

[ericstj]

Still failing, but most were fixed by picking up the new version of Moq. Everything remaining is failing at this:
https://github.com/castleproject/Core/blob/dca4ed09df545dd7512c82778127219795668d30/src/Castle.Core/DynamicProxy/Generators/MethodWithInvocationGenerator.cs#L78C22-L78C32

var constructor = invocation.GetConstructors()[0];

Where invocation is a System.Type that seems to be generated. Seeing the same callstack across multiple different tests, all trying to create mocks for abstract types or interfaces. It's not clear to me why this is failing only on browser-wasm - perhaps some difference with Reflection / Emit behavior here? @lewing any suggestions for how to follow up on this?

[lewing]

Is it just a case of the constructors being linked out then an attempt to access them via reflection? AOT and EAT lanes trim the tests

cc @lambdageek @radical

[ericstj]

Is it just a case of the constructors being linked out then an attempt to access them via reflection

I'm not certain - looking through this code it seems to me like these types are generated at runtime with ref.emit - so the linker shouldn't be involved. I'm trying to debug what Moq is doing in the non-browser case to try and answer the question about "what types are these".

[ericstj]

Ok, so at least on windows I see these types being used:

{Name = "InterfaceMethodWithoutTargetInvocation" FullName = "Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation"}

That type does have a constructor, when not trimmed: https://github.com/castleproject/Core/blob/dca4ed09df545dd7512c82778127219795668d30/src/Castle.Core/DynamicProxy/Internal/InterfaceMethodWithoutTargetInvocation.cs#L28

So I downloaded the helix payload for the failing test and examined the IL, sure enough that constructor is missing:

.class public auto ansi sealed beforefieldinit Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation
	extends Castle.DynamicProxy.AbstractInvocation
{
	.custom instance void [System.Private.CoreLib]System.ComponentModel.EditorBrowsableAttribute::.ctor(valuetype [System.Private.CoreLib]System.ComponentModel.EditorBrowsableState) = (
		01 00 01 00 00 00 00 00
	)
	// Methods
	.method public hidebysig specialname virtual 
		instance object get_InvocationTarget () cil managed 
	{
		// Method begins at RVA 0x5059
		// Header size: 1
		// Code size: 2 (0x2)
		.maxstack 8

		IL_0000: ldnull
		IL_0001: ret
	} // end of method InterfaceMethodWithoutTargetInvocation::get_InvocationTarget

	.method public hidebysig specialname virtual 
		instance class [System.Private.CoreLib]System.Reflection.MethodInfo get_MethodInvocationTarget () cil managed 
	{
		// Method begins at RVA 0x5059
		// Header size: 1
		// Code size: 2 (0x2)
		.maxstack 8

		IL_0000: ldnull
		IL_0001: ret
	} // end of method InterfaceMethodWithoutTargetInvocation::get_MethodInvocationTarget

	.method public hidebysig specialname virtual 
		instance class [System.Private.CoreLib]System.Type get_TargetType () cil managed 
	{
		// Method begins at RVA 0x5059
		// Header size: 1
		// Code size: 2 (0x2)
		.maxstack 8

		IL_0000: ldnull
		IL_0001: ret
	} // end of method InterfaceMethodWithoutTargetInvocation::get_TargetType

	.method family hidebysig virtual 
		instance void InvokeMethodOnTarget () cil managed noinlining 
	{
		// Method begins at RVA 0x504d
		// Header size: 1
		// Code size: 11 (0xb)
		.maxstack 8

		IL_0000: ldstr "Linked away"
		IL_0005: newobj instance void [System.Private.CoreLib]System.NotSupportedException::.ctor(string)
		IL_000a: throw
	} // end of method InterfaceMethodWithoutTargetInvocation::InvokeMethodOnTarget

	// Properties
	.property instance object InvocationTarget()
	{
		.get instance object Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation::get_InvocationTarget()
	}
	.property instance class [System.Private.CoreLib]System.Reflection.MethodInfo MethodInvocationTarget()
	{
		.get instance class [System.Private.CoreLib]System.Reflection.MethodInfo Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation::get_MethodInvocationTarget()
	}
	.property instance class [System.Private.CoreLib]System.Type TargetType()
	{
		.get instance class [System.Private.CoreLib]System.Type Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation::get_TargetType()
	}

} // end of class Castle.DynamicProxy.Internal.InterfaceMethodWithoutTargetInvocation

That explains the error, but doesn't really explain why this PR would cause that. I see the same test passing in other PRs, so I'm downloading the helix payload from those to see if they differ (retain the constructor).

[ericstj]

I found this --

runtime/eng/testing/ILLinkDescriptors/ILLink.Descriptors.Castle.xml

Lines 2 to 6 in a2c19cd

	<assembly fullname="Castle.Core">
	<namespace fullname="Castle.DynamicProxy" />
	<type fullname="Castle.DynamicProxy.Internal.CompositionInvocation" />
	<type fullname="Castle.DynamicProxy.Internal.InheritanceInvocation" />
	</assembly>

Which looks like it's preserving metadata on similar types... I wonder if something about the update I made to Moq / Castle.Core caused it to start hitting this codepath that it did not before and we just need to root more types for the linker 🤔.

[ericstj]

I see -- indeed that is the problem. This change started using the type in Castle.Core instead of emitting a type: castleproject/Core@0c8f9d0#diff-4ba914dc0e136772f890d9ffaf385d702f91df37c92f648ff06bb31a39def6c1

That change came down as part of the update to Moq so I'll need to include that type in the ILLink.Descriptors.Castle.xml.

[ericstj]

Ok - I learned how to get a repro in a plain-old console app. dotnet new console add reference to Moq, use it, and set PublishTrimmed to true. This lets me debug and work through the issues using a proper debugger. 👍 The latest changes got me through the callstack that was failing -- now to see if things get further in browser...

[ericstj]

All remaining issues in PR validation are known failures.