Add `TypeName` APIs to simplify metadata lookup. #111598

teo-tsirpanis · 2025-01-20T01:13:09Z

Tests for Unescape TBA.

dotnet-issue-labeler · 2025-01-20T01:13:14Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

dotnet-issue-labeler · 2025-01-20T01:13:16Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

jkotas · 2025-01-20T02:36:19Z

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

+                return input;
+            }
+
+            ValueStringBuilder builder = new(stackalloc char[128]);


stackalloc is relatively expensive when it is not reachable. It should be in the helper method too.

jkotas · 2025-01-20T02:39:56Z

Could you please delete

runtime/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems

Lines 1492 to 1494 in 29013d8

    
           <Compile Include="$(CommonPath)System\Reflection\Metadata\TypeNameHelpers.cs"> 
        
             <Link>Common\System\Reflection\Metadata\TypeNameHelpers.cs</Link> 
        
           </Compile>

and replace the calls to it with this API?

(Other uses of TypeNameHelpers.cs should be delete too once these APIs are available in the LKG runtime.)

jkotas · 2025-01-20T02:47:31Z

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

+                    }
+                    builder.Append(input.Slice(0, indexOfEscapeChar));
+                    int indexOfNextChar = indexOfEscapeChar + 1;
+                    if (indexOfNextChar < input.Length && input[indexOfNextChar] is char c && NeedsEscaping(c))


You should not need to call NeedsEscaping here. Take a look how the existing Unescape helper in TypeNameHelpers.cs is implemented.

Given that this is very rare case, it is better to keep the implementation as simple as possible - the existing Unescape helper in TypeNameHelpers.cs that processes one character at a time looks simpler.

Copied implementation from TypeNameHelpers.cs, removed it and updated usages.

jkotas · 2025-01-20T02:49:52Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

+        /// </summary>
+        /// <param name="name">The input string containing the name to convert.</param>
+        /// <returns>A string of characters with any escaped characters converted to their unescaped form.</returns>
+        /// <remarks>The unescaped string can be used for looking up the type name or namespace in metadata.</remarks>


Also mention that the API does not validate that the escape sequences are only used for characters that require escaping?

jkotas · 2025-01-20T03:01:36Z

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs

@@ -13,13 +13,14 @@ namespace System.Reflection.Metadata.Tests
    public class TypeNameTests


Is there test coverage for parsing a nested types with namespace e.g. TypeName.Parse("a.b+c.d")?

What is TypeName.Parse("a.b+c.d").Namespace going to return?

I guess it will return a.b+c which is obviously wrong. I didn't know this syntax is supported, TIL!

I think it should return a, because the reflection APIs seem to ignore the namespace in the nested type. Passing System.Collections.Generic.Dictionary`2+MyRandomNamespace.Enumerator to Type.GetType returns the expected result, while if I flip the namespaces it returns null.

Updated to return a in your example.

Passing System.Collections.Generic.Dictionary`2+MyRandomNamespace.Enumerator to Type.GetType returns the expected result

.NET Framework throws exception in this case. This looks like a bug in .NET Core - regression introduced by rewrite of the type name parser in C#. I do not think that the parser should be silently dropping any significant part of the input to the floor.

Updated to return a in your example.

I think that the behavior for my example should be:

Namespace returns "c", Name returns "d"
or

Namespace returns empty string, Name returns "c.d"

Copilot reviewed 5 out of 17 changed files in this pull request and generated no comments.

Files not reviewed (12)

src/coreclr/tools/ILVerification/ILVerification.projitems: Language not supported
src/coreclr/tools/aot/ILCompiler.Compiler/ILCompiler.Compiler.csproj: Language not supported
src/coreclr/tools/aot/ILCompiler.TypeSystem/ILCompiler.TypeSystem.csproj: Language not supported
src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems: Language not supported
src/tools/illink/src/linker/Mono.Linker.csproj: Language not supported
src/libraries/Common/src/System/Reflection/Metadata/TypeNameHelpers.cs: Evaluated as low risk
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CustomAttributeTypeNameParser.cs: Evaluated as low risk
src/tools/illink/src/linker/Linker/TypeNameResolver.cs: Evaluated as low risk
src/mono/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.Mono.cs: Evaluated as low risk
src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs: Evaluated as low risk
src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.CoreCLR.cs: Evaluated as low risk
src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.NativeAot.cs: Evaluated as low risk

teo-tsirpanis added 2 commits January 20, 2025 03:11

Add TypeName.Namespace and tests.

a573f96

Add TypeName.Unescape.

dfe213f

dotnet-issue-labeler bot added area-System.Reflection.Metadata new-api-needs-documentation labels Jan 20, 2025

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 20, 2025

teo-tsirpanis added 3 commits January 20, 2025 04:09

Fix infinite loops.

2037805

Simplify loop.

d0c0e02

Update reference assembly.

3567d78

jkotas reviewed Jan 20, 2025

View reviewed changes

jkotas requested a review from adamsitnik January 20, 2025 03:02

teo-tsirpanis added 6 commits January 20, 2025 21:35

Address PR feedback around Unescape.

afc4cb1

Remove file with duplicate Unescape method.

dde6eda

Fix tests.

1da808a

Reduce allocations when calling Namespace across a type name hierachy.

ae14c6a

Fix nested types with namespaces.

0b03353

Add tests for Unescape.

41d2f7c

teo-tsirpanis requested a review from Copilot January 20, 2025 22:15

Copilot AI reviewed Jan 20, 2025

View reviewed changes

teo-tsirpanis force-pushed the typename-namespace-unescape branch from e1a9730 to d08be6b Compare January 20, 2025 22:19

Fix compile errors.

1e3f969

teo-tsirpanis force-pushed the typename-namespace-unescape branch from d08be6b to 1e3f969 Compare January 20, 2025 22:30

teo-tsirpanis force-pushed the typename-namespace-unescape branch from 1e8ddbf to e5ecca6 Compare January 20, 2025 23:09

Restore TypeNameHelpers and use it in all places except CoreLib.

269d0cc

teo-tsirpanis force-pushed the typename-namespace-unescape branch from e5ecca6 to 269d0cc Compare January 20, 2025 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `TypeName` APIs to simplify metadata lookup. #111598

Add `TypeName` APIs to simplify metadata lookup. #111598

teo-tsirpanis commented Jan 20, 2025

dotnet-issue-labeler bot commented Jan 20, 2025

dotnet-issue-labeler bot commented Jan 20, 2025

jkotas Jan 20, 2025

teo-tsirpanis Jan 20, 2025

jkotas commented Jan 20, 2025

jkotas Jan 20, 2025

jkotas Jan 20, 2025

teo-tsirpanis Jan 20, 2025

jkotas Jan 20, 2025

teo-tsirpanis Jan 20, 2025

jkotas Jan 20, 2025 •

edited

Loading

teo-tsirpanis Jan 20, 2025

teo-tsirpanis Jan 20, 2025

jkotas Jan 21, 2025

		@@ -13,13 +13,14 @@ namespace System.Reflection.Metadata.Tests
		public class TypeNameTests

Add TypeName APIs to simplify metadata lookup. #111598

Are you sure you want to change the base?

Add TypeName APIs to simplify metadata lookup. #111598

Conversation

teo-tsirpanis commented Jan 20, 2025

dotnet-issue-labeler bot commented Jan 20, 2025

dotnet-issue-labeler bot commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jan 20, 2025

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

jkotas Jan 20, 2025 • edited Loading

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

Add `TypeName` APIs to simplify metadata lookup. #111598

Add `TypeName` APIs to simplify metadata lookup. #111598

jkotas Jan 20, 2025 •

edited

Loading