DocCommentId incorrect for a nested type of a generic class

[joelmartinez]

Originating bug report on the .net core API docs: https://github.com/dotnet/dotnet-api-docs/issues/2854

To summarize, we start with this method signature:

public static ImmutableArray<TSource> ToImmutableArray<TSource>(this ImmutableArray<TSource>.Builder builder)

Compiler output is

M:System.Collections.Immutable.ImmutableArray.ToImmutableArray``1(System.Collections.Immutable.ImmutableArray{``0}.Builder)

and DocCommentId output is

M:System.Collections.Immutable.ImmutableArray.ToImmutableArray``1(System.Collections.Immutable.ImmutableArray`1.Builder{``0})

I verified it with the following minimal example:

public class Test<T> {
	public class Builder {}

	public static Test<T> ToTest (Test<T>.Builder b) => new Test<T> ();
}

which gives the following result

M:Mono.Cecil.Tests.Test`1.ToTest(Mono.Cecil.Tests.Test`1.Builder{`0})

Simple test case was written against the cecil master branch in the unit test project ...

[jbevain]

@joelmartinez thanks for reporting this. Are you going to submit a PR for the issue?

[joelmartinez]

@jbevain I will if it's not resolved by the time the original issue gets prioritized onto my plate ( ;) @mimisasouvanh). Not 100% sure when that will be, but I'll definitely need to resolve that at some point in the future ... so I'm definitely up for working on it at that point.

[joelmartinez]

Ok, so doing some more investigation on this ... when I debug into the DocCommentId class, the Builder class's .ToString() method returns this:

Mono.Cecil.Tests.Test`1/Builder<T>

It is a GenericInstanceType, and .HasGenericParameters == true, but there are no elements in .GenericParameters, only one entry in .GenericArguments that contains the T parameter. Looking at Roslyn's logic here, it seems that .IsGenericType is false, based on the resulting docid in the docxml.

@jbevain, can you comment on whether there could be an issue in mono.cecil's parsing of this type's metadata? Offhand, I would expect .HasGenericParameters to be false for Builder ... there only being a generic instance on the .DeclaringType, right?

[jbevain]

@joelmartinez sadly Cecil is correct. At the metadata level, a nested type is almost a standalone type, so it has no knowledge of its declaring type generic parameters.

So for:

class List<T>
{
    public class Enumerator {}
}

At the metadata level you'll have an Enumerator<T>

and for:

class Foo<X>
{
    public class Bar<Y> {}
}

You'll have a Bar<X,Y>.

[joelmartinez]

@jbevain I figured it was something like that. But that then begs the question ... is roslyn wrong here? Or should I somehow adjust DocCommentId here with some special cases to watch out for this particular scenario and have different output?

[jbevain]

It's possible that Roslyn is accounting for this oddity and erasing the genericity of the nested type to make it closer to C#.

[joelmartinez]

Building on this and leaving notes on this gh issue as I go ... I've got an initial fix/implementation working for this scenario, but I'm going to have to continue extending the new implementation. As a test, I made a .net core 2.2 app that replicates this particular test, so the following set of classes:

using System;

///<summary>class</summary>
public class Test<T> {
    
    ///<summary>inner class</summary>
	public class Builder {}

    ///<summary>class method</summary>
	public static Test<T> ToTest (Test<T>.Builder b) => new Test<T> ();
}

///<summary>class</summary>
public class Test<T, K> {
    
    ///<summary>inner class</summary>
	public class Builder<K> {}

    ///<summary>class method</summary>
	public static Test<T, K> ToTest (Test<T, K>.Builder<K> b) => new Test<T, K> ();
}

The relevant method signatures are as follows ... I've got the first one working:

M:Test`1.ToTest(Test{`0}.Builder)

but if I make it more complicated, this is where I need to adjust

M:Test`2.ToTest(Test{`0,`1}.Builder{`1})

So it looks like I need to write the generic list, and simply exclude any arguments that are also present on the declaring type.

@jbevain does this sound like a good approach for this new implementation?

[jbevain]

Yeah that looks sound. Don't forget that you can have multiple levels of nested types for more fun!

[joelmartinez]

lol, yeah ... I'll make sure to include a number of tests and scenarios once it's implemented (while also double checking against the roslyn results). I don't know exactly when I'll finish this, but I'll have a PR for you at some point :)