Add semantic classification for records by allisonchou · Pull Request #47960 · dotnet/roslyn

allisonchou · 2020-09-22T23:28:56Z

Fixes #46985
The default color for records is the default color for classes.

https://devdiv.visualstudio.com/DevDiv/_git/VSLanguageServerClient/pullrequest/276802 will be needed to support record colorization in LSP.

allisonchou · 2020-09-22T23:55:28Z

@CyrusNajmabadi Are there areas missing here? Colorization still doesn't seem to be working properly in some cases.

I feel like this line will have to change to take in a SemanticModel so we can call GetSymbolInfo, but I think that will involve a significant amount of changes, so just want to check that this is the right move beforehand.

CyrusNajmabadi · 2020-09-22T23:59:09Z

src/Compilers/Core/Portable/Symbols/SymbolEqualityComparer.cs

+        /// <summary>
+        /// Compares two <see cref="ISymbol"/> instances, ignoring all options
+        /// </summary>
+        public static readonly SymbolEqualityComparer IgnoreAll = new SymbolEqualityComparer(TypeCompareKind.AllIgnoreOptions);


src/EditorFeatures/CSharpTest/Classification/SemanticClassifierTests.cs

CyrusNajmabadi · 2020-09-23T00:00:50Z

I feel like this line will have to change to take in a SemanticModel so we can call GetSymbolInfo,

Nope: that's syntactic. You can determine what's going on just by checking the SyntaxKind of the containing node. IN this case syntaxKind.RecordDeclaration.

CyrusNajmabadi · 2020-09-23T00:01:53Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitorHelpers.cs

+
+            IMethodSymbol candidate = null;
+
+            // TO-DO: Change hard-coded '<Clone>$' to WellKnownMemberNames.CloneMethodName once it is made public


CyrusNajmabadi · 2020-09-23T00:04:03Z

src/EditorFeatures/Core.Wpf/Classification/ClassificationTypeFormatDefinitions.cs

+                this.DisplayName = EditorFeaturesResources.User_Types_Records;
+                this.ForegroundColor = Color.FromRgb(0x2B, 0x91, 0xAF);
+            }
+        }


i assume this is the color for classes. i"m on the fence if i want a different color here :) might be fun to call out records as the snazzy newnews. @mikadumont for thoughts. We can possibly pick one of the safe colors we found for regexes. that would a different PR though :)

@JoeRobich Do you need to do anything for advanced classification for this?

question still applies. @JoeRobich

@CyrusNajmabadi What do you mean by advanced classification?

whatever this stuff is:

:)

Nope. Since we are inheriting the ClassName color, there is no need to add entries to the ColorScheme files. When we decide a custom color is required we can add the entries at that time.

src/Workspaces/Core/Portable/Classification/ClassificationExtensions.cs

CyrusNajmabadi

small tweaks.

JoeRobich · 2020-09-23T01:04:33Z

One question we may want to ask ourselves is whether we want the Record classification to inherit the Class color if no custom Record color is defined. This is handled by the BaseClassification attribute on the ClassificationTypeDefinition. You then wouldn't have to set a Foreground color in the EditorFormatDefintion for the User - Record Name or define colors in the ColorScheme files, since it would be inherited.

Here are some things that will make this more complete. You can use this earlier PR as a guide - #31231

For classification within the Editor:

Will need entries for each theme in the VisualStudio2017 and VisualStudio2019 ColorScheme files (here).

For classification within QuickInfo and SignatureHelp:

Will need SymbolDisplayPart entry for Record (here)
Will need TextTags entriy for Record (here)
Will need to update mapping between all the different classification, part, tag types (here, here, and here)
Will need to update the SymbolDisplayVisitor and tests (here)

Adding a RecordName to SymbolDisplayPartKind will mean parts of Rolsyn that act on SymbolDisplayPartKind.ClassName will need to be updated such as https://github.com/dotnet/roslyn/blob/master/src/Features/Core/Portable/CodeLens/CodeLensReferencesService.cs#L258

…igure out why parameterized record test isn't working

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

allisonchou · 2020-09-23T21:38:40Z

Marking the PR as ready to review. For now, I've taken Joey's suggestion to make the default record color the same color as classes, although this can change in a follow-up PR if we decide to go with a different color.

…ications2

src/Compilers/Core/Portable/Symbols/SymbolEqualityComparer.cs

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

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitorHelpers.cs

CyrusNajmabadi · 2020-10-19T16:19:41Z

@allisonchou @AlekseyTs here's what i think makes sense:

in VB .IsRecord is never true. VB has no concept of this special sort of C# type, and think so of them entirely as normal classes.
As such, if you take a record from metadata, import in VB, and display using C#'s display logic, you would see that just as class X not record X.
For C#, both for source and metadata we would expect to see .IsRecord be true for the things we consider records :) so obviously record Foo in source would have this be true. If this is compiled to metadata and reimported, it should stay true. The c# symbol display logic for these types would show them a record X.... The VB symbol display logic for this would just be Class X...

For all IDE features, i would want us only to use .IsRecord and never look for things like a special 'Clone' mehtod

@allisonchou is this something you can pick back up?

333fred · 2020-10-19T17:58:59Z

Hmm. I was about to argue that I disagree with 1, but then I realized we don't import function pointer types in VB, we just mark them as unsupported by the language, which is an observable difference between C# compilations and VB compilations. In light of that, I think I'm fine with this behavior, though I do feel like someone is going to report a bug on us for it eventually.

CyrusNajmabadi · 2020-10-19T18:10:02Z

though I do feel like someone is going to report a bug on us for it eventually.

Sure. But a reasoned and consistent position is fine to fall back on. :)

allisonchou · 2020-10-19T21:40:45Z

@CyrusNajmabadi Thanks Cyrus! I've updated the code so that VB .IsRecord always returns false.
It's actually the same code as a previous iteration of this PR, I just had to update the failing test.

allisonchou · 2020-10-19T21:49:12Z

@dotnet/roslyn-compiler After taking Cyrus' suggestion, I believe this PR is now once again ready for review.

allisonchou · 2020-10-21T18:35:46Z

@dotnet/roslyn-compiler Would it be possible to get another look at this PR from the compiler side?

CyrusNajmabadi · 2020-10-22T01:04:44Z

@jcouv @AlekseyTs @333fred can you ptal at this IDE unblocker PR. Thanks!

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

333fred · 2020-10-22T17:59:17Z

We should have some explicit tests using the IsRecord API directly. It's fine to do this by going through some of the tests in RecordTests and adding single asserts for that property, but we definitely want direct tests of the API, not just through SymbolDisplay.

333fred · 2020-10-22T17:59:33Z

Done review pass (commit 20) #Closed

AlekseyTs

Changes under Compilers LGTM (iteration 22).

333fred

Compiler changes LGTM (commit 22)

allisonchou · 2020-10-23T20:09:47Z

Thanks everyone!

Preliminary changes

2a6b746

allisonchou added the Area-IDE label Sep 22, 2020

allisonchou requested a review from CyrusNajmabadi September 22, 2020 23:28

Update another file

a68bb4f

CyrusNajmabadi reviewed Sep 22, 2020

View reviewed changes

src/EditorFeatures/CSharpTest/Classification/SemanticClassifierTests.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 23, 2020

View reviewed changes

src/Workspaces/Core/Portable/Classification/ClassificationExtensions.cs Outdated Show resolved Hide resolved

CyrusNajmabadi suggested changes Sep 23, 2020

View reviewed changes

Partial code review feedback - need to fix up a few more places and f…

ee10771

…igure out why parameterized record test isn't working

jcouv added Area-Compilers Feature - Records Records labels Sep 23, 2020

JoeRobich reviewed Sep 23, 2020

View reviewed changes

src/Compilers/Core/Portable/SymbolDisplay/SymbolDisplayPartKind.cs Show resolved Hide resolved

allisonchou added 3 commits September 23, 2020 13:14

More modifications to support records

2ea4848

Accidentally removed class color instead of record color - reverting

be7c09e

Add quickinfo test for records

bbad8bb

allisonchou marked this pull request as ready for review September 23, 2020 21:39

allisonchou requested review from a team as code owners September 23, 2020 21:39

allisonchou requested a review from a team September 23, 2020 21:39

allisonchou added 2 commits September 23, 2020 14:42

Merge branch 'master' of github.com:dotnet/roslyn into RecordsClassif…

2316f94

…ications2

Add record support in LSP

ce79fb5

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

src/Compilers/Core/Portable/Symbols/SymbolEqualityComparer.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.cs Show resolved Hide resolved

AlekseyTs reviewed Sep 23, 2020

View reviewed changes

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitorHelpers.cs Outdated Show resolved Hide resolved

CyrusNajmabadi mentioned this pull request Oct 19, 2020

Do not override 'Equals(object)' in records. #48767

Merged

VB IsRecord is never true

46740a1

CyrusNajmabadi requested review from 333fred, AlekseyTs and jaredpar October 22, 2020 01:04

333fred reviewed Oct 22, 2020

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs Show resolved Hide resolved

allisonchou added 2 commits October 22, 2020 19:12

Update IsRecord tests

3ad7060

Update IsRecord tests

bb4c3d6

AlekseyTs approved these changes Oct 23, 2020

View reviewed changes

333fred approved these changes Oct 23, 2020

View reviewed changes

Merge branch 'master' into RecordsClassifications2

1a9014d

allisonchou merged commit 2421055 into dotnet:master Oct 23, 2020

ghost added this to the Next milestone Oct 23, 2020

allisonchou deleted the RecordsClassifications2 branch October 23, 2020 20:09

allisonchou mentioned this pull request Oct 23, 2020

Include Record in Razor Legend dotnet/razor#2670

Merged

RikkiGibson mentioned this pull request Oct 26, 2020

Revert "Add semantic classification for records" #48931

Closed

CyrusNajmabadi mentioned this pull request Nov 3, 2020

IDE classification for record constructor is wroing #49147

Closed

Youssef1313 mentioned this pull request Nov 6, 2020

Partial records miss their names in 'Find all references' window #49206

Closed

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020


		IMethodSymbol candidate = null;

		// TO-DO: Change hard-coded '<Clone>$' to WellKnownMemberNames.CloneMethodName once it is made public

Conversation

allisonchou commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allisonchou commented Sep 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Sep 23, 2020

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!

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

JoeRobich commented Sep 23, 2020

Uh oh!

Uh oh!

allisonchou commented Sep 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred commented Oct 19, 2020

Uh oh!

CyrusNajmabadi commented Oct 19, 2020

Uh oh!

allisonchou commented Oct 19, 2020

Uh oh!

allisonchou commented Oct 19, 2020

Uh oh!

allisonchou commented Oct 21, 2020

Uh oh!

CyrusNajmabadi commented Oct 22, 2020

Uh oh!

Uh oh!

333fred commented Oct 22, 2020

Uh oh!

333fred commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

allisonchou commented Oct 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

allisonchou commented Sep 22, 2020 •

edited

Loading

CyrusNajmabadi commented Oct 19, 2020 •

edited

Loading

333fred commented Oct 22, 2020 •

edited

Loading