Added member classification API support for C# by mkrueger · Pull Request #24100 · dotnet/roslyn

mkrueger · 2018-01-08T13:12:12Z

Added classification for c# methods, events, fields and properties.

VB.NET is missing and unit tests. I've atm problems compiling/running the tests on my system. So do not merge it - it's untested.
But that's basically what we need in Visual Studio 4 Mac. It should be a base for discussion about classification enhancements.

esp. for making a difference between properties, events and fields this is useful.

Pilchie

Will need to update PublicAPI.Unshipped.txt, tests, etc.

Pilchie · 2018-01-08T15:51:35Z

src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs

Should we do local/parameter as well, since we're here?

Should const/non-const/enum fields get their own classifications? I personally think so.

Added locals, parameters, const, enum fields.

CyrusNajmabadi · 2018-01-08T16:27:14Z

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

paging @DustinCampbell . I think the primary concern we had here (and why we didn't do this earlier), is that we wanted flexibility to be able to classify things at a veyr fine grained level. For example someone could want a set of classifications for "private static method" vs "public instance method". I was prototyping being able to do this using the SymbolSpecification stuff @dpoeschl introduced for naming. However, the primary blocker was that it would need a revamp of fonts/colors in VS to support this, even if we did have it at the data layer.

@CyrusNajmabadi for context, we wanted to add these to the API so that VS 4 Mac doesn't lose the support they already had, even if VS doesn't make use of it right away.

Classification types are a hierarchy, so this shouldn't block future work to have more detailed classifications.

I think the concern was more that this was part of our public API. So we wanted to get the public API shape right around classification. But i agree that such a desire should not block pretty simple stuff that could be done now. I just want Dustin involved so he's aware of what's going on and doesn't have any strong objections himself.

Thanks @CyrusNajmabadi! Sorry for the delay (I've been OOF sick).

I'm generally OK with this. There are a lot of pivots we could consider here (read-only fields, different colors for different accessibility, etc., etc.), but I don't think that should block this. After all, we'd probably want to introduce more pivots for types as well and that's a similar concern.

CyrusNajmabadi · 2018-01-08T16:28:20Z

src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs

should we have different classification for method vs extension method? I personally think so.

I like the idea to have extension methods in a different classification

CyrusNajmabadi · 2018-01-08T17:18:42Z

Ah, for future recollection: one other issue i had with using SymbolSpecifications for this purpose was that we have customers that want classification that would not even fall under that bucket. For example, being able to classify individual keywords differently. While this sounds like it could be strange, the use cases actually made sense. For example, there were some customers that wanted to classify flow-control keywords differently, to help draw attention to them. Right now, such a concept is not expressible with SymbolSpecification (though it could be augmented to support this in the future).

mkrueger · 2018-01-08T21:50:51Z

In older versions of our syntax definition we had different colors for different keywords - at least there was the possibility to do so.

I removed that feature because no one really used it and made the color styles too colorful - but in any case that's not something that is implemented in the name classification.

DustinCampbell

There are a few changes I'd like to request:

As written, this change will only classify member references. However, to match what we do with type declarations, another change needs to happen to classify member declarations. This is done in the syntactic classifier here: https://github.com/dotnet/roslyn/blob/master/src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs#L106-L146.
Tests!
Consider making the change for VB as well.

mkrueger · 2018-01-24T16:44:40Z

Should be a discussion starting point for what we minimally need. ATM I've issues to build roslyn on my system will take me a while to set up the environment for it.

But ty for the GetClassificationForIdentifier - I'll look what I can do :).

Added classification for c# methods, events, fields and properties.

heejaechang · 2018-01-25T10:10:17Z

@mkrueger can you fix up commits so that it only shows your change?

mkrueger · 2018-01-25T13:30:19Z

Tests still missing - I can't compile roslyn on my machine so this is still only a discussion PR.

If anyone could help me:

The current .NET SDK does not support targeting .NET Core 2.0. Either target .NET Core 1.1 or lower, or use a version of the .NET SDK that supports .NET Core 2.0. CompilersBoundTreeGenerator
C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.TargetFrameworkInference.targets 112

and

MSB3644 Die Verweisassemblys für Framework ".NETFramework,Version=v4.6.1" wurden nicht gefunden. Installieren Sie zum Beheben dieses Problems das SDK oder das Paket zur Festlegung von Zielversionen für die vorliegende Frameworkversion, oder legen Sie als neues Ziel für die Anwendung eine Version des Frameworks fest, für die Sie das SDK oder Paket zur Festlegung der Zielversionen installiert haben. Assemblys werden im globalen Assemblycache (GAC) aufgelöst und anstelle von Verweisassemblys verwendet. Daher wird die Assembly für das gewünschte Framework unter Umständen nicht ordnungsgemäß als Ziel festgelegt. RemoteHostClientMock C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Microsoft.Common.CurrentVersion.targets 1124

I Installed vs.net 2017 enterprise and checked everything in the visual studio installer. 4.6.1 facade assemblies are installed in C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.6.1

My dotnet info is:

dotnet --info
The specified SDK version [2.2.0-preview1-007622] from global.json [C:\Users\mkrue\OneDrive\Dokumente\GitHub\roslyn\global.json] not found; install specified SDK version

Microsoft .NET Core Shared Framework Host

Version : 2.0.5
Build : 17373eb129b3b05aa18ece963f8795d65ef8ea54

Without compiling + testing it's not possible to verify the correctness. However we made some good discussion points. I added enum/constants and extension methods.

jaredpar · 2018-01-25T16:09:58Z

src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs

            return false;
        }

+        static string GetClassificationForField(IFieldSymbol fieldSymbol)


Please use explicit accessibility modifier here.

Added some helper functions for testing the new classification names as well.

The usage test is failing - needs investigation.

mkrueger · 2018-02-05T15:40:01Z

Classification tests should work. The extension method usage test fails for an unknown reason. However it's difficult for me to debug that test - vs.net runs for a very long time not hitting the breakpoint there :/.

All other classifications I added are already covered by various unit tests.

There are some other tests failing - but it looks like it's caused by my system language so I don't know if that is connected to my changes in any way.

Would be nice if someone could help us out in there.

CyrusNajmabadi · 2018-02-07T08:26:01Z

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

+        {
+            private UserTypesFieldsFormatDefinition()
+            {
+                this.DisplayName = EditorFeaturesResources.User_Types_Fields;


This should probably be User_Members_Fields as Fields are not Types.

Hrmm.. although, then these would be sorted separately in teh fonts/colors dialog. So probably best to leave like this.

Y that's what I thought - User_Types is a bit unfortunate but fits 80 - however type in that case is no type in a programming language just a user defined type of things to classify.

CyrusNajmabadi · 2018-02-07T08:27:51Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

@@ -1,5 +1,6 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.

+using System.Linq;


classification is super perf sensitive and is called a bunch. We've avoided linq here because of that.

CyrusNajmabadi · 2018-02-07T08:28:18Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

+            {
+                // TODO: Add proper extension method check - I'm sure there is some syntax based check somewhere burried in roslyn but I couldn't find it.
+                bool isExtensionMethod =
+                    (methodDeclaration.Parent as TypeDeclarationSyntax)?.Modifiers.Any(m => m.IsKind(SyntaxKind.StaticKeyword)) == true &&


SyntaxTokenList already has a .Any overload that just takes a SyntaxKind. So no need to use the lambda version.

All you have to do is check the modifiers of the first parameter. That's sufficient. "this" is only legal on extension methods, so it's fine to assume that if you see 'this' on the first parameter, then ti's an extension method.

Is there any way checking that ? there must be something to check that but I didn't find it.

CyrusNajmabadi · 2018-02-07T08:29:52Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

+                switch (varDecl.Parent)
+                {
+                    case FieldDeclarationSyntax fieldDeclaration:
+                        return fieldDeclaration.Modifiers.Any(m => m.IsKind(SyntaxKind.ConstKeyword)) ? ClassificationTypeNames.ConstantName : ClassificationTypeNames.FieldName;


don't use lambda overload. don't use linq. wrap long line. align ? and : of ternary on next line.

CyrusNajmabadi · 2018-02-07T08:30:19Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

+                    case EventDeclarationSyntax eventDeclarationSyntax:
+                        return ClassificationTypeNames.EventName;
+                }
+                return ClassificationTypeNames.Identifier;


i would have everything fall out and generate .LocalName

CyrusNajmabadi · 2018-02-07T08:31:59Z

src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs

+            {
+                if (fieldSymbol.ContainingType.IsEnumType())
+                    return ClassificationTypeNames.EnumFieldName;
+                return ClassificationTypeNames.ConstantName;


curlies always required. But just replace this with a return ... ? ... : ...

CyrusNajmabadi · 2018-02-07T08:32:29Z

I didn't see any code for VB here.

mkrueger · 2018-02-09T06:58:43Z

I don't plan adding VB - anyways I need help with that change because I don't know enough about the VS.NET side - it surely breaks highlighting there ?

As requested by the PR.

CyrusNajmabadi · 2018-02-09T18:17:09Z

src/Workspaces/CSharp/Portable/Classification/ClassificationHelpers.cs

+                bool isExtensionMethod =
+                    (methodDeclaration.Parent as TypeDeclarationSyntax)?.Modifiers.Any(SyntaxKind.StaticKeyword) == true &&
+                    methodDeclaration.Modifiers.Any(SyntaxKind.StaticKeyword) &&
+                    methodDeclaration.ParameterList.Parameters.FirstOrDefault()?.Modifiers.Any(SyntaxKind.ThisKeyword) == true;


basically, my point is this: just check the parameter. you don't need the other checks as they're superfluous.

Is there an extension method check somewhere ?

not that i know of.

CyrusNajmabadi · 2018-02-09T18:18:01Z

I don't plan adding VB

Someone will need to :) Are you coordinating with @dotnet/roslyn-ide so that it gets these additions as well?

heejaechang · 2018-02-09T20:03:07Z

tagging @jinujoseph

mkrueger · 2018-02-12T06:39:51Z

@dotnet/roslyn-ide - not yet. This PR should be more a discussion starting point what VS4Mac needs and what can be done to improve semantic highlighting. I already got some feedback and added some classifications VS4MAC didn't have.

So we need to look & discuss at things - we won't be able to really change that once it's implemented.

DustinCampbell · 2018-02-12T17:55:56Z

@mkrueger : The failing tests look like they're related to the new classifications.

DustinCampbell · 2018-02-12T18:05:29Z

Since I reviewed this change, it looks like EditorFormatDefinitions have been added. Unless I'm mistaken, this will make the new classification types appear in the Fonts and Colors dialog in Visual Studio, but I believe the intent here was to update the API. If we want to take this PR as is, we need to do some further design work. Calling @kuhlenh!

mkrueger · 2018-02-13T11:01:18Z

The failing tests are caused by the IClassificationTypeRegistryService from VS.NET that doesn't recognize the new tag names.

translation. It may make sense to add new SymbolDisplayPartKinds matching the new classification type names.

DustinCampbell · 2018-02-14T16:10:16Z

Offline: I'll add VB support after this goes in.

DustinCampbell · 2018-02-14T16:11:40Z

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

+                case ClassificationTypeNames.MethodName:
+                case ClassificationTypeNames.PropertyName:
+                case ClassificationTypeNames.EventName:
+                    return SymbolDisplayPartKind.Text;      // TODO: Add more SymbolDisplayPartKinds 


I'll take care of this.

DustinCampbell

Looks good @mkrueger! I'll add VB support in a follow-up PR.

jasonmalinowski · 2018-02-15T22:02:10Z

src/Workspaces/Core/Portable/PublicAPI.Shipped.txt

+const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.LocalName = "local name" -> string
+const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.ParameterName = "parameter name" -> string
+const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.MethodName = "method name" -> string
+const Microsoft.CodeAnalysis.Classification.ClassificationTypeNames.ExtensionMethodName = "extension method name" -> string


Any idea how we would expand this for extension everything?

No idea what you mean with that - I can need the classification type names that get added so I made them public.

jasonmalinowski · 2018-02-15T22:04:44Z

src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/NameSyntaxClassifier.cs

+                    return true;
+                case IParameterSymbol parameterSymbol:
+                    if (parameterSymbol.IsImplicitlyDeclared && parameterSymbol.Name == "value")
+                        break;


break should be surrounded by {}.

jasonmalinowski · 2018-02-15T22:06:44Z

src/EditorFeatures/CSharpTest/Classification/TotalClassifierTests.cs

+                Punctuation.CloseCurly);
+        }
+
+       /* [Fact, Trait(Traits.Feature, Traits.Features.Classification)]


Why the skip here? Can we get a proper Skip attribute applied if there's some tracking bug?

jasonmalinowski · 2018-02-15T22:16:10Z

src/EditorFeatures/Core/Implementation/Classification/ClassificationUtilities.cs

        {
            foreach (var classifiedSpan in list)
            {
+                var classificationType = typeMap.GetClassificationType(classifiedSpan.ClassificationType) ?? typeMap.GetClassificationType(ClassificationTypeNames.Identifier);


Any reason we can't just explicitly check for the types we know we added but didn't define formation definitions for yet? Might be easier than a fallback that means we'll forget to add a format definition, and things magically kinda work?

jasonmalinowski · 2018-02-15T22:17:58Z

Since I reviewed this change, it looks like EditorFormatDefinitions have been added. Unless I'm mistaken, this will make the new classification types appear in the Fonts and Colors dialog in Visual Studio, but I believe the intent here was to update the API. If we want to take this PR as is, we need to do some further design work. Calling @kuhlenh!

EditorFormatDefinition is at least documented as supporting UserVisible(false) as metadata on it, which might mean it's not shown in the UI. That might be cleaner than the "just return identifier" hack, or set us up for betterness in other places.

jasonmalinowski · 2018-02-15T22:18:15Z

@mkrueger @DustinCampbell: is master the branch you want here? If you want this in 15.7 we'll have to rebase it.

DustinCampbell · 2018-02-16T02:34:49Z

We verified over email that 15.7 is what we want. Once @mkrueger rebases onto dev15.7.x and CI passes, I think we're good to merge. Then I'll follow up with some other fixes and VB support. I'm hoping we can get it all wrapped up tomorrow.

mkrueger · 2018-02-19T08:50:42Z

I tidied up the PR a bit and rebased it onto dev15.7.x : #24931

mkrueger requested a review from a team as a code owner January 8, 2018 13:12

Therzok approved these changes Jan 8, 2018

View reviewed changes

Pilchie reviewed Jan 8, 2018

View reviewed changes

CyrusNajmabadi reviewed Jan 8, 2018

View reviewed changes

DustinCampbell requested changes Jan 17, 2018

View reviewed changes

Added member classification support.

67a3957

Added classification for c# methods, events, fields and properties.

mkrueger requested review from a team as code owners January 25, 2018 10:03

Improved classification support.

df12780

jaredpar reviewed Jan 25, 2018

View reviewed changes

mkrueger added 3 commits January 30, 2018 11:16

Added new classification types to the public API.

72471cd

Added some helper functions for testing the new classification names as well.

Fixed classifier tests.

9186df2

Added extension method classification test.

6145d56

The usage test is failing - needs investigation.

CyrusNajmabadi reviewed Feb 7, 2018

View reviewed changes

Fixed linq usage & corrected some formatting issues.

fd07864

As requested by the PR.

CyrusNajmabadi reviewed Feb 9, 2018

View reviewed changes

mkrueger added 3 commits February 13, 2018 13:26

Fixed some classification unit tests.

025178a

Added new classification type names to the SymbolDisplayPartKind

5b077eb

translation. It may make sense to add new SymbolDisplayPartKinds matching the new classification type names.

Removed vs.net integration of the new classifications.

dec1950

DustinCampbell reviewed Feb 14, 2018

View reviewed changes

DustinCampbell changed the title ~~Added member classification support.~~ Added member classification API support for C# Feb 14, 2018

DustinCampbell approved these changes Feb 14, 2018

View reviewed changes

jasonmalinowski approved these changes Feb 15, 2018

View reviewed changes

Applied some requested changes.

411282c

mkrueger closed this Feb 19, 2018

mkrueger mentioned this pull request Feb 20, 2018

Added member classification support for c#. #24931

Merged

		@@ -1,5 +1,6 @@
		// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

		using System.Linq;

Conversation

mkrueger commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

CyrusNajmabadi Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jan 8, 2018

Uh oh!

mkrueger commented Jan 8, 2018

Uh oh!

DustinCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

mkrueger commented Jan 24, 2018

Uh oh!

heejaechang commented Jan 25, 2018

Uh oh!

mkrueger commented Jan 25, 2018 • 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!

mkrueger commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkrueger Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

CyrusNajmabadi commented Feb 7, 2018

Uh oh!

mkrueger commented Feb 9, 2018

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

mkrueger commented Jan 8, 2018 •

edited

Loading

CyrusNajmabadi Jan 8, 2018 •

edited

Loading

CyrusNajmabadi Jan 8, 2018 •

edited

Loading

mkrueger commented Jan 25, 2018 •

edited

Loading

mkrueger Feb 9, 2018 •

edited

Loading