Skip to content

Conversation

AaronRobinsonMSFT
Copy link
Member

This follows the pattern for other names that
are cached on RuntimeType.

Fixes #118389

This follows the pattern for other names that
are cached on RuntimeType.
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Aug 7, 2025
@Copilot Copilot AI review requested due to automatic review settings August 7, 2025 17:30
Copilot

This comment was marked as outdated.

Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Aug 8, 2025

// This method looks like an attractive inline but expands to two calls,
// neither of which can be inlined or optimized further. So block it
// from inlining.
[MethodImpl(MethodImplOptions.NoInlining)]
private string? GetCachedName(TypeNameKind kind) => Cache.GetName(kind);
looks like a very questionable comment. We use the Cache property in number of places.

TypeNameKind enum and the overall of structure of this code looks overengineered and unnecessarily inefficient. I am wondering why it cannot be just:

string? AssemblyQualifiedName => Cache.AssemblyQualifiedName;

string? AssemblyQualifiedName => m_assemblyQualifiedName ?? 
     HasRoundtripableFullName(m_runtimeType) ?
         ConstructName(ref m_assemblyQualifiedName, TypeNameFormatFlags.FormatNamespace | TypeNameFormatFlags.FormatFullInst | TypeNameFormatFlags.FormatAssembly) : null;

Dtto for all other names.

@jkotas
Copy link
Member

jkotas commented Aug 8, 2025

Console.WriteLine(typeof(delegate**).Namespace); is crashing with BadImageFormatException. It should return null instead.

Would you like to fix this one as well in this PR, or is it left for a separate PR? (It should be just about switching order of two lines and adding a test case.)

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

Update Namespace property for function pointers.
@AaronRobinsonMSFT AaronRobinsonMSFT enabled auto-merge (squash) August 8, 2025 23:06
@AaronRobinsonMSFT
Copy link
Member Author

@ericstj FYI this added some new tests for the Reflection stack and reworked some caching logic in RuntimeType.

Remove redundant logic in FullName property.
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR caches the AssemblyQualifiedName property value on RuntimeType to improve performance by avoiding repeated computation. This follows the established pattern where other name properties like Name, FullName, and ToString are already cached.

Key changes include:

  • Adding caching for AssemblyQualifiedName property in both CoreCLR and Mono implementations
  • Consolidating name formatting logic and improving null handling for function pointers
  • Removing unused FormatNoVersion flag from type string formatting

Reviewed Changes

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

Show a summary per file
File Description
src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs Adds caching for AssemblyQualifiedName, refactors name construction methods, and improves namespace handling
src/mono/System.Private.CoreLib/src/System/RuntimeType.Mono.cs Updates method signatures to return nullable strings and improves function pointer handling
src/libraries/System.Private.CoreLib/src/System/RuntimeType.cs Adds shared helper method IsFullNameRoundtripCompatible for consistent name validation
src/coreclr/vm/typestring.h Removes unused FormatNoVersion flag from type string formatting
src/coreclr/vm/typestring.cpp Simplifies assembly display name formatting by removing conditional version handling
src/libraries/System.Runtime/tests/System.Reflection.Tests/TypeInfoTests.cs Adds tests for function pointer FullName and Namespace behavior

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 647cdc5 into dotnet:main Aug 11, 2025
139 of 141 checks passed
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the runtime_118389 branch August 12, 2025 00:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should Type.AssemblyQualifiedName be cached?
2 participants