builder.Append('T');

Adding this character doesn't look necessary. #Closed

Answered elsewhere. We're trying to conform to valid IL

builder.Append(StringExtensions.GetNumeral(typeParameter.Ordinal));

Is it really useful to add an ordinal at the end of a declaration? It feels like it is better to have two distinct helpers "add declaration" and "add reference" because they really have nothing in common in terms of behavior. #Closed

I can split into two helpers. But both declaration and reference should have "T" and an ordinal, to conform to valid IL

Okay. I made a change to deviate from valid IL

GetCciAdapter

It worries me that we start using helpers like this outside of emit layer #Closed

Count()

Isn't there a property for that? #Closed

if (i > 0)

For simplicity, we could always add comma at the end of each type as we add spaces after non-type constraints. #Closed

I kept this as-is. The trailing comma just feels ugly and the complexity is minimal

typeParameter

In an error scenario this could be a type parameter of an enclosing type. Right? #Closed

I think we can drop these prefixes otherwise we are going to allow class and struct to conflict by full name. #Closed

Are you thinking we'll use the extension grouping type name directly as the key for purpose of grouping and conflicts?
The reason I included those keywords is to conform to valid IL. There are some places where the IL won't parse without those keywords (illustrated in some tests).

The reason I included those keywords is to conform to valid IL.

I do not think it is our goal to produce a proper IL. We certainly use IL grammar as an inspiration, but I think we can drop some aspects of it that are not part of our specification of what should be included into the content-based name of the extension grouping type.

We probably shouldn't include arity this way. We should simply use metadata name. It may or may not include arity, but CLR uses only that name to match types #Closed

builder.Append('`');

This suffix is not always present in metadata. #Closed

Similar comment about arity as for code in appendContainingType #Closed

builder.Append(',');

I guess it is fine to not include sizes and lower bounds because:

1. they cannot be expressed in syntax
2. Even if they can be non-default, we probably do not want to differentiate the name by them.

Consider adding a comment that we are intentionally ignoring them. #Closed

appendModifiers(functionPointer.Signature.ReturnTypeWithAnnotations.CustomModifiers, builder);

This doesn't look right. There could be ReturnTypeWithAnnotations.CustomModifiers even when functionPointer.Signature.RefKind != RefKind.None #Closed

I don't think that's possible. See logic in FunctionPointerMethodSymbol.CreateFromSource

I don't think that's possible. See logic in FunctionPointerMethodSymbol.CreateFromSource

I am pretty sure the model can represent this and such a symbol can be imported from metadata. If you are assuming some constraints on the symbols that can come through here, consider adding an assert in the branch above, and, perhaps, a comment explaining the assumption.

RefCustomModifiers

I think there could be non-ref custom modifiers #Closed

appendType(typeArguments[i].Type, builder);

Should we worry about custom modifiers? #Closed

modifierStrings.Sort(StringComparer.Ordinal);

In metadata order of custom modifiers is significant. like order of parameters and we are not sorting parameters. #Closed

((PublicModel.NamedTypeSymbol)modifier.Modifier).UnderlyingTypeSymbol

Consider using CSharpCustomModifier.ModifierSymbol instead #Closed

IsEmpty

IsDefault? #Closed

// Note: in valid IL, we would need a valid identifier name

This doesn't look accurate. For example, here is a quote from ECMA-335:

Within a generic type definition, its generic parameters are referred to by their index. Generic
parameter zero is referred to as !0, generic parameter one as !1, and so on. Similarly, within the body
of a generic method definition, its generic parameters are referred to by their index; generic parameter
zero is referred to as !!0, generic parameter one as !!1, and so on.

And a simple search for "!0" finds many examples of its usage in type names:

.class …
MultiSet1<T> extends class Set1<!0[]>

Etc. #Closed

builder.Append(StringExtensions.GetNumeral(typeParameter.Ordinal));

Do we really need the ordinal at this position? #Closed

modifierStrings

No longer used #Closed

Consider removing code from here to the end of the test. It looks like a WIP artifact. #Closed

It looks like there are artifacts like that in other tests too. I do not think we should be testing CompileIL behavior. Even if it changes, I don't think we will need to do anything about it.

builder.Append("void");

It looks like other special types also have keyword-like names (see I.8.2.2 Built-in value and reference types). Consider using them as well, at least for those types that have a dedicated entry in "II.23.1.16 Element types used in signatures" table. #Closed

I considered, tried it that and ended up reverting it as it was not clearly worth extra logic. The reason I kept void is to try to conform to valid IL as a guiding principle
Relevant test is GroupingTypeRawName_10

Is the '*' character really necessary? #Closed

I've tried to conform to valid IL as a guiding principle. There are some deviations:

• For "class"/"valuetype" keyword, there was a compelling argument to remove.
• For named type parameters, there was a compelling reason too (see GroupingTypeRawName_65).

For * and comma logic, we could deviate too but I'd rather not.

unmanagedType

No longer used. #Closed

dSrc

No longer used #Closed

dSrc

No longer used #Closed

dSrc

No longer used #Closed

nit: this could be simplified by starting the for at i = 1 instead. #Resolved

Consider testing dynamic #Resolved

Aren't these going to result in the same raw name?

unsafe static class E
{
    extension(delegate*<@void>) { }
    extension(delegate*<void>) { }
}

class @void;

I guess that's acceptable though. #Resolved

Should variance (in T and out T) be also included in the name? In any case, consider testing that. #Resolved

Should we file an issue for this? #Resolved

The ildasm source in runtime repo handles this (that's how I found the correct representation) but we're not using that version yet.