System.Reflection.CustomAttributeFormatException thrown on a retrieval of derived attribute with overridden property getter #110945
Conversation
…l of derived attribute with overridden property getter
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
| @@ -1580,7 +1580,7 @@ private static void AddCustomAttributes( | |||
| RuntimeMethodInfo setMethod = property.GetSetMethod(true)!; | |||
|
|
|||
| // Public properties may have non-public setter methods | |||
| if (!setMethod.IsPublic) | |||
| if (setMethod == null || !setMethod.IsPublic) | |||
There was a problem hiding this comment.
is null to avoid unnecessary comparison operator
| @@ -1580,7 +1580,7 @@ private static void AddCustomAttributes( | |||
| RuntimeMethodInfo setMethod = property.GetSetMethod(true)!; | |||
There was a problem hiding this comment.
The nullability suppression should also be removed
src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetCustomAttribute.cs
Outdated
Show resolved
Hide resolved
| Type? baseDeclaredType = declaredType.BaseType; | ||
|
|
||
| while ((m_getterMethod == null || m_setterMethod == null) | ||
| && baseDeclaredType != null && baseDeclaredType is RuntimeType baseDeclaredRuntimeType) |
There was a problem hiding this comment.
| && baseDeclaredType != null && baseDeclaredType is RuntimeType baseDeclaredRuntimeType) | |
| && baseDeclaredType is RuntimeType baseDeclaredRuntimeType) |
The pattern match also does a null check.
| out _, out _, out _, | ||
| out m_getterMethod, out m_setterMethod, out m_otherMethod, | ||
| out isPrivate, out m_bindingFlags); | ||
|
|
||
| // lookup getter/setter when getter and setter are inherited from base class but just a setter/getter is overridden on a sub type | ||
| if ((m_getterMethod != null && m_getterMethod.IsVirtual && m_setterMethod == null) |
There was a problem hiding this comment.
nit: as mentioned earlier, is null and is not null is preferred over == null and != null since it will prevent a call to the == or != operator if that is overridden. In this case, it is just more obvious to understand intent when using RuntimeMethodInfo doesn't overload those operators so it doesn't matter butis.
There was a problem hiding this comment.
RuntimeMethodInfo doesn't overload those operators so it doesn't matter
MethodInfo does overload those operators. MethodInfo operators are going to be selected to compare RuntimeMethodInfo quick test
There was a problem hiding this comment.
Oops thanks; updated comment
| out _, out _, out _, | ||
| out m_getterMethod, out m_setterMethod, out m_otherMethod, | ||
| out isPrivate, out m_bindingFlags); | ||
|
|
||
| // lookup getter/setter when getter and setter are inherited from base class but just a setter/getter is overridden on a sub type |
There was a problem hiding this comment.
nit: start a comment with upper case letter and end with a period.
I can't get around this error when launching the test in mono windows/linux : The used commands are : When activating debug log, I get : I see that there is an open issue about this problem in s390x arch #60550 (not sure if it is the same), I have x64 |
pedrobsaila
requested review from
thaystg,
steveisok and
vitek-karas
as code owners
|
/azp run runtime |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Let's see if we can get fresh test results. Previously it looked like there were some linker failures in the build. |
|
sorry I forgot about this PR, I need to port my changes to nativeaot and fix linker issues. I'll try to resume work on it this week |
|
ready for a new round of reviews |
|
Before this change, public class Base
{
public virtual int P { get; set; }
}
public class Derived : Base
{
public new virtual int P { get => 1; } // NOTE: new
}public class Base
{
public int P { get; set; } // NOTE: not even virtual
}
public class Derived : Base
{
public new virtual int P { get => 1; }
}public class Base
{
public uint P { get; set; } // NOTE: uint
}
public class Derived : Base
{
public new virtual int P { get => 1; }
}After this change they all return non-null. This is changing a lot more than a corner case custom attribute issue. I think there are some unresolved design issues that need to be discussed before going to implementation (provided the implementation requires changing how properties worked for 20 years). The properties in derived classes have been historically considered different things. There's no such thing as an "virtual property". Only the getters/setters are virtual. E.g. how do the proposed changes relate to how this works: // Prints 0 even though we ask for inherited attributes
Console.WriteLine(typeof(Derived).GetProperty("P", typeof(int)).GetCustomAttributes(inherit: true).Length);
public class Base
{
[My]
public virtual int P { get; set; }
}
public class Derived : Base
{
public override int P { get => 1; }
}
[AttributeUsage(AttributeTargets.All, Inherited = true)]
class MyAttribute : Attribute; |
steveharter
added
the
needs-author-action
|
Hi @steveharter. I'm waiting here for thre design problems mentionned by @MichalStrehovsky to be addressed before I can continue working on this issue. I can fix the corner case of using new keyword as it is simple but there are other points beside that. Or do you want me to just fix this point ? |
dotnet-policy-service
bot
removed
needs-author-action
Fixes #110412