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

Add SymbolDisplayGenericsOptions.IncludeArity as an alternative to SymbolDisplayGenericsOptions.IncludeTypeParameters #73051

Closed
ericstj opened this issue Apr 16, 2024 · 14 comments
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@ericstj
Copy link
Member

ericstj commented Apr 16, 2024

Background and Motivation

Today the only value for SymbolDisplayGenericsOptions that shows the number of type parameters is IncludeTypeParameters which includes all the parameter names.

Parameter names are not strictly speaking part of the surface area. Derived types can use different parameter names, and changing the generic parameter name is not breaking.

APICompat uses SymbolDisplayGenericsOptions when producing a comparison key for API. PublicApiAnalyzer does as well. As a result both of these tools prohibit changing the name of a generic parameter.

Proposed API

namespace Microsoft.CodeAnalysis
{
    /// <summary>
    /// Specifies the options for how generics are displayed in the description of a symbol.
    /// </summary>
    [Flags]
    public enum SymbolDisplayGenericsOptions
    {
        /// <summary>
        /// Omits the type parameter list entirely.
        /// </summary>
        None = 0,

        /// <summary>
        /// Includes the type parameters. 
        /// For example, "Goo&lt;T&gt;" in C# or "Goo(Of T)" in Visual Basic.
        /// </summary>
        IncludeTypeParameters = 1 << 0,

        /// <summary>
        /// Includes type parameters and constraints.
        /// For example, "where T : new()" in C# or "Of T as New" in Visual Basic.
        /// </summary>
        IncludeTypeConstraints = 1 << 1,

        /// <summary>
        /// Includes <c>in</c> or <c>out</c> keywords before variant type parameters.
        /// For example, "Goo&lt;out T&gt;" in C# or (Goo Of Out T" in Visual Basic.
        /// </summary>
        IncludeVariance = 1 << 2,

+        /// <summary>
+        /// Represents the arity while omitting the actual type parameter names.
+        /// For example, "Dictionary&lt;,&gt;" in C# or "Dictionary(Of ,)" in Visual Basic.
+        /// </summary>
+        IncludeArity = 1 << 3,
    }
}

Usage Examples

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;

// Create a format with specific options
var format = SymbolDisplayFormat.FullyQualifiedFormat.WithGenericsOptions(SymbolDisplayGenericsOptions.IncludeArity);

var type = typeof(Dictionary<,>);
MetadataReference mdRef = MetadataReference.CreateFromFile(type.Assembly.Location);

var cSharpCompilation = CSharpCompilation.Create($"AssemblyLoader_{DateTime.Now:MM_dd_yy_HH_mm_ss_FFF}")
    .AddReferences(mdRef);

var typeSymbol = cSharpCompilation.GetTypeByMetadataName(type.FullName);

var formattedType = typeSymbol.ToDisplayString(format);

Console.WriteLine($"Formatted type: {formattedType}");

Expected output:

Formatted type: global::System.Collections.Generic.Dictionary<,>

Alternative Designs

Some alternate API for rendering a symbol - like fetching a DocID or using SyntaxGenerator.

Risks

@ericstj ericstj added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Apr 16, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 16, 2024
@CyrusNajmabadi
Copy link
Member

@ericstj I'm not quite getting the use case. CAn you flesh out the example more, including where generics are used today, and why the actual type argument values are a problem?

@ericstj
Copy link
Member Author

ericstj commented Apr 17, 2024

Here are two cases:
https://github.com/dotnet/sdk/blob/26854d4fe75c45af1c17e696d98ca4c8b509f170/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Comparing/SymbolEqualityComparer.cs#L40
https://github.com/dotnet/roslyn-analyzers/blob/b07c100bfc66013a8444172d00cfa04c9ceb5a97/src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs#L74

It's actually non-breaking to change the name of a type parameter, however we'd like to make that an optional rule that folks can enable. To do this we'd like to have a symbol representation that omits the generic parameter names, but still represents the arity. That way we can create a key that represents the API without including non-essential information (parameter names). It's a similar scenario to the format of typeof and the docID syntax - which both omit the generic parameter names. If there's an existing API we should be using here I can also investigate changing how we produce the key string.

@CyrusNajmabadi
Copy link
Member

I'd probably prefer emitting as List`1. So something about wanting emitting the rarity instead.

@ericstj
Copy link
Member Author

ericstj commented Apr 17, 2024

I'm not picky on the precise format here. It just needs to represent the number of generic parameters and not their names.

@CyrusNajmabadi
Copy link
Member

I bring it up because compiler has UseArityForGenericTypes as an internal option currently.

@ericstj
Copy link
Member Author

ericstj commented Apr 17, 2024

Maybe we can just use GetDocumentationCommentId. That does format like we'd want. I might give it a try and see if its' reasonable.

@ericstj
Copy link
Member Author

ericstj commented Apr 17, 2024

One downside I see for swapping to different methods for this is that we call it a lot. So much that I've been thinking about adding a cache for these strings. It would be nice if ISymbol would cache some of these. Maybe we should wrap all the ISymbol interfaces and add caching for the methods/properties we call the most.

@ericstj
Copy link
Member Author

ericstj commented Apr 19, 2024

Looks like GetDocumentationCommentId isn't sufficient as this omits return value. I think exposing UseArityForGenericTypes would work for us.

@ericstj
Copy link
Member Author

ericstj commented Apr 19, 2024

Ok I tested out using UseArityForGenericTypes by setting through reflection. This worked well for classes, but not methods. I would also like a similar setting for methods.

@CyrusNajmabadi
Copy link
Member

in cases like this, where you're making a domain-specific key, our recommendation usually becomes: figure out what your domain needs, and extract out of the symbols. We're unlikely to add a feature (like for methods) that nothing else uses. We end up just chasing how the feature works, as opposed to some facility intrinsic to what the language is modeling.

@ericstj
Copy link
Member Author

ericstj commented Apr 19, 2024

I was able to make this work by combining the DocId with the DocID of the type/return-value for members. This gives me what I need.

@ericstj
Copy link
Member Author

ericstj commented Apr 19, 2024

Actually, I see there's a SymbolDisplayParts API, that might be a better way to handle this. I'll experiment with that.

@CyrusNajmabadi
Copy link
Member

And with the SymbolDisplayParts, you can see which are TypeParameter symbols. And you could elide those or transform them into anything you want.

@ericstj
Copy link
Member Author

ericstj commented Apr 19, 2024

Exactly, that's what I'm doing. 👍 I'll close this as I think I have what I need.

@ericstj ericstj closed this as completed Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

No branches or pull requests

2 participants