Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Custom attribute properties were not always applied correctly #44813

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Custom attribute properties were not always applied correctly #44813

merged 1 commit into from
Nov 19, 2020

Conversation

steveharter
Copy link
Member

@steveharter steveharter commented Nov 17, 2020 •
edited
Loading

Fixes #31195

Note the two new tests were failing before this change with:

    System.Reflection.Tests.CustomAttributeTests.AttributeWithDifferentPropertyTypes [FAIL]
      System.Reflection.CustomAttributeFormatException : 'StringArray' property specified was not found.
    System.Reflection.Tests.CustomAttributeTests.AttributeWithSamePropertyTypes [FAIL]
      Assert.Null() Failure
      Expected: (null)
      Actual:   Object[] [""]

@steveharter steveharter added this to the 6.0.0 milestone Nov 17, 2020
@steveharter steveharter self-assigned this Nov 17, 2020
GrabYourPitchforks
GrabYourPitchforks approved these changes Nov 19, 2020
View reviewed changes
src/coreclr/src/vm/customattribute.cpp
@@ -937,6 +936,7 @@ FCIMPL7(void, COMCustomAttribute::GetPropertyOrFieldData, ReflectModuleBaseObjec
break;
case SERIALIZATION_TYPE_SZARRAY:
{
*value = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should this be moved above the switch statement to make this code more future-proof should new case statements be added in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was considered, but there's the tiny cost of double assignment.

@steveharter steveharter merged commit 63923b5 into dotnet:master Nov 19, 2020
@steveharter steveharter deleted the arrayAttributes branch November 19, 2020 21:16
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@GrabYourPitchforks GrabYourPitchforks GrabYourPitchforks approved these changes

Assignees

@steveharter steveharter

Labels
area-System.Reflection
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

CustomAttribute with Array Properties sets nulls incorrectly
2 participants
@steveharter @GrabYourPitchforks