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.

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

Non-recursive TypeName.FullName implementation #112242

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -142,47 +142,111 @@ public string FullName
{
get
{
// This is a recursive method over potentially hostile input. Protection against DoS is offered
// via the [Try]Parse method and TypeNameParserOptions.MaxNodes property at construction time.
// This FullName property getter and related methods assume that this TypeName instance has an
// acceptable node count.
//
// The node count controls the total amount of work performed by this method, including:
// - The max possible stack depth due to the recursive methods calls; and
// - The total number of bytes allocated by this function. For a deeply-nested TypeName
// object, the total allocation across the full object graph will be
// O(FullName.Length * GetNodeCount()).

if (_fullName is null)
if (_fullName is not null) // Fast path for already computed FullName and nested types
{
if (IsConstructedGenericType)
if (_nestedNameLength > 0 && _fullName.Length > _nestedNameLength) // Declaring types
{
_fullName = TypeNameParserHelpers.GetGenericTypeFullName(GetGenericTypeDefinition().FullName.AsSpan(),
#if SYSTEM_PRIVATE_CORELIB
CollectionsMarshal.AsSpan(_genericArguments));
#else
_genericArguments.AsSpan());
#endif
// Stored fullName represents the full name of the nested type.
// Example: Namespace.Declaring+Nested
_fullName = _fullName.Substring(0, _nestedNameLength);
Copy link
Member

Choose a reason for hiding this comment

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

How about thread safety of this? Concurrent initialization with the same content should be safe, but this branch is modifying to different content.

Copy link
Member Author

Choose a reason for hiding this comment

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

TypeName is not thread safe. In worst case, multiple different threads are going to perform a string allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeName is not thread safe.

Isn't it? It's immutable. There might be some extra allocations but it would work fine when accessed from multiple threads, wouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, it's fine to assume that something that is immutable is also thread-safe.

And again, in worst case, multiple threads would allocate the same string and duplicate the work, but each of them would end up computing the same text.

}
else if (IsArray || IsPointer || IsByRef)

return _fullName;
}

// We need a Stack<T> to avoid recursion, but use just List<T> as Stack<T> is not a part of CoreLib.
List<NameInfo> stack = [new(this, NameFlags.None)];
danmoseley marked this conversation as resolved.
Show resolved Hide resolved
// Since we start from "this" which is the leaf node of the tree,
// we append all the information in a reverse order and then reverse the string at the end.
ValueStringBuilder builder = new(stackalloc char[128]);
while (stack.Count > 0)
{
NameInfo currentInfo = stack[stack.Count - 1];
stack.RemoveAt(stack.Count - 1);
Copy link
Member

Choose a reason for hiding this comment

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

Decrease stack.Count? Otherwise it should be infinite loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

RemoveAt takes care of decreasing the Count

TypeName current = currentInfo.typeName;

// For a generic type, we push its definition and all args to the stack.
// Definition`2[[Arg1],[Arg2]]
if (current.IsConstructedGenericType)
{
ValueStringBuilder builder = new(stackalloc char[128]);
builder.Append(GetElementType().FullName);
_fullName = TypeNameParserHelpers.GetRankOrModifierStringRepresentation(_rankOrModifier, ref builder);
stack.Add(new(current.GetGenericTypeDefinition(), NameFlags.GenericTypeDefinition | currentInfo.flags));
var genericArguments = current.GetGenericArguments();
for (int i = 0; i < genericArguments.Length; i++)
{
// TypeName is unaware whether it is a generic argument or not, we need to track it.
NameFlags flag = NameFlags.GenericArg;
if (i == 0)
{
flag |= NameFlags.FirstGenericArg;
}
if (i == genericArguments.Length - 1)
{
flag |= NameFlags.LastGenericArg;
}
stack.Add(new(genericArguments[i], flag));
}
}
else

// Append the closing braces and assembly name
// Generic`1[[ArgTypeName, ThisAssemblyNamePart]]
if ((currentInfo.flags & NameFlags.GenericArg) != 0)
{
Debug.Fail("Pre-allocated full name should have been provided in the ctor");
if ((currentInfo.flags & (NameFlags.GenericTypeDefinition | NameFlags.SuffixAlreadyPrinted)) == 0)
{
builder.Append(']'); // every generic arg ends with ']'
if ((currentInfo.flags & NameFlags.LastGenericArg) != 0)
{
builder.Append(']'); // the last generic arg ends with ']]'
}

if (current.AssemblyName is not null) // generic arg is always fully qualified
{
AppendInReverseOrder(ref builder, current.AssemblyName.FullName.AsSpan());
builder.Append(" ,");
}
}
}

if (current.IsArray || current.IsPointer || current.IsByRef)
{
TypeNameParserHelpers.AppendReversedRankOrModifierStringRepresentation(current._rankOrModifier, ref builder);
stack.Add(new(current.GetElementType(), currentInfo.flags | NameFlags.SuffixAlreadyPrinted));
}
else if (current.IsSimple && current._fullName is not null)
{
if ((currentInfo.flags & NameFlags.GenericTypeDefinition) != 0)
{
builder.Append('['); // generic type definition is followed with '[': "List`1["
}

int length = current._nestedNameLength > 0 ? current._nestedNameLength : current._fullName.Length;
AppendInReverseOrder(ref builder, current._fullName.AsSpan(0, length));

if ((currentInfo.flags & NameFlags.GenericArg) != 0)
{
builder.Append('['); // every generic arg starts with '['

if ((currentInfo.flags & NameFlags.FirstGenericArg) == 0)
{
builder.Append(','); // every generic arg except the first one is followed with ','
}
}
}
}
else if (_nestedNameLength > 0 && _fullName.Length > _nestedNameLength) // Declaring types

Span<char> characters = builder.RawChars.Slice(0, builder.Length);
characters.Reverse();
Copy link
Preview

Copilot AI Feb 7, 2025

Choose a reason for hiding this comment

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

Consider using a more efficient method to reverse the string if performance is a concern.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
_fullName = characters.ToString();
builder.Dispose();
Comment on lines +239 to +240
Copy link
Member

Choose a reason for hiding this comment

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

ToString already calls Dispose

Copy link
Member Author

Choose a reason for hiding this comment

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

But in this case I am not calling builder.ToString(), but builder.RawChars.ToString which does not call the Dispose

return _fullName;

static void AppendInReverseOrder(ref ValueStringBuilder builder, ReadOnlySpan<char> value)
{
// Stored fullName represents the full name of the nested type.
// Example: Namespace.Declaring+Nested
_fullName = _fullName.Substring(0, _nestedNameLength);
for (int i = value.Length - 1; i >= 0; i--)
{
builder.Append(value[i]);
}
}

return _fullName!;
}
}

Expand Down Expand Up @@ -537,5 +601,18 @@ private TypeName MakeElementTypeName(int rankOrModifier)
genericTypeArguments: ImmutableArray<TypeName>.Empty,
rankOrModifier: rankOrModifier);
#endif

private record struct NameInfo(TypeName typeName, NameFlags flags);

[Flags]
private enum NameFlags : byte
{
None = 0,
GenericTypeDefinition = 1,
FirstGenericArg = 2,
GenericArg = 4,
LastGenericArg = 8,
SuffixAlreadyPrinted = 16
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,26 +20,6 @@ internal static class TypeNameParserHelpers
private static readonly SearchValues<char> s_endOfFullTypeNameDelimitersSearchValues = SearchValues.Create("[]&*,+\\");
#endif

internal static string GetGenericTypeFullName(ReadOnlySpan<char> fullTypeName, ReadOnlySpan<TypeName> genericArgs)
{
Debug.Assert(genericArgs.Length > 0);

ValueStringBuilder result = new(stackalloc char[128]);
result.Append(fullTypeName);

result.Append('[');
foreach (TypeName genericArg in genericArgs)
{
result.Append('[');
result.Append(genericArg.AssemblyQualifiedName); // see recursion comments in TypeName.FullName
result.Append(']');
result.Append(',');
}
result[result.Length - 1] = ']'; // replace ',' with ']'

return result.ToString();
}

/// <returns>Positive length or negative value for invalid name</returns>
internal static int GetFullTypeNameLength(ReadOnlySpan<char> input, out bool isNestedType)
{
Expand Down Expand Up @@ -203,6 +183,35 @@ internal static string GetRankOrModifierStringRepresentation(int rankOrModifier,
return builder.ToString();
}

internal static void AppendReversedRankOrModifierStringRepresentation(int rankOrModifier, ref ValueStringBuilder builder)
{
if (rankOrModifier == ByRef)
{
builder.Append('&');
}
else if (rankOrModifier == Pointer)
{
builder.Append('*');
}
else
{
builder.Append(']');

if (rankOrModifier == 1)
{
builder.Append("*");
}
else if (rankOrModifier != SZArray)
{
Debug.Assert(rankOrModifier >= 2);

builder.Append(',', rankOrModifier - 1);
}

builder.Append('[');
}
}

/// <summary>
/// Are there any captured generic args? We'll look for "[[" and "[" that is not followed by "]", "*" and ",".
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,7 @@ public void AppendRankOrModifierStringRepresentationAppendsExpectedString(int in
[InlineData(typeof(ValueTuple<bool, short, int, DateTime, char, ushort, long, sbyte>))]
public void GetGenericTypeFullNameReturnsSameStringAsTypeAPI(Type genericType)
{
TypeName openGenericTypeName = TypeName.Parse(genericType.GetGenericTypeDefinition().FullName.AsSpan());
ReadOnlySpan<TypeName> genericArgNames = genericType.GetGenericArguments().Select(arg => TypeName.Parse(arg.AssemblyQualifiedName.AsSpan())).ToArray();

Assert.Equal(genericType.FullName, TypeNameParserHelpers.GetGenericTypeFullName(openGenericTypeName.FullName.AsSpan(), genericArgNames));
Assert.Equal(genericType.FullName, TypeName.Parse(genericType.FullName.AsSpan()).FullName);
}

[Theory]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -834,7 +834,9 @@ public void ArrayRank_SByteOverflow()
[InlineData(typeof(List<int>))]
[InlineData(typeof(List<List<int>>))]
[InlineData(typeof(Dictionary<int, string>))]
[InlineData(typeof(Dictionary<int[], string[,,,,,]>))]
[InlineData(typeof(Dictionary<string, List<string>>))]
[InlineData(typeof(Dictionary<int?, List<List<List<string>>>>))]
[InlineData(typeof(NestedNonGeneric_0))]
[InlineData(typeof(NestedNonGeneric_0.NestedNonGeneric_1))]
[InlineData(typeof(NestedGeneric_0<int>))]
Expand Down
Loading