Improve the signature display when a tuple appears many times within it. by CyrusNajmabadi · Pull Request #56025 · dotnet/roslyn

CyrusNajmabadi · 2021-08-31T03:53:59Z

Similar to what we do with anonymous types, we'll now show tuples like so (when they appear at least two times in a signature).

Before:

After:

CyrusNajmabadi · 2021-08-31T03:55:09Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs


+            if (this.format.MiscellaneousOptions.IncludesOption(SymbolDisplayMiscellaneousOptions.DoNotExpandTupleType))
+            {
+                builder.Add(CreatePart(SymbolDisplayPartKind.StructName, symbol, "<tuple>"));


this is similar to what we do with anonymous types. With anon-types, the compiler just makes a single part, allowing the IDE control of how to build the final display string. For tuples it's fine (by default) to expand it as (...), but we need a way to have that not happen in order to use the new display logic.

CyrusNajmabadi · 2021-08-31T03:56:33Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

                {
                    AddSpace();
-                    builder.Add(CreatePart(SymbolDisplayPartKind.FieldName, symbol, element.Name));
+                    builder.Add(CreatePart(SymbolDisplayPartKind.FieldName, element, element.Name));


this was a bug before (we were adding in the tuple again for each tuple element). This caused problems with the IDE code as we'd then try to expand these references out as well.

CyrusNajmabadi · 2021-08-31T03:57:10Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMiscellaneousOptions.cs

+        /// Insert a tuple into the display parts as a single part instead of multiple parts (similar
+        /// to how anonymous types are inserted0.
+        /// </summary>
+        DoNotExpandTupleType = 1 << 9,


open to discussing a good name here. We use the term 'expand' in other places (like 'ExpandNullable'). Maybe DoNotExpandTuples?

CyrusNajmabadi · 2021-08-31T03:57:28Z

src/EditorFeatures/CSharpTest/QuickInfo/SemanticQuickInfoSourceTests.cs

+  {
+    $$M(default);
+  }",
+                MainDescription(@"void C.M((int x, string y) t)"),


in the single use case, we keep the signature as before.

CyrusNajmabadi · 2021-08-31T03:59:01Z

src/Features/CSharp/Portable/LanguageServices/CSharpStructuralTypeDisplayService.cs

-    [ExportLanguageService(typeof(IAnonymousTypeDisplayService), LanguageNames.CSharp), Shared]
-    internal class CSharpAnonymousTypeDisplayService : AbstractAnonymousTypeDisplayService
+    [ExportLanguageService(typeof(IStructuralTypeDisplayService), LanguageNames.CSharp), Shared]
+    internal class CSharpStructuralTypeDisplayService : AbstractStructuralTypeDisplayService


most of the code changes are a strictly mechanical change of 'AnonymousType' => 'StructuralType'. I'll mark which parts of the code are actually changed.

CyrusNajmabadi · 2021-08-31T03:59:52Z

...Portable/LanguageServices/AnonymousTypeDisplayService/AbstractAnonymousTypeDisplayService.cs

@@ -1,176 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.


this type was not deleted. but enough changed happened to it that github considers it a rewrite.

CyrusNajmabadi · 2021-08-31T04:00:02Z

...ousTypeDisplayService/AbstractStructuralTypeDisplayService.StructuralTypeCollectorVisitor.cs

+                    orderAndCount.count++;
+                    _namedTypes[symbol] = orderAndCount;
+                    return;
+                }


this is new logic.

CyrusNajmabadi · 2021-08-31T04:03:40Z

...ortable/LanguageServices/AnonymousTypeDisplayService/AbstractStructuralTypeDisplayService.cs

+                else
+                {
+                    typeParts.AddRange(GetTypeParts(structuralType, semanticModel, position));
+                }


this part is new.

CyrusNajmabadi · 2021-08-31T04:04:01Z

...ortable/LanguageServices/AnonymousTypeDisplayService/AbstractStructuralTypeDisplayService.cs

+
+        private static ImmutableArray<INamedTypeSymbol> GetTransitiveStructuralTypeReferences(
+            ImmutableArray<INamedTypeSymbol> structuralTypes)
+        {


this method has been rewritten.

CyrusNajmabadi · 2021-08-31T04:04:32Z

...ures/Core/Portable/LanguageServices/AnonymousTypeDisplayService/StructuralTypeDisplayInfo.cs

+            IDictionary<INamedTypeSymbol, string> structuralTypeToName,
+            SemanticModel semanticModel,
+            int position)
+        {


this method was rewritten.

CyrusNajmabadi · 2021-08-31T04:05:32Z

...ortable/LanguageServices/SymbolDisplayService/AbstractSymbolDisplayService.AnonymousTypes.cs

-                        info.AnonymousTypesParts);
+                    AddToGroup(SymbolDescriptionGroups.StructuralTypes, info.TypesParts);

-restart:


this loop moved into StructuralTypeDisplayInfo.ReplaceStructuralTypes

davidwengier · 2021-08-31T06:26:19Z

I was expecting this to be a new section on the tool tip. Did it get too long with a new section, or do you just consider them the same? And if so, how much is that your compiler-dev brain at work? :)

"Structural types" doesn't have much meaning to me, though I'm not exactly against it, as it I can work out what it means. Mainly just surprised, especially if adding delegate info is going to give us a new section here anyway.

davidwengier

Another random thought, is it worth using 't for tuples to distinguish them?

davidwengier · 2021-08-31T06:31:12Z

...ortable/LanguageServices/AnonymousTypeDisplayService/AbstractStructuralTypeDisplayService.cs

+            {
+                if (namedType.IsTupleType && count == 1)
+                {
+                    // Ignore tuples only referenced once.  We'll keep them inline.  If the tuple shows up multiple times,


I wonder if this should only apply if there isn't otherwise going to be a "Structural Types" section? eg, what happens in this case:

public (int a, int b) Goo((int a, int b), (string x, string y);

You're going to end up with a "Structural Types" section that doesn't list out all of the structural types.

That's an interesting point. I could revise to make it that once we have at least one duplicated tuple that all tuples then are broken out... That seems reasonable to me.

CyrusNajmabadi · 2021-08-31T06:47:11Z

Did it get too long with a new section

I just didn't think this warranted a new section. To me each section is very distinct in it's purpose. If we added a new section it would effectively be done the exact same as the existing anonymous type section.

Because of that, I just washed one section that showed you all the type expansions in a single place.

CyrusNajmabadi · 2021-08-31T06:47:56Z

especially if adding delegate info is going to give us a new section here anyway.

I would definitely put delegates into this section, and I had that in mind when writing this :-)

CyrusNajmabadi · 2021-08-31T06:48:44Z

Structural types" doesn't have much meaning to me, though I'm not exactly against it,

You're right. I think that can just simplify to Types: and remain just as clear.

CyrusNajmabadi · 2021-08-31T06:51:01Z

Another random thought, is it worth using 't for tuples to distinguish them?

Possibly. Are you thinking 't1, 't2, etc ?

davidwengier · 2021-08-31T07:22:25Z

Another random thought, is it worth using 't for tuples to distinguish them?

Possibly. Are you thinking 't1, 't2, etc ?

Nevermind, ignore me. I always assumed the "a" in 'a stood for "anonymous".. just tried with two and it turns out its just alphabetical.. this is fine :)

333fred · 2021-08-31T14:34:17Z

@CyrusNajmabadi I don't think this requires a full session, but can you please send an email to the api design alias with the proposal?

CyrusNajmabadi · 2021-08-31T18:13:20Z

On it.

akhera99 · 2021-08-31T18:42:55Z

Fixes: #29949 #53061 #33609

CyrusNajmabadi · 2021-08-31T19:29:47Z

@333fred renamed to CollapseTupleTypes

CyrusNajmabadi · 2021-08-31T19:44:58Z

@dotnet/roslyn-compiler for small public API change to enhance SymbolDisplay.

CyrusNajmabadi · 2021-09-01T19:07:22Z

@dotnet/roslyn-compiler @333fred for eyes on the SymbolDisplay change.

… multiple times

CyrusNajmabadi · 2021-09-01T22:02:40Z

Rebasing to 17.1

333fred

Symbol display change LGTM

CyrusNajmabadi · 2021-09-09T02:41:33Z

@jcouv can you ptal?

cston · 2021-09-09T03:23:35Z

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayMiscellaneousOptions.cs

+        /// Insert a tuple into the display parts as a single part instead of multiple parts (similar
+        /// to how anonymous types are inserted).
+        /// </summary>
+        CollapseTupleTypes = 1 << 9,


CollapseTupleTypes

I'm curious why special-case tuples and anonymous types. Wouldn't similar behavior be useful for other type arguments?

Primarily because they are structural types that require a lot more than a name to reference. For example, when you have a nominal type, it doesn't matter how many members that type has, you just reference it by name. Hit with a structural type you have to include that structure in somehow.

If we have more cases like that in the future, I'd expect we go down a similar route.

when you have a nominal type, it doesn't matter how many members that type has, you just reference it by name

But the nominal type could be arbitrarily complicated, for instance Dictionary<string, object>.

Potentially. We haven't had that arise as significant enough to need attention. If we did though, I'd prefer that as a separate flag.

jcouv

Compiler change LGTM Thanks (iteration 34). I take Fred's sign-off as evidence of sufficient public API discussion.

CyrusNajmabadi requested review from a team as code owners August 31, 2021 03:53

ghost added the Area-IDE label Aug 31, 2021

CyrusNajmabadi commented Aug 31, 2021

View reviewed changes

davidwengier approved these changes Aug 31, 2021

View reviewed changes

akhera99 linked an issue Aug 31, 2021 that may be closed by this pull request

VS Intellisense, generic parameters, and value-tuples #29949

Closed

CyrusNajmabadi added 2 commits September 1, 2021 15:01

rename interface

8467974

rename file

f40a011

CyrusNajmabadi and others added 14 commits September 1, 2021 15:01

Tuple display

5d95678

Revert

8be4e37

Add tuples to the structural display section

c6cd3eb

addtests

b48f9b0

Add vb tests

11102fe

Add ocmment

20c9a8c

renames

2e40e29

Simplify

83652ca

Add more tests

470ab3a

ensure this works recursively with anonymous types

5fe5d55

Change option name

8a85ade

Always use the TYpe display if htere is at least one tuple referenced…

02595aa

… multiple times

Add test

c9d9cc1

Add VB test

8e663dd

CyrusNajmabadi force-pushed the tupleDisplay branch from 8e58534 to 8e663dd Compare September 1, 2021 22:01

CyrusNajmabadi requested a review from a team as a code owner September 1, 2021 22:01

CyrusNajmabadi requested a review from a team September 1, 2021 22:01

CyrusNajmabadi changed the base branch from main to release/dev17.1-preview1 September 1, 2021 22:02

333fred approved these changes Sep 2, 2021

View reviewed changes

cston reviewed Sep 9, 2021

View reviewed changes

jcouv approved these changes Sep 9, 2021

View reviewed changes

CyrusNajmabadi merged commit ff43d2f into dotnet:release/dev17.1-preview1 Sep 9, 2021

CyrusNajmabadi deleted the tupleDisplay branch September 9, 2021 16:01

Youssef1313 mentioned this pull request Apr 10, 2022

Add new line before 'where' constraints in Quick Info #60545

Merged

Neme12 mentioned this pull request May 14, 2022

Quick Info shows "Types:" section even when it's empty #61320

Closed

		@@ -1,176 +0,0 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

CyrusNajmabadi commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier commented Aug 31, 2021

Uh oh!

davidwengier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

davidwengier commented Aug 31, 2021

Uh oh!

333fred commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

akhera99 commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Aug 31, 2021

Uh oh!

CyrusNajmabadi commented Sep 1, 2021

Uh oh!

CyrusNajmabadi commented Sep 1, 2021

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

CyrusNajmabadi commented Aug 31, 2021 •

edited

Loading

333fred commented Aug 31, 2021 •

edited

Loading