System.Reflection.CustomAttributeFormatException thrown on a retrieval of derived attribute with overridden property getter

[pedrobsaila]

Fixes #110412

Sorry for the delay, I've been busy these last weeks

[MichalStrehovsky]

Is this logic equivalent to the others, or just equivalent enough to pass the test? What corner cases are we missing in the test that necessitate the more complex logic elsewhere?

[pedrobsaila]

Sorry, I was going to be off for a few days so I did rapidely just enough to pass the test and not let the PR hanging, I fixed it now. The missing condition stops the lookup in base classes early if someone used the new keyword to hide the property instead of overriding it.

[jeffhandley]

@AaronRobinsonMSFT Can you review this when time permits please?

[MichalStrehovsky]

This implements this for PublishTrimmed; to implement for PublishAot, the new semantic needs to be implemented here too:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/CustomAttributeBasedDependencyAlgorithm.cs

Lines 237 to 278 in 09e6ddd

	private static bool AddDependenciesFromPropertySetter(DependencyList dependencies, NodeFactory factory, TypeDesc attributeType, string propertyName)
	{
	EcmaType attributeTypeDefinition = (EcmaType)attributeType.GetTypeDefinition();
	MetadataReader reader = attributeTypeDefinition.MetadataReader;
	var typeDefinition = reader.GetTypeDefinition(attributeTypeDefinition.Handle);
	foreach (PropertyDefinitionHandle propDefHandle in typeDefinition.GetProperties())
	{
	PropertyDefinition propDef = reader.GetPropertyDefinition(propDefHandle);
	if (reader.StringComparer.Equals(propDef.Name, propertyName))
	{
	PropertyAccessors accessors = propDef.GetAccessors();
	if (!accessors.Setter.IsNil)
	{
	MethodDesc setterMethod = (MethodDesc)attributeTypeDefinition.EcmaModule.GetObject(accessors.Setter);
	if (factory.MetadataManager.IsReflectionBlocked(setterMethod))
	return false;
	// Method on a generic attribute
	if (attributeType != attributeTypeDefinition)
	{
	setterMethod = factory.TypeSystemContext.GetMethodForInstantiatedType(setterMethod, (InstantiatedType)attributeType);
	}
	dependencies.Add(factory.ReflectedMethod(setterMethod.GetCanonMethodTarget(CanonicalFormKind.Specific)), "Custom attribute blob");
	}
	return true;
	}
	}
	// Haven't found it in current type. Check the base type.
	TypeDesc baseType = attributeType.BaseType;
	if (baseType != null)
	return AddDependenciesFromPropertySetter(dependencies, factory, baseType, propertyName);
	// Not found. This is bad metadata that will result in a runtime failure, but we shouldn't fail the compilation.
	return true;
	}

[AaronRobinsonMSFT]

LGTM, thank you!

@MichalStrehovsky Any further feedback?

[MichalStrehovsky]

@MichalStrehovsky Any further feedback?

If we really want this, it looks reasonable. For posterity though, this is implementing some of C# language semantics into the runtime reflection stack (the runtime reflection stack doesn't consider property inheritance in general, like I pointed out in #110945 (comment), or how Type.InvokeMember(BindingFlags.SetProperty) works).

IL is more expressible than what C# allows. C# can sometimes get into IL-level corner cases due to NuGet versioning and compile-time seeing different things than runtime.

What is implemented here will allow things like:

[Derived(Prop = 123)]
class Program;

class BaseAttribute : Attribute
{
    public virtual int Prop { get; set; }
}

class DerivedAttribute : BaseAttribute
{
    // Note `new` keyword
    public new virtual int Prop { get => base.Prop; }
}

to work even though the C# compiler would not accept this.

Hopefully this is sufficiently leafy within the reflection stack that it won't affect many things. I'm not worried about this breaking existing apps (because we're just fixing a throw to not throw). However there's is an aspect that if someone does take advantage of this not throwing, they may run into new issues that we didn't consider.

Like I wrote in #110945 (comment) this needed a bit more design discussion to consider these things before we decide whether we really want to fix this.