Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

madelson · 2022-02-12T01:37:48Z

ghost · 2022-02-12T01:37:55Z

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

Issue Details

Fix #64335

Author:	madelson
Assignees:	-
Labels:	`area-System.Reflection`
Milestone:	-

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs

Previously, this test asserted that GetCustomAttributes on an open generic type would return null. Now we return empty instead.

src/libraries/System.Runtime/tests/System/Attributes.cs

jkotas · 2022-02-15T13:36:33Z

Could you please also fix it for NativeAOT here: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs#L149

madelson · 2022-02-16T02:06:12Z

Could you please also fix it for NativeAOT here: https://github.com/dotnet/runtime/blob/main/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/Attribute.CoreRT.cs#L149

@jkotas is your proposal to return Array.Empty<Attribute>() in that case instead of null? Do we still want to use the exception-based control flow?

Also, do you know if there is test coverage that should be updated for this?

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

jkotas · 2022-02-16T02:27:41Z

is your proposal to return Array.Empty() in that case instead of null?

I think it needs to return Attribute array of the right size.

Do we still want to use the exception-based control flow?

It would be nice to use non-exception based control flow, same as CoreCLR.

Also, do you know if there is test coverage that should be updated for this?

We are working to get the existing tests enabled for NativeAOT. No need to add special test coverage.

Enable open generic attribute type requests for parameters, break out larger tests into separate cases.

jkotas · 2022-02-16T17:32:37Z

src/libraries/System.Runtime/tests/System/Attributes.cs

+        public static void GetCustomAttributesBehaviorMemberInfoReturnsNull()
+        {
+            FieldInfo field = new CustomFieldInfo(null!);
+            if (PlatformDetection.IsMonoRuntime)


Should we fix Mono to return null in this corner case as well to avoid this condition?

@jkotas I'll defer to your decision but I'm not sure it makes sense. Looking through the code it seems like null vs throw in both CoreCLR and Mono are going to be inconsistent depending on the member type and other factors (e. g. whether inherit is true or false). It would take quite a bit more test coverage and probably quite a few changes to get to a consistent state across all the different call flows.

Another option would just be to stop testing this case, since it is so niche so perhaps undefined behavior is fine.

Thoughts?

Another option would just be to stop testing this case, since it is so niche so perhaps undefined behavior is fine.

I agree. We either care about this corner case and the behavior should be consistent across runtimes; or we do not care about this corner case and we should not be testing it at all.

jkotas · 2022-02-16T17:51:58Z

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs

-            return (object[])Array.CreateInstance(elementType, elementCount);
+            return (object[])Array.CreateInstance(caType, elementCount);
+
+            T[] CreateArray<T>() => elementCount == 0 ? Array.Empty<T>() : new T[elementCount];


(Non-static) local methods with captured arguments are inefficient. The compiler will create display type to pass the captured locals around that comes with a lot of ceremony.

I think I would inline it. The local method is not worth it. Also, I think it may be worth it to check for Attribute and use the faster non-reflection based path for it. Something like:

bool useAttributeArray = false; bool useObjectArray = false; if (caType == typeof(Attribute)) { useAttributeArray = true; } else if (caType.IsValueType) { useObjectArray = true; } else if (caType.ContainsGenericParameters) { if (caType.IsSubclassOf(typeof(Attribute))) { useAttributeArray = true; } else { useObjectArray = true; } } if (useAttributeArray) (elementCount != 0) ? new Attribute[elementCount] : Array.Empty<Attribute>(); if (useObjectArray) (elementCount != 0) ? new object[elementCount] : Array.Empty<object>(); return (elementCount != 0) ? (object[])Array.CreateInstance(caType, elementCount) : caType.GetEmptyArray();

jkotas

LGTM otherwise. Thank you!

jkotas · 2022-02-16T17:54:40Z

Looks like Mono is crashing on the newly added tests even though they are disabled:

#13 0x0000007f8d2cfbdc in mono_assertion_message_unreachable (file=0x64 <error: Cannot access memory at address 0x64>, line=6) at /__w/1/s/src/mono/mono/eglib/goutput.c:234
#14 0x0000007f8d115120 in custom_attr_class_name_from_method_token (image=0x5589355e20, method_token=167772165, nspace=0x7fc25c2d58, class_name=0x7fc25c2d50, assembly_token=<optimized out>) at /__w/1/s/src/mono/mono/metadata/custom-attrs.c:2441

madelson · 2022-02-18T20:48:06Z

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeParameterInfo.cs

            if (attributeType.UnderlyingSystemType is not RuntimeType attributeRuntimeType)
                throw new ArgumentException(SR.Arg_MustBeType, nameof(attributeType));

+            if (MdToken.IsNullToken(m_tkParamDef))
+                return CustomAttribute.CreateAttributeArrayHelper(attributeRuntimeType, 0);


This change was required to prevent a failure in Microsoft.CSharp.RuntimeBinder.Tests.AccessTests.PublicMethod().

I feel a bit weird exposing the CreateAttributeArrayHelper method as internal, but this is really the exact logic we want here. It makes this branch consistent with other GetCustomAttribute methods which all allow the consumer to cast the array to the requested attribute type.

Moving this after the argument check presumably has some slight perf cost but ultimately feels more correct (and unless we change CreateAttributeArrayHelper to take Type instead of RuntimeType we need the check to happen first regardless).

Ok. Do we need a test to directly verify the behavior of ParamerInfo.GetCustomAttributes for this case?

madelson · 2022-02-18T22:08:39Z

Looks like Mono is crashing on the newly added tests even though they are disabled:

@jkotas @buyaa-n From some experimentation, it seems that this is caused by adding the generic assembly attribute. Any thoughts on how best to address this? I tried putting the assembly/module generic attributes behind #if !MONO but that did not seem to work. Another option would be to move the module/attribute tests to https://github.com/dotnet/runtime/pull/65237/files#diff-d55f466b22e54fbf3b96b94f21d22c390ae92ff4047896b0e87c6af28e4dcc6f which I believe is CoreCLR-only.

jkotas · 2022-02-19T07:04:24Z

I do not think that it makes sense to keep pilling workarounds for Mono bugs.

@SamMonoRT @lambdageek Could you please take a look and suggest fix for the Mono crash that the CI is hitting?

…for assembly and module into CoreCLR-only tests.

madelson · 2022-02-26T18:44:07Z

@jkotas @buyaa-n I'm a bit lost interpreting the current crop of errors. Some of them seem to be in cpp files while others are canceled/timed out. Any advice on where to look?

jkotas · 2022-02-26T19:47:35Z

I see some infrastructure related failures (the logs that talk about deadlettered queues). I also see number of failures related to the change in the PR:

[23:03:07] info: Expected: typeof(System.InvalidCastException)
[23:03:07] info: Actual:   typeof(System.ArrayTypeMismatchException): Source array type cannot be assigned to destination array type.
[23:03:07] info: ---- System.ArrayTypeMismatchException : Source array type cannot be assigned to destination array type.
[23:03:07] info:    at System.Array.CopySlow(Array , Int32 , Array , Int32 , Int32 , Boolean )
[23:03:07] info:    at System.Tests.AttributeGetCustomAttributes.<>c__DisplayClass13_0.<GetCustomAttributesBehaviorMemberInfoReturnsStringArray>b__0()
[23:03:07] info: ----- Inner Stack Trace -----
[23:03:07] info:    at System.Array.CopySlow(Array , Int32 , Array , Int32 , Int32 , Boolean )
[23:03:07] info:    at System.Tests.AttributeGetCustomAttributes.<>c__DisplayClass13_0.<GetCustomAttributesBehaviorMemberInfoReturnsStringArray>b__0()
[23:03:14] info: Finished:    System.Runtime.Tests.dll

or

    reflection/GenericAttribute/GenericAttributeTests/GenericAttributeTests.sh [FAIL]
      Unhandled exception. System.Exception: Error in line: 161
         at Program.AssertAny(IEnumerable`1 source, Func`2 condition, Int32 count, Int32 line)
         at Program.Main(String[] args)

drieseng · 2022-02-27T07:45:13Z

src/libraries/System.Runtime/tests/System/Attributes.cs

@@ -283,27 +283,6 @@ private static void GenericAttributesTestHelper<TGenericParameter>(Func<Type, At
            Assert.Equal(1, closedGenericAttributes.Length);
            Assert.Equal(typeof(GenericAttribute<TGenericParameter>[]), closedGenericAttributes.GetType());
        }
-
-        [Fact]


Is this just a test, or are you actually removing tests because they fail on Mono?

Based on the discussion here it seemed like these weren't worth it given the failures on Mono. They're all tests I added in earlier commits, not existing tests.

madelson · 2022-02-28T02:15:15Z

Many of the remaining errors from the checks are like this one

Example trace:

Fatal error. Internal CLR error. (0x80131506)
   at Tests.System.Net.MultiArrayBufferTests.AddSeveralBytesRepeatedlyAndConsumeSeveralBytesRepeatedly_UsingSliceWithLength_Success()
   at System.RuntimeMethodHandle.InvokeMethod(System.Object, System.Span`1<System.Object> ByRef, System.Signature, Boolean, Boolean)
   at System.Reflection.RuntimeMethodInfo.Invoke(System.Object, System.Reflection.BindingFlags, System.Reflection.Binder, System.Object[], System.Globalization.CultureInfo)
   at System.Reflection.MethodBase.Invoke(System.Object, System.Object[])

Any thoughts on where to start looking?

jkotas · 2022-02-28T02:24:14Z

If the failure is on Windows 7 x86, it is most likely #65890 (ie not related to your changes).

buyaa-n · 2022-03-01T23:31:26Z

I do not think that it makes sense to keep pilling workarounds for Mono bugs.

@SamMonoRT @lambdageek Could you please take a look and suggest fix for the Mono crash that the CI is hitting?

It seems this is the only blocker for this PR, @jkotas should we open an issue for this and unblock the PR? Seems @madelson commented out the tests that causing mono failure:

runtime/src/tests/reflection/GenericAttribute/GenericAttributeTests.cs

Lines 25 to 30 in 86160e1

    
           // Module module = programTypeInfo.Module; 
        
           // AssertAny(CustomAttributeExtensions.GetCustomAttributes(module), a => a is SingleAttribute<long>); 
        
           // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<long>)).GetEnumerator().MoveNext()); 
        
           // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<long>)).GetEnumerator().MoveNext()); 
        
           // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext()); 
        
           // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext());

jkotas · 2022-03-01T23:35:48Z

this is the only blocker for this PR, @jkotas should we open an issue for this and unblock the PR?

Fine with me.

buyaa-n · 2022-03-01T23:36:54Z

src/tests/reflection/GenericAttribute/GenericAttributeTests.cs

+        // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<long>)).GetEnumerator().MoveNext());
+        // Assert(CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<long>)).GetEnumerator().MoveNext());
+        // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext());
+        // Assert(!CustomAttributeExtensions.GetCustomAttributes(module, typeof(SingleAttribute<>)).GetEnumerator().MoveNext());	


Should we include these tests within the above mulitilne comment? Seems related to dotnet/msbuild#6734

CC @davidwrighton

I wasn't sure if it was related and wanted to ask about that. It's possible that these tests could be brought back.

Do you know what the preferred approach is for iterating on this particular test locally? [https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/coreclr/testing.md#build-individual-test](this page) suggests that I should run

dotnet.cmd build c:\dev\runtime\src\tests\reflection\GenericAttribute\GenericAttributeMetadata.ilproj -c Release

but that just builds a dll and doesn't seem to actually invoke this main method.

Sorry idk how to run them locally, @davidwrighton @jkotas might help with that, probably it is not ready to be brought back

The instructions for running these tests are at https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/coreclr/windows-test-instructions.md

buyaa-n · 2022-03-01T23:43:28Z

this is the only blocker for this PR, @jkotas should we open an issue for this and unblock the PR?

Fine with me.

Thanks, looks there is an open issue for enabling similar tests #56880

buyaa-n

Thanks for working on this @madelson!

buyaa-n · 2022-03-04T04:19:33Z

An issue created for mono failure, CI failure looks related to this issue and unrelated to this change => merging.

… generic type (#65237)" This reverts commit 2029495.

… generic type (#65237)" (#66508) This reverts commit 2029495.

…for open generic type (dotnet#65237)" (dotnet#66508)" This reverts commit f99ba2e.

… type * Revert "Revert "Avoid Attribute.GetCustomAttributes() returning null for open generic type (#65237)" (#66508)" This reverts commit f99ba2e. * Make DynamicMethod.GetCustomAttributes() return well-typed arrays. This change makes DynamicMethod.GetCustomAttributes() compatible with Attribute.GetCustomAttributes().

… type * Revert "Revert "Avoid Attribute.GetCustomAttributes() returning null for open generic type (dotnet#65237)" (dotnet#66508)" This reverts commit f99ba2e. * Make DynamicMethod.GetCustomAttributes() return well-typed arrays. This change makes DynamicMethod.GetCustomAttributes() compatible with Attribute.GetCustomAttributes().

Avoid Attribute.GetCustomAttributes() returning null for open generic…

79c3416

… type. Fix dotnet#64335

dotnet-issue-labeler bot added the area-System.Reflection label Feb 12, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 12, 2022

madelson commented Feb 12, 2022

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs Outdated Show resolved Hide resolved

madelson commented Feb 12, 2022

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs Show resolved Hide resolved

madelson added 2 commits February 12, 2022 15:56

Fix GenericAttributeTests to reflect dotnet#64335

f0f1e6c

Previously, this test asserted that GetCustomAttributes on an open generic type would return null. Now we return empty instead.

Skip new attribute tests on Mono.

fc0f401

buyaa-n reviewed Feb 14, 2022

View reviewed changes

src/libraries/System.Runtime/tests/System/Attributes.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Feb 14, 2022

View reviewed changes

src/libraries/System.Runtime/tests/System/Attributes.cs Outdated Show resolved Hide resolved

buyaa-n reviewed Feb 15, 2022

View reviewed changes

src/libraries/System.Runtime/tests/System/Attributes.cs Outdated Show resolved Hide resolved

buyaa-n requested review from davidwrighton and steveharter February 15, 2022 04:20

jkotas reviewed Feb 16, 2022

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs Outdated Show resolved Hide resolved

Address feedback from dotnet#65237

224c985

madelson requested a review from MichalStrehovsky as a code owner February 16, 2022 12:08

Fixes for dotnet#65237

a1717a9

Enable open generic attribute type requests for parameters, break out larger tests into separate cases.

jkotas reviewed Feb 16, 2022

View reviewed changes

jkotas approved these changes Feb 16, 2022

View reviewed changes

Additional follow-up from dotnet#65237

3631f5a

madelson commented Feb 18, 2022

View reviewed changes

Further test cleanup for dotnet#65237

e62566d

madelson added 3 commits February 24, 2022 07:08

Attempt to fix Mono test crashes by moving generic attribute testing …

e0aef33

…for assembly and module into CoreCLR-only tests.

Fix GenericAttributeTests.cs compilation

a3ecfe1

Comment out generic attribute assembly tests

5179891

Trim more tests that fail on Mono

86160e1

drieseng reviewed Feb 27, 2022

View reviewed changes

runfoapp bot mentioned this pull request Feb 28, 2022

System.IO.Tests work item failing with SIGKILL #65791

Closed

retrigger checks

0bfa71c

buyaa-n reviewed Mar 1, 2022

View reviewed changes

buyaa-n approved these changes Mar 1, 2022

View reviewed changes

buyaa-n mentioned this pull request Mar 3, 2022

Mono is crashing on the newly added tests for generic Attributes #66168

Open

buyaa-n merged commit 2029495 into dotnet:main Mar 4, 2022

SteveSandersonMS mentioned this pull request Mar 11, 2022

Recent change to Attribute.GetCustomAttributes is breaking in ASP.NET Core #66496

Closed

jkotas added a commit that referenced this pull request Mar 11, 2022

Revert "Avoid Attribute.GetCustomAttributes() returning null for open…

8dea545

… generic type (#65237)" This reverts commit 2029495.

jkotas mentioned this pull request Mar 11, 2022

Revert "Avoid Attribute.GetCustomAttributes() returning null for open generic type" #66508

Merged

stephentoub pushed a commit that referenced this pull request Mar 11, 2022

Revert "Avoid Attribute.GetCustomAttributes() returning null for open…

f99ba2e

… generic type (#65237)" (#66508) This reverts commit 2029495.

stephentoub mentioned this pull request Mar 11, 2022

Generic attributes: Attribute.GetCustomAttributes returns null when passed an unbound generic type #64335

Closed

madelson added a commit to madelson/runtime that referenced this pull request Mar 13, 2022

Revert "Revert "Avoid Attribute.GetCustomAttributes() returning null …

a3d333a

…for open generic type (dotnet#65237)" (dotnet#66508)" This reverts commit f99ba2e.

ghost locked as resolved and limited conversation to collaborators Apr 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

madelson commented Feb 12, 2022

ghost commented Feb 12, 2022

jkotas commented Feb 15, 2022

madelson commented Feb 16, 2022 •

edited

Loading

jkotas commented Feb 16, 2022

jkotas Feb 16, 2022

madelson Feb 18, 2022

jkotas Feb 19, 2022

jkotas Feb 16, 2022 •

edited

Loading

jkotas left a comment

jkotas commented Feb 16, 2022

madelson Feb 18, 2022

jkotas Feb 19, 2022

madelson Feb 19, 2022

madelson commented Feb 18, 2022

jkotas commented Feb 19, 2022

madelson commented Feb 26, 2022

jkotas commented Feb 26, 2022

drieseng Feb 27, 2022

madelson Feb 27, 2022

madelson commented Feb 28, 2022

jkotas commented Feb 28, 2022

buyaa-n commented Mar 1, 2022

jkotas commented Mar 1, 2022

buyaa-n Mar 1, 2022 •

edited

Loading

madelson Mar 2, 2022

buyaa-n Mar 2, 2022 •

edited

Loading

jkotas Mar 2, 2022

buyaa-n commented Mar 1, 2022 •

edited

Loading

buyaa-n left a comment

buyaa-n commented Mar 4, 2022

Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

Avoid Attribute.GetCustomAttributes() returning null for open generic type #65237

Conversation

madelson commented Feb 12, 2022

ghost commented Feb 12, 2022

jkotas commented Feb 15, 2022

madelson commented Feb 16, 2022 • edited Loading

jkotas commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

jkotas commented Feb 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson commented Feb 18, 2022

jkotas commented Feb 19, 2022

madelson commented Feb 26, 2022

jkotas commented Feb 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson commented Feb 28, 2022

jkotas commented Feb 28, 2022

buyaa-n commented Mar 1, 2022

jkotas commented Mar 1, 2022

buyaa-n Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buyaa-n commented Mar 1, 2022 • edited Loading

buyaa-n left a comment

Choose a reason for hiding this comment

buyaa-n commented Mar 4, 2022

madelson commented Feb 16, 2022 •

edited

Loading

jkotas Feb 16, 2022 •

edited

Loading

buyaa-n Mar 1, 2022 •

edited

Loading

buyaa-n Mar 2, 2022 •

edited

Loading

buyaa-n commented Mar 1, 2022 •

edited

Loading