Port .NET Native type name parser #1472

mateoatr · 2020-09-05T04:49:15Z

This adds the type name parsing logic used by CoreRT in reflection scenarios. With this we can successfully parse generic types, fixing scenarios such as #1469.

For reviewing I'd suggest paying particular attention to ReflectionTypeNameResolver in TypeParsing.cs; all other new files were ported from CoreRT with insignificant modifications.

vitek-karas

We should try hard to avoid changing existing behavior. I didn't dig deep enough to tell how much of a difference there is between the old and new algorithms, but at least the visible differences around assembly resolution should be removed.
Also - we should take this opportunity to add more tests - the fact that none of the assembly differences caused test failures means that we're missing coverage.
Also - given the bug which started all this - we should have tests for that bug and similar cases as well.

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

src/linker/Linker.Steps/LinkAttributesStep.cs

src/linker/Linker/TypeParsing.cs

src/linker/Linker.Steps/MarkStep.cs

test/Mono.Linker.Tests/Tests/TypeNameParserTests.cs

src/linker/Linker.Steps/MarkStep.cs

MichalStrehovsky · 2020-09-07T07:06:15Z

src/linker/Linker/AssemblyNameHelpers.cs

+
+			if (a.Version != null) {
+				Version canonicalizedVersion = a.Version.CanonicalizeVersion ();
+				if (canonicalizedVersion.Major != ushort.MaxValue) {


Did you make any changes to these files that we should review?

@marek-safar The fact that this repo uses formatting convention not used by the .NET teams makes source sharing frustrating.

I agree and this code should not land under src/linker/ but under external/<something> where it can use any formatting convention and it's clear where it's coming from. I don't know where this code was copied from but it should also copy tests as I'm not comfortable landing hundreds of lines of a parser code without test coverage.

This is originally extracted from the runtime reflection stack. The tests for that are the Type.GetType tests in the runtime repo - converting them to something workable is probably a non-trivial amount of work. If we need to service this, the fix should go through the runtime.

Given this code shipped in several major versions of a .NET runtime and the .NET Native compiler, I have pretty high confidence in it even with light (integration-level) test coverage here.

@MichalStrehovsky

Did you make any changes to these files that we should review?

Not really. Probably the most interesting change is this - how we parse the assembly version, which is a bit different from how we originally did it for .NET Native, however the code here is how we currently do it in CoreRT: https://github.com/dotnet/corert/blob/9d8bd29a71f68941aaa2c00e9c9a0eed86cafa10/src/System.Private.CoreLib/shared/System/Reflection/AssemblyNameFormatter.cs#L26-L52
That, and the fact that the ToString of NestedTypeName returns DeclaringType + "/" Name instead of DeclaringType + "+" Name, because of how Cecil encodes things.

@mateoatr please move the code under extern and undo the formating changes in the shared files

MichalStrehovsky · 2020-09-07T07:09:04Z

src/linker/Linker/TypeParsing.cs

+		{
+			if (typeName is ConstructedGenericTypeName) {
+				ConstructedGenericTypeName genericTypeName = (ConstructedGenericTypeName) typeName;
+				return assembly.MainModule.GetType (genericTypeName.GenericType.ToString ());


Will this lose the generic arguments?

One of the things #1387 calls out is Type.GetType("Foo[Bar]")) should make sure both Foo and Bar are kept. It doesn't look like that would happen here.

marek-safar · 2020-09-07T07:40:12Z

src/linker/Linker/AssemblyNameHelpers.cs

+using System.Collections.Generic;
+using System.Globalization;
+
+namespace System.Reflection.Runtime.Assemblies


The namespace does not match the file path (and tbh it has quite odd name)

Should I change all the places where this namespace is used to Mono.Linker?

No - move the files that come from the runtime to the external directory like Marek suggests below (something like external\TypeParsing\System\Reflection\Runtime\Assemblies) and add a README.md to the root of TypeParsing that tells the source and commit ID that was used as the base. That way we'll have future reference.

Also undo the formatting changes on the files that come from outside so that this is easier to diff, should we have a need.

src/linker/Linker/TypeParsing.cs

MichalStrehovsky · 2020-09-11T09:10:59Z

src/linker/Linker/TypeNameResolver.cs

+				var genericTypeDef = ResolveTypeName (assembly, constructedGenericTypeName.GenericType)?.Resolve ();
+				var genericInstanceType = new GenericInstanceType (genericTypeDef);
+				foreach (var arg in constructedGenericTypeName.GenericArguments) {
+					genericInstanceType.GenericArguments.Add (ResolveTypeName (assembly, arg));


If ResolveTypeName returns null, this will create a generic instance over null. That will probably crash things.

(Also for HasElementTypeName below.)

vitek-karas · 2020-09-11T11:34:46Z

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

 								if (foundType == null) {
 									// Intentionally ignore - it's not wrong for code to call Type.GetType on non-existing name, the code might expect null/exception back.
 									reflectionContext.RecordHandledPattern ();
 								} else {
-									reflectionContext.RecordRecognizedPattern (foundType, () => _markStep.MarkTypeVisibleToReflection (foundType, new DependencyInfo (DependencyKind.AccessedViaReflection, callingMethodDefinition), callingMethodDefinition));
-									methodReturnValue = MergePointValue.MergeValues (methodReturnValue, new SystemTypeValue (foundType));
+									var typeDef = foundType?.Resolve ();


This should be moved above the if and we should check that Resolve didn't return null. It's "easy" to create a type ref in Cecil - but there's no guarantee that it will actually resolve. If the Resolve fails here, we will likely nullref below.

vitek-karas · 2020-09-11T11:35:32Z

src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs

@@ -1344,7 +1345,7 @@ void ProcessCreateInstanceByName (ref ReflectionPatternContext reflectionContext
 								continue;
 							}

-							MarkConstructorsOnType (ref reflectionContext, resolvedType,
+							MarkConstructorsOnType (ref reflectionContext, resolvedType?.Resolve (),


Same here - Resolve may return null - which needs to be handled.

external/TypeParsing/README.md

external/TypeParsing/System/Reflection/AssemblyNameFormatter.cs

src/linker/Linker/ModuleDefinitionExtensions.cs

* Add type parser * PR feedback * Fix mono build * Move corert code to external * Match filepaths

* Add type parser * PR feedback * Fix mono build * Move corert code to external * Match filepaths Commit migrated from dotnet/linker@ff54278

Add type parser

a0b2791

mateoatr requested review from marek-safar, vitek-karas and MichalStrehovsky September 5, 2020 04:49

mateoatr self-assigned this Sep 5, 2020

vitek-karas requested changes Sep 6, 2020

View reviewed changes

MichalStrehovsky reviewed Sep 7, 2020

View reviewed changes

marek-safar requested changes Sep 7, 2020

View reviewed changes

Mateo Torres Ruiz added 2 commits September 10, 2020 00:22

PR feedback

075accd

Merge master

0053a53

mateoatr requested review from vitek-karas, MichalStrehovsky and marek-safar September 10, 2020 16:33

Fix mono build

b1ef30d

MichalStrehovsky reviewed Sep 11, 2020

View reviewed changes

vitek-karas reviewed Sep 11, 2020

View reviewed changes

Move corert code to external

9514e51

marek-safar reviewed Sep 14, 2020

View reviewed changes

external/TypeParsing/README.md Outdated Show resolved Hide resolved

marek-safar reviewed Sep 14, 2020

View reviewed changes

external/TypeParsing/System/Reflection/AssemblyNameFormatter.cs Outdated Show resolved Hide resolved

marek-safar reviewed Sep 14, 2020

View reviewed changes

src/linker/Linker/ModuleDefinitionExtensions.cs Show resolved Hide resolved

Match filepaths

cc1cd8f

vitek-karas approved these changes Sep 15, 2020

View reviewed changes

marek-safar approved these changes Sep 15, 2020

View reviewed changes

marek-safar merged commit ff54278 into dotnet:master Sep 16, 2020

mateoatr added a commit to mateoatr/linker that referenced this pull request Nov 13, 2020

Port .NET Native type name parser (dotnet#1472)

3fdf588

* Add type parser * PR feedback * Fix mono build * Move corert code to external * Match filepaths

This was referenced Nov 17, 2020

[release/5.0] Mark generic arguments of dynamically accessed types passed as string #1635

Closed

[release/5.0] Mark generic arguments of dynamically accessed types #1637

Merged

agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022

Port .NET Native type name parser (dotnet/linker#1472)

020dfaa

* Add type parser * PR feedback * Fix mono build * Move corert code to external * Match filepaths Commit migrated from dotnet/linker@ff54278

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port .NET Native type name parser #1472

Port .NET Native type name parser #1472

mateoatr commented Sep 5, 2020

vitek-karas left a comment

MichalStrehovsky Sep 7, 2020

marek-safar Sep 7, 2020

MichalStrehovsky Sep 8, 2020

mateoatr Sep 10, 2020

marek-safar Sep 11, 2020

MichalStrehovsky Sep 7, 2020

marek-safar Sep 7, 2020

mateoatr Sep 10, 2020

MichalStrehovsky Sep 11, 2020

MichalStrehovsky Sep 11, 2020

vitek-karas Sep 11, 2020

vitek-karas Sep 11, 2020

Port .NET Native type name parser #1472

Port .NET Native type name parser #1472

Conversation

mateoatr commented Sep 5, 2020

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment