Introduce a DiscardSyntaxClassifier by lameox · Pull Request #40396 · dotnet/roslyn

lameox · 2019-12-14T19:56:32Z

This is my first contribution so please let me know if there is anything else needed apart from the exported highlighter or if there is a better place in the codebase to introduce the changes.

dnfclas · 2019-12-14T19:56:45Z

All CLA requirements met.

CyrusNajmabadi · 2019-12-15T19:19:08Z

hi @lameox thanks for the contribution!

CyrusNajmabadi · 2019-12-15T19:21:07Z

+            }
+
+
+            return SpecializedCollections.EmptyEnumerable<TextSpan>();


should be impossible to hit this. we should throw here.

can use a switch expression here as well.

Since this is argument syntax (as opposed to parameter syntax), wouldn't this be hit for most arguments?

Yeah i think so. This is why I originally used AbstractKeywordHighlighter instead of AbstractKeywordHighlighter<ArgumentSyntax> and overwrote IsHighlightableNode so GetHighlightsForNode only got called for the required types of nodes. However it was suggested I should use AbstractKeywordHighlighter<ArgumentSyntax> which unfortunately seals IsHighlightableNode. Therefore this code always executes and I need to return the empty enumerable.

I'm not seeing how this woudl be subtantively different. Both cases need to check all nodes to see if they should run against them.

CyrusNajmabadi · 2019-12-15T19:22:07Z

+namespace Microsoft.CodeAnalysis.Editor.CSharp.Highlighting.KeywordHighlighters
+{
+    [ExportHighlighter(LanguageNames.CSharp)]
+    internal class DiscardParameterHighlighter : AbstractKeywordHighlighter


ther's a different baseclass you can subclass right? where you can specify through a type argument that you are constrained to ArgumentSyntax right?

yes, it's AbstractKeywordHighlighter<ArgumentSyntax>.

CyrusNajmabadi

Can be simplified

CyrusNajmabadi · 2019-12-15T21:23:30Z

+            {
+                var syntax = (IdentifierNameSyntax)node.Expression;
+
+                if (syntax.Identifier.Text == "_")


Switch to typechecks

CyrusNajmabadi

Lgtm

CyrusNajmabadi · 2019-12-15T22:34:28Z

@jinujoseph can we get a buddy for this community pr? Thanks!

sharwell · 2019-12-16T22:44:12Z

+            if (node.Expression is IdentifierNameSyntax nameSyntax
+                && nameSyntax.Identifier.Text == "_")


💡 Can use pattern matching here:

if (node.Expression is IdentifierNameSyntax { Identifier: { Text: "_" } } nameSyntax)

I still need to get used to pattern matching tbh. Should all new code aim to make use of pattern matching or is it up to preference?

It's up to preference. in some places it works nicely (like here), in others, it can feel more forced/unpleasant.

sharwell · 2019-12-16T22:47:30Z

+            }
+
+
+            return SpecializedCollections.EmptyEnumerable<TextSpan>();


Since this is argument syntax (as opposed to parameter syntax), wouldn't this be hit for most arguments?

sharwell · 2019-12-16T22:49:05Z

+{
+    void Method()
+    {
+        int i = int.TryParse("""", out var {|Cursor:[|_|]|});


💡 Should add a test showing that this does not highlight the argument if you write it as @_. This token has the same ValueText as _, but different Text, and in this case the difference is meaningful.

Added tests covering that case.

Added a test to ensure @_ is not highlighted incorrectly.

…d of DiscardParameterHighlighter

… argument

JoeRobich · 2019-12-16T23:36:47Z

Hi @lameox,

Thanks for taking time to work on this.

I am not sure that a highlighter is the proper way to go about this. The Keyword highlighters are intended for providing extra information about Keywords under the cursor. For instance when the cursor is on an If Keyword the Else and Else If keywords will be highlighted to provided extra context about the If keyword.

The original issue was with the classification of the discard keyword in code like int.TryParse("1", out _). These cases should be identifiable though during a syntax classification pass, since as you've worked out, it is identifiable as an out argument syntax whose expression is an identifier named '_'.
Our syntactic classification occurs in src\Workspaces\CSharp\Portable\Classification\ClassificationHelpers.cs. Since this is an identifier token being classified, I think a new case should be added to the GetClassificationForIdentifier method to handle this pattern. I think tests for this change should be added in SyntacticClassifierTests.cs and also ensure that a test in TotalClassifierTests.cs handles this pattern.

-Joey

lameox · 2019-12-16T23:48:46Z

Hey there,

thank you for your feedback. I am really new to the codebase and therefore not yet familiar with where to do certain changes. Since it is 1 am where i live i will try to look into moving this fix to the correct place tomorrow and update this PR accordingly. I am sorry if i caused some wasted time to the people helping me so far. Are there any resources I can use to locate where in the codebase to do changes? I searched through the existing unit tests for this but seem to have arrived at the wrong place :(

Thank's for your time
-Jan

CyrusNajmabadi

c

…he pattern match

JoeRobich

LGTM

Can you add a test for _ = int.Parse("");?

lameox · 2020-01-15T20:10:14Z

@JoeRobich Sure thing. This uncovered the missing case for VariableDeclaratorSyntax. From what i can tell i don't actually need symbols for those and can just check the Identifier for _.

Another thing i was kind of suprised about was the test for

 _ = int.Parse("");

In this case we get the following symbol classifications:

Keyword "_"
Static "Parse"
Method "Parse"

This is in contrast to all other tests where the order of the Static "Parse" and Method "Parse" is reversed. For now i just reversed the expected order in the test but this seems kind of fishy to me. Should i keep it like that or investigate further?

JoeRobich · 2020-01-15T20:14:28Z

Should i keep it like that or investigate further?

@lameox Leave it as is. We are likely to make some changes with regards to the Static classification in 16.6.

…tion/DiscardSyntaxClassifier.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

…n't discards from the compilers view. Kept the tests around and tweaked them so they now check that nothing is classified in these cases.

lameox · 2020-01-16T16:57:26Z

Close and reopen to trigger retest

JoeRobich

Thanks @lameox!

…rTests.cs Co-Authored-By: Joey Robichaud <joseph.robichaud@microsoft.com>

jnm2 · 2026-09-09T06:33:15Z

+        [Fact, Trait(Traits.Feature, Traits.Features.Classification)]
+        public async Task DiscardsInLambda()
+        {
+            await TestAsync(@"
+class X
+{
+    void N()
+    {
+        System.Func<int, int, int> a = (int _, int _) => 0;
+    }
+}",
+            Namespace("System"),
+            Delegate("Func"),
+            Keyword("_"),
+            Keyword("_"));
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.Classification)]
+        public async Task DiscardsInLambdaWithInferredType()
+        {
+            await TestAsync(@"
+class X
+{
+    void N()
+    {
+        System.Func<int, int, int> a = (_, _) => 0;
+    }
+}",
+            Namespace("System"),
+            Delegate("Func"),
+            Keyword("_"),
+            Keyword("_"));
+        }


These tests did not actually show success. The syntactic pass applied ParameterName in addition to the semantic pass adding Keyword, and the merge step prioritizes ParameterName over Keyword. The tests needed to be written past the merge step.

Fixed in #85225

…cally, resolving classification conflict (#85225) Fixes #51553. Roslyn classifies discards as keywords for syntax highlighting which shows whether or not `_` is referring to a symbol. <img width="329" height="230" alt="image" src="https://github.com/user-attachments/assets/3364de33-039c-451a-94e5-25cc2f091731" /> However, it's not working for lambda parameter discards: <img width="394" height="49" alt="image" src="https://github.com/user-attachments/assets/60c81709-2e7d-45ce-9821-f3830cea834a" /> There was an attempt to implement it in #40396 for lambda parameter discards: https://github.com/dotnet/roslyn/blob/6de0973c513f7d3940cc0b52ff7cd7d55b869982/src/Workspaces/CSharp/Portable/Classification/SyntaxClassification/DiscardSyntaxClassifier.cs#L36-L44 But this was insufficient because it was implemented in the semantic classifier. The syntactic classifier already classified it as ParameterName, and the two are merged with ordering rules which cause ParameterName to win over Keyword. https://github.com/dotnet/roslyn/blob/6de0973c513f7d3940cc0b52ff7cd7d55b869982/src/EditorFeatures/Core/Classification/ClassificationTypeFormatDefinitions.cs#L520-L523 This PR fixes the issue by implementing the proper classification in the syntactic classifier instead of the semantic classifier. This is preferable because the semantic classifier may run after more of a delay, so this is less visual flicker in the IDE. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/85225)

Introduce a DiscardParameterHighlighter which fixes dotnet#39768

543803d

lameox requested a review from a team as a code owner December 14, 2019 19:56

jinujoseph assigned sharwell Dec 15, 2019

jinujoseph added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Dec 15, 2019