Add signature parser based on Roslyn doc comments #1197
Conversation
| // This includes '`n' for mangled generic types | ||
| builder.Append (typeReference.Name); | ||
|
|
||
| if (typeReference.HasGenericParameters || !typeReference.IsGenericInstance) |
There was a problem hiding this comment.
Is this condition correct?
There was a problem hiding this comment.
I think so - unless I missed a case in cecil. Non-generics don't need more work, and uninstantiated generics are already mangled. I added a clarifying comment.
- SignatureParser -> DocumentationSignatureParser - GetSignaturePart extension moved to DocumentationSignatureParser - Inverted a condition - Added a clarifying comment about generics
To let it build on mono
test/Mono.Linker.Tests.Cases.Expectations/Assertions/ExpectGeneratedStringAttribute.cs
Outdated
Show resolved
Hide resolved
And add comments clarifying their behavior
There were two issues: - Nunit doesn't like assertions inside of a TestCaseSource method. - On mono, the test infra failed to resolve the new assertion attributes
test/Mono.Linker.Tests/Tests/DocumentationSignatureParserTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I think we should make the prefix optional (the T: prefix and similar). You mentioned that Roslyn also makes it optional effectively. It would make the format pretty straightforward for simple cases.
If Roslyn doesn't support the ID without the prefix, then I'm fine we also require it - as we need to use the same syntax as Roslyn.
There was a problem hiding this comment.
I think we should make the prefix optional (the T: prefix and similar). You mentioned that Roslyn also makes it optional effectively. It would make the format pretty straightforward for simple cases.
If Roslyn doesn't support the ID without the prefix, then I'm fine we also require it - as we need to use the same syntax as Roslyn.
| if (PeekNextChar (id, index) == '{') { | ||
| typeArguments = new List<string> (); | ||
| if (!ParseTypeArguments (id, ref index, typeParameterContext, typeArguments)) { | ||
| continue; |
There was a problem hiding this comment.
Should we output a warning in places where the parsing fails?
There was a problem hiding this comment.
Ideally yes, but I'm not sure in this case. Roslyn doesn't raise a warning when this is malformed in SuppressMessage - it simply ignores it in that case. So I think linker should do the same in this case.
There was a problem hiding this comment.
Agreed it would be nice. Currently the way it will be done is that the caller of the parser gets to decide what kind of warnings/errors to log - which unfortunately doesn't distinguish between syntax errors and resolution errors. I initially tried to separate those two cases, but ended up just following the Roslyn implementation.
- Allow parsing a signature without a member type prefix - Allow parsing a member without a type name - Allow matching multiple member kinds - Rename some public methods to more closely match linker terminology - Add a few more tests for edge cases
- License headers - Don't use string interpolation - Don't use extension method for GetSignaturePart - Remove unnecessary nullable context - Remove unnecessary IndexOf
* Add signature parser based on Roslyn doc comments * Fix formatting * PR feedback - SignatureParser -> DocumentationSignatureParser - GetSignaturePart extension moved to DocumentationSignatureParser - Inverted a condition - Added a clarifying comment about generics * Avoid ImmutableArray To let it build on mono * Rename test attributes And add comments clarifying their behavior * Fix mono test failure There were two issues: - Nunit doesn't like assertions inside of a TestCaseSource method. - On mono, the test infra failed to resolve the new assertion attributes * Factor parser for use from DynamicDependencyAttribute - Allow parsing a signature without a member type prefix - Allow parsing a member without a type name - Allow matching multiple member kinds - Rename some public methods to more closely match linker terminology - Add a few more tests for edge cases * PR feedback - License headers - Don't use string interpolation - Don't use extension method for GetSignaturePart - Remove unnecessary nullable context - Remove unnecessary IndexOf * Update link for edge cases Commit migrated from dotnet/linker@0e38c61
This adds a parser for the format described at https://github.com/dotnet/csharplang/blob/master/spec/documentation-comments.md#id-string-format, which we plan to use for UnconditionalSuppressMessageAttribute (#1147) and probably also DynamicDependencyAttribute.
It's adapted from Roslyn, with some changes to allow for resolving from metadata instead of sources. This doesn't support the legacy fxcop format, and it also doesn't allow some special cases that Roslyn has - see comments in the sources. The Roslyn implementation uses a visitor pattern, where symbols in their syntax tree implement accept methods. This seemed hard to adapt to our use case, and I don't think we need the full generality of this pattern, so this parser follows the same structure without truly using the visitor pattern.