Open joelmartinez opened 5 years ago
@joelmartinez thanks for reporting this. Are you going to submit a PR for the issue?
@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.
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?
@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>
.
@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?
It's possible that Roslyn is accounting for this oddity and erasing the genericity of the nested type to make it closer to C#.
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?
Yeah that looks sound. Don't forget that you can have multiple levels of nested types for more fun!
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 :)
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:
Compiler output is
and
DocCommentId
output isI verified it with the following minimal example:
which gives the following result
Simple test case was written against the cecil master branch in the unit test project ...