Add feature to strike out obsolete symbols #72156

sharwell · 2024-02-16T22:53:01Z

This feature should also work for LSP completion in VS Code. Completion inside Visual Studio does not support deprecated indicators, so there is no change to completion in Visual Studio at this time.

Related to #26488

CyrusNajmabadi · 2024-02-21T18:01:06Z

Could you verify the vscode case? it would be good to make sure it's working there (at least visually) :)

src/EditorFeatures/CSharpTest/CommonMemberData.cs

sharwell · 2024-02-21T18:05:25Z

Could you verify the vscode case? it would be good to make sure it's working there (at least visually) :)

I have no idea how to do this.

@dibarbet ?

CyrusNajmabadi · 2024-02-21T18:07:05Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs

+                else if (classificationType == ClassificationTypeNames.ObsoleteSymbol)
+                {
+                    // 6. Token modifiers - each set bit will be looked up in SemanticTokensLegend.tokenModifiers
+                    modifierBits |= TokenModifiers.ObsoleteSymbol;


do we have lsp specific tests to validate the data is at least what we expect?

We have TestCompletionForObsoleteSymbol added in this PR, which may cover what you are looking for?

CyrusNajmabadi · 2024-02-21T18:08:53Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+
+internal abstract class AbstractObsoleteSymbolService(int? dimKeywordKind) : IObsoleteSymbolService
+{
+    private readonly int? _dimKeywordKind = dimKeywordKind;


this is odd enough that i'd like docs :)

➡️ Now documented

CyrusNajmabadi · 2024-02-21T18:09:30Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+        var compilation = await document.Project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
+        var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+
+        using var _2 = ArrayBuilder<TextSpan>.GetInstance(out var result);


nit: obsolete is likely so rare that usign TempArray is likely ok :)

➡️ Kept ArrayBuilder<T> so I didn't need to reimplement RemoveDuplicates, but rewrote the code to avoid taking the builder from the pool in the overwhelming majority case where no obsolete symbols are used in the classification region.

CyrusNajmabadi · 2024-02-21T18:11:14Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+                        {
+                            if (trivia.HasStructure)
+                            {
+                                stack.Add(trivia.GetStructure()!);


interesting. we classify with strikethrough in doc comments? if so, nifty. are there tests?

if so, nifty. are there tests?

➡️ Yes, and yes (included in TestDeclarationAndUseOfObsoleteAlias and TestGenerics for both languages).

CyrusNajmabadi · 2024-02-21T18:11:29Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+                            }
+                        }
+
+                        foreach (var trivia in token.TrailingTrivia)


nit. extract helper fort ehse loops. so you can just call it both with Leading/TrailingTrivia

➡️ Extracted helper

dibarbet

Could you verify the vscode case? it would be good to make sure it's working there (at least visually) :)

I have no idea how to do this.

@dibarbet ?

@sharwell see https://github.com/dotnet/vscode-csharp/blob/main/CONTRIBUTING.md#configuring-roslyn-language-server, should be as simple as setting dotnet.server.path in vscode to the locally built roslyn server in artifacts/bin, then reloading the window.

Since I already have it, I tested this out. Its struck through in completion, but not in classification. I commented on the semantic tokens code for why its not working, but either it needs to use the deprecated modifier from the LSP spec, or we have to add custom client side configuration to handle the ObsoleteSymbol modifier.

dibarbet · 2024-02-21T19:16:48Z

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/TokenModifiers.cs

@@ -16,5 +16,6 @@ internal enum TokenModifiers
        None = 0,
        Static = 1,
        ReassignedVariable = 2,
+        ObsoleteSymbol = 4,


This doesn't appear to be a known LSP modifier - https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#semanticTokenModifiers

This probably needs to use the existing token modifier (deprecated), or we have to add custom client side support for the modifier
See https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#custom-token-types-and-modifiers

➡️ Now updated to use the deprecated modifier

CyrusNajmabadi · 2024-02-21T20:31:20Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+                        result.Add(aliasToken.Span);
+                }
+
+                return aliasToken;


Cleaner (i think) is that you should be able to do GetDeclaredSymbol on the node to get the IAliasSymbol, and then just check the Target of that symbol. this def should work for C#... and should be good for VB i think.

I wasn't quite sure what the suggestion here was.

CyrusNajmabadi · 2024-02-21T20:33:41Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+                if (!creationKeyword.Span.IsEmpty)
+                {
+                    var symbol = semanticModel.GetSymbolInfo(node, cancellationToken).Symbol;
+                    if (IsSymbolObsolete(symbol) || IsSymbolObsolete(symbol?.ContainingType))


maybe doc. up to you. but i do think it would be good to potentially say "the symbol should be constructor symbol for the type, but we want to strike it out if either the constructor or the type is obsolete".

➡️ Docs added, and slightly tweaked the behavior

CyrusNajmabadi · 2024-02-21T20:35:58Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+    protected static bool IsSymbolObsolete([NotNullWhen(true)] ISymbol? symbol)
+    {
+        // Avoid infinite recursion. Iteration limit chosen arbitrarily; cases are generally expected to complete on
+        // the first iteration or fail completely.


nit. i do not think this is actually possible. first, an alias can't target other aliases. so in the alias case, we unwrap immediately to namespace or type. and, for named types, the compiler never generates infinite types. importantly, many many many other 'symbol walkers' would outright break in many other places if that happened.

➡️ I'm not worried about valid code here, but rather for invalid code. e.g. something like a cycle in using aliases.

so aliases cannot refer to each other in our symbol model. An alias only every points to a single namespace or type. And neither of those two can ever point to an alias. Aliases are actually kind odd, and really shouldn't necessarily be a symbol in the first place (you can't get them with GetSymbolInfo for example, you can only get them with GetAliasInfo). So you should think of them more like a single pointer that points into the real symbol model, and the real symbol model never points back at them.

So loops in them aren't possible if you follow the above guidelines (like using GetDeclaredSymbol(UsingDirective) to get the alias in the first place, and then only looking at its target.

CyrusNajmabadi · 2024-02-21T20:37:03Z

src/Workspaces/Core/Portable/ObsoleteSymbol/AbstractObsoleteSymbolService.cs

+                continue;
+            }
+
+            if (symbol is INamedTypeSymbol { OriginalDefinition.SpecialType: SpecialType.System_Nullable_T, TypeArguments: [var valueType] })


nit. go whole hog and have a SymbolVisitor for this. I think we should handle things like arrays or tuples for this as well.

Basically, if the type signature (Which is not hard to traverse) has any types that are obsolete, it's fine to mark. I would not just limt this to nullable.

I thought about it, but decided against it in the end. I'll make sure there are tests added that indicate that they are simply reflecting current behavior and not necessarily one right or wrong answer.

⏱️ Still need to add tests for arrays and tuples

CyrusNajmabadi · 2024-02-21T20:37:51Z

...paces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SyntaxFacts/CSharpSyntaxFacts.cs

+        public void GetPartsOfImplicitObjectCreationExpression(SyntaxNode node, out SyntaxToken keyword, out SyntaxNode argumentList, out SyntaxNode? initializer)
+        {
+            var implicitObjectCreationExpression = (ImplicitObjectCreationExpressionSyntax)node;
+            keyword = implicitObjectCreationExpression.NewKeyword;


nit. this is only a c# case, and we know in c# it's always the first token. so could literally just hardcode taht at the use site. But i don't feel strongly. if you want to do it this way, that's fine with me as well :)

➡️ I think the use site doesn't have access to C# syntax, so I'll keep this one. To me it worked since it matches the existing GetPartsOfObjectCreationExpression method.

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb

CyrusNajmabadi · 2024-02-21T20:44:19Z

src/Workspaces/VisualBasic/Portable/ObsoleteSymbol/VisualBasicObsoleteSymbolService.vb

+                Return
+            End If
+
+            If localDeclaration.Declarators.Count <> 1 Then


nit. i don't see why we can't do this per declarator. just striking out the identifier if it has an obsolete type seems like it would work.

We could, but there are some cases I wasn't sure about:

If there are more than one declarator, are all declarators required to have the same type?

If there are more than one item in Names, are all locals declared by these names required to have the same type?

Since the overwhelming majority of cases involve one declarator with one name, it seemed like there are diminishing returns to handling the other cases. Happy to add them though if it will be valuable.

If there are more than one declarator, are all declarators required to have the same type?

No. This is legal:

Dim x As String, y As Integer

If there are more than one item in Names, are all locals declared by these names required to have the same type?

Yes. All would have the same type.

Note: this is why i think it's better to just strikeout the name of the thing, vs the 'dim'.

Since the overwhelming majority of cases involve one declarator with one name, it seemed like there are diminishing returns to handling the other cases. Happy to add them though if it will be valuable.

I don't disagree. You are certainly covering the 99.9% case. But it seemed actually trivial to just walk the declarators, and walk the names, and then strike them out if obsolete.

The major reason i want this, tbh, is that i don't like the special-casing of hte dim kind and all that jazz. It seems simpler, and 100% encompassing to just hit the variables themselves and mark those.

--

Note: this is a NIT. It was more curiosity and suggestion than anything else. If you want to keep as is, no problem at all.

➡️ Added TestDeclarators specifically for this, but did not alter the current behavior

The major reason i want this, tbh, is that i don't like the special-casing of hte dim kind and all that jazz. It seems simpler, and 100% encompassing to just hit the variables themselves and mark those.

The main reason I kept the current behavior is for parity with var in C#. Since var is an identifier token, I didn't need to do anything special for it to work in C#.

CyrusNajmabadi · 2024-02-21T20:44:58Z

overall this is fantastic. only very minor nits and suggestions.

davidwengier · 2024-02-21T21:51:02Z

...res/LanguageServer/Protocol/Handler/Completion/AbstractLspCompletionResultCreationService.cs

+                        {
+                            var preferredTag = completionItemTags[i];
+                            if (supportedClientTags.Contains(preferredTag))
+                                lspTag = preferredTag;


Shouldn't the add be moved here in some way? Unless I'm reading it wrong, given tags on an item of "class", "foo" and server supported tags of "class", "struct", this method would return "class", "class".

➡️ For the case you describe, this method would return "class", "foo". Server-supported tags only influence this method if one Roslyn tag maps to more than one LSP tag. In the current definition of RoslynTagToCompletionItemTags, there are no such tags. Support for one-to-many mapping with server-defined precedence matches what we were already using for RoslynTagToCompletionItemKinds.

src/Features/LanguageServer/Protocol/Handler/SemanticTokens/TokenModifiers.cs

dibarbet · 2024-02-23T00:31:30Z

@sharwell just tested it out again - mostly works, just needs some work to get the option to work, see this commit for Roslyn - 39006c0

And on the client side it needs to add the option (and configure the strikethrough behavior) - dotnet/vscode-csharp#6915

But I wanted to ask - is there any reason to not default the option value to true? At least on the vscode side there's no real point in having an additional server side option for this, users can already configure strikethrough behavior pretty easily in their settings, e.g.

"editor.semanticTokenColorCustomizations": {
        "enabled": true, // enable for all themes
        "rules": {
            "*.deprecated": {
                "strikethrough": true
            }
        }
    }

If the option was defaulted in roslyn to true, we wouldn't need to expose directly it on the vscode side (and just let theme configuration handle it)

Pic:

sharwell · 2024-03-26T14:24:49Z

@dibarbet I enabled the feature by default

dibarbet

LSP side changes lgtm, I'll handle updating our default themes on the vscode-csharp side

See dotnet#26488

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 16, 2024

sharwell changed the title ~~Strike out obsolete symbols~~ Add feature to strike out obsolete symbols Feb 21, 2024

sharwell marked this pull request as ready for review February 21, 2024 16:07

sharwell requested a review from a team as a code owner February 21, 2024 16:07

CyrusNajmabadi reviewed Feb 21, 2024

View reviewed changes

src/EditorFeatures/CSharpTest/CommonMemberData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 21, 2024

View reviewed changes

src/EditorFeatures/CSharpTest/CommonMemberData.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 21, 2024

View reviewed changes

dibarbet reviewed Feb 21, 2024

View reviewed changes

CyrusNajmabadi reviewed Feb 21, 2024

View reviewed changes

...edUtilitiesAndExtensions/Compiler/VisualBasic/Services/SyntaxFacts/VisualBasicSyntaxFacts.vb Show resolved Hide resolved

CyrusNajmabadi reviewed Feb 21, 2024

View reviewed changes

davidwengier mentioned this pull request Feb 21, 2024

Split modifiers and token types into their own classes dotnet/razor#9927

Merged

davidwengier reviewed Feb 21, 2024

View reviewed changes

dibarbet mentioned this pull request Feb 23, 2024

Add support for obsolete classifications dotnet/vscode-csharp#6915

Closed

dibarbet approved these changes Mar 26, 2024

View reviewed changes

Add feature to strike out obsolete symbols

f7a47ad

Pass Deprecated tag through to LSP completion

c448f16

See dotnet#26488

sharwell force-pushed the obsolete branch from 4550b24 to c448f16 Compare March 27, 2024 13:31

sharwell enabled auto-merge March 27, 2024 13:31

sharwell merged commit e5ed581 into dotnet:main Mar 27, 2024
27 checks passed

dotnet-policy-service bot added this to the Next milestone Mar 27, 2024

dotnet-bot mentioned this pull request Apr 9, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-34808.241 #72953

Closed

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

Add feature to strike out obsolete symbols #72156

Add feature to strike out obsolete symbols #72156

Conversation

sharwell commented Feb 16, 2024 • edited Loading

CyrusNajmabadi commented Feb 21, 2024

sharwell commented Feb 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

sharwell Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 21, 2024

Choose a reason for hiding this comment

sharwell Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

dibarbet commented Feb 23, 2024 • edited Loading

sharwell commented Mar 26, 2024

dibarbet left a comment

Choose a reason for hiding this comment

sharwell commented Feb 16, 2024 •

edited

Loading

sharwell Feb 21, 2024 •

edited

Loading

sharwell Feb 21, 2024 •

edited

Loading

sharwell Feb 21, 2024 •

edited

Loading

sharwell Feb 21, 2024 •

edited

Loading

CyrusNajmabadi Feb 21, 2024 •

edited

Loading

sharwell Feb 22, 2024 •

edited

Loading

sharwell Feb 22, 2024 •

edited

Loading

dibarbet commented Feb 23, 2024 •

edited

Loading