Fix: Incorrect order of checks for TypeBuilder `GetConstructor` and `GetField` method by BartoszKlonowski · Pull Request #53645 · dotnet/runtime

BartoszKlonowski · 2021-06-02T21:53:31Z

This pull request fixes#45988

It changes the order of checks for GetConstructor and GetField method according to the workflow of GetMethod.

Full explanation can be found in the linked issue.

Because of reordered checks of those method, unit tests had to be adjusted.
For both GetField and GetConstructor they now check whether for not generic type the element will be created without throwing.

janvorli · 2021-06-08T15:08:57Z

cc: @eerhardt

GetConstructor and GetField had the same incorrect checks order comparing to GetMethod as in the CoreCLR. To keep the consistent solution, Mono has also been adjusted.

eerhardt · 2021-06-14T23:10:08Z

@BartoszKlonowski - looks like it is still failing on mono.

BartoszKlonowski · 2021-07-02T20:30:26Z

@eerhardt Unfortunately, I've tried to understand the order of checks for Mono - GetMethod seems to be a lot different than GetField and GetConstructor.
Can you guide me a bit through it? Otherwise I will need more time to analyse that.

eerhardt · 2021-07-06T20:28:47Z

@lambdageek - any thoughts on the mono checks here? Can you help @BartoszKlonowski out?

lambdageek · 2021-07-06T20:58:26Z

@BartoszKlonowski I think you need something like this change
https://github.com/dotnet/runtime/pull/53645/files#diff-1767a4830dd55e36974e0b34848e8f736f0f5e852866b7afc0abbe078f2d5dd1R133-R134

in Mono's GetField and GetConstructor - ie instead always throwing if the type is a generic type definition
https://github.com/dotnet/runtime/pull/53645/files#diff-621fff19cdf2db5ff0e021ff669fcd0d1bb6453d59d10b3769a2be8b624a565eR1924-R1925

you should instead check whether the generic type definition that type is instantiated from is the same as the declaring type of the FieldInfo.

BartoszKlonowski · 2021-07-14T11:14:04Z

@lambdageek Thank you! Can you verify if I understood your advice correctly?

lambdageek · 2021-07-14T15:20:58Z

+
+ if (type == null)
+ throw new ArgumentException("Type is not generic", nameof(type));
+


I think this needs the same code that GetMethod has:
if(typeisTypeBuilder&&type.ContainsGenericParameters)type=type.MakeGenericType(type.GetGenericArguments());
(and same for GetField)

I've tried, but it won't build.

Looks like you also need
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern", Justification = "Type.MakeGenericType is used to create a typical instantiation")]

eerhardt · 2021-07-14T18:55:30Z

- return res;
- }
-
 private static bool IsValidGetMethodType(Type type)


Can you put this method back where it was originally declared? That will make reviewing the change much easier. Also when people look at source history, will be able to see what change was made here easily. #Closed

jeffhandley · 2021-07-23T20:56:46Z

@eerhardt Since you're the most active reviewer on this, I assigned this PR to you for follow-up/decision before the RC1 snap.

eerhardt · 2021-07-26T15:14:37Z

@BartoszKlonowski - I left 2 pieces of feedback above. Let me know if you will address these, or if you want me to resolve them. That way we can move this PR forward.

eerhardt · 2021-07-27T14:43:31Z

Looks like the tests are still failing:

 Starting: System.Reflection.Emit.Tests (parallel test collections = on, max threads = 4)
System.Reflection.Emit.Tests.TypeBuilderGetConstructor.GetConstructor_DeclaringTypeOfConstructorNotGenericTypeDefinitionOfType_ThrowsArgumentException [FAIL]
Assert.Equal() Failure
â†“ (pos 0)
Expected: type
Actual: constructor
â†‘ (pos 0)
Stack Trace:
/_/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs(123,0): at System.AssertExtensions.Throws[T](String expectedParamName, Func`1 testCode)

eerhardt · 2021-07-27T21:38:37Z

Tests are still failing:

System.Reflection.Emit.Tests.TypeBuilderGetConstructor.GetConstructor_TypeNotGeneric_ThrowsArgumentException [FAIL]
Assert.Equal() Failure
? (pos 0)
Expected: constructor
Actual: type
? (pos 0)
Stack Trace:
/_/src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs(123,0): at System.AssertExtensions.Throws[T](String expectedParamName, Func`1 testCode)
/_/src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs(77,0): at

Are you able to run them locally? That would give you a faster turn around to get the CI green.

BartoszKlonowski · 2021-08-03T17:31:12Z

@eerhardt I'm afraid I won't be able to finish this, sorry. I really appreciate your help and support, but analysing this and fixing the issue with tests would require much more time than I currently have...

steveharter · 2021-08-04T16:57:47Z

@eerhardt if you want to take over this PR, please do so ASAP otherwise I recommend closing. Thanks

eerhardt · 2021-09-07T15:05:58Z

I'll get to this PR once I am finished with my 6.0 deliverables.

…cks-order

Ensure checks are consistent across GetConstructor, GetField, and GetMethod for both coreclr and mono.

buyaa-n

LGTM, thanks!

BartoszKlonowski added 2 commits May 31, 2021 23:28

Reorder checks for TypeBuilder.GetConstructor()
d115433

Reorder checks for TypeBuilder.GetField()
22cf90a

ghost added the area-System.Reflection label Jun 2, 2021

Adjust unit test to reordered TypeBuilder checks
f2efe72

eerhardt reviewed Jun 8, 2021
View reviewed changes

Comment threadsrc/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs Outdated

eerhardt reviewed Jun 8, 2021
View reviewed changes

Comment threadsrc/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs Outdated

Use 'not' keyword for pattern matching
4940fc1

BartoszKlonowski requested a review from marek-safar as a code owner June 14, 2021 19:37

Apply the same Type checks for Mono
3472615
GetConstructor and GetField had the same incorrect checks order comparing to GetMethod as in the CoreCLR. To keep the consistent solution, Mono has also been adjusted.

karelz mentioned this pull request Jun 23, 2021
LocalEndPointTestIPv6Sync.TcpAcceptSocket_WhenServerBoundToSpecificAddress_LocalEPIsSame failing #52141
Closed

Check whether a declaring type isn't type's generic definition
fd50ec9

lambdageek reviewed Jul 14, 2021
View reviewed changes

BartoszKlonowski added 2 commits July 14, 2021 18:55

Make generic type if contains generic parameters
6fcfbf5

Suppres UnrecognizedReflectionPattern for GetField and GetConstructor
f039dc9

eerhardt reviewed Jul 14, 2021
View reviewed changes

Comment threadsrc/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs Outdated

terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021

jeffhandley assigned eerhardt Jul 23, 2021

BartoszKlonowski added 4 commits July 26, 2021 20:44

Move GetMethodType validation method at original order
863b984

Test GetConstructor against throwing an exception
f806283

Fix: copy-paste error 'type' -> 'constructor'
70f45eb

Align Mono and CoreCLR with nameof for GetConstructor
b2c2b4a

karelz mentioned this pull request Jul 27, 2021
Failed test SendAsync_CorrectVersionSelected_LoopbackServer #56026
Closed

Use type for argument exception and test against type parameter
98036aa

steveharter mentioned this pull request Aug 4, 2021
TypeBuilder::GetConstructor and GetField type checks are in the wrong order #45988
Closed

jeffhandley added this to the 7.0.0 milestone Aug 19, 2021

eerhardt added 2 commits October 4, 2021 14:15

Merge remote-tracking branch 'upstream/main' into fix-typebuilder-che…
474a10b
…cks-order

Get tests passing on both coreclr and mono.
23795a0
Ensure checks are consistent across GetConstructor, GetField, and GetMethod for both coreclr and mono.

eerhardt approved these changes Oct 6, 2021
View reviewed changes

eerhardt requested review from lambdageek and steveharter October 6, 2021 20:10

runfoappBot mentioned this pull request Oct 7, 2021
System.Tests.TimeZoneInfoTests.TestWindowsNlsDisplayNames is failing on rolling builds #60119
Closed

buyaa-n approved these changes Nov 1, 2021
View reviewed changes

Comment threadsrc/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs Outdated

Fix mono exception messages to use SR resources.
f7ed1ac

eerhardt merged commit f981785 into dotnet:mainNov 2, 2021

ghost locked as resolved and limited conversation to collaborators Dec 3, 2021


		if (type == null)
		throw new ArgumentException("Type is not generic", nameof(type));

Uh oh!

Conversation

BartoszKlonowski commented Jun 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janvorli commented Jun 8, 2021

Uh oh!

Uh oh!

Uh oh!

eerhardt commented Jun 14, 2021

Uh oh!

BartoszKlonowski commented Jul 2, 2021

Uh oh!

eerhardt commented Jul 6, 2021

Uh oh!

lambdageek commented Jul 6, 2021

Uh oh!

BartoszKlonowski commented Jul 14, 2021

Uh oh!

lambdageekJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

BartoszKlonowskiJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

lambdageekJul 14, 2021

Choose a reason for hiding this comment

Uh oh!

eerhardtJul 14, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeffhandley commented Jul 23, 2021

Uh oh!

eerhardt commented Jul 26, 2021

Uh oh!

eerhardt commented Jul 27, 2021

Uh oh!

eerhardt commented Jul 27, 2021

Uh oh!

BartoszKlonowski commented Aug 3, 2021

Uh oh!

steveharter commented Aug 4, 2021

Uh oh!

eerhardt commented Sep 7, 2021

Uh oh!

buyaa-n left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

BartoszKlonowski commented Jun 2, 2021•
edited
Loading

eerhardtJul 14, 2021•
edited
Loading