-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Introduce a DiscardSyntaxClassifier #40396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
543803d
31feeb8
e6d2199
2bed547
8bd63d9
d23ec64
8786800
3552b39
e71d97c
03b7756
d366c3c
8e93d34
bf47f1b
74d8060
3a493ea
8b77049
16ba315
d498eb8
25900e9
3bed577
7bdbfb9
5ab4a04
b948f05
e914514
2122217
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3612,5 +3612,231 @@ void M<T>() where T : notnull { } | |
| TypeParameter("T"), | ||
| Keyword("notnull")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task NonDiscardVariableDeclaration() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| var _ = int.Parse(""""); | ||
| } | ||
| }", | ||
| Keyword("var"), | ||
| Static("Parse"), | ||
| Method("Parse")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task NonDiscardVariableDeclarationMultipleDeclarators() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| int i = 1, _ = 1; | ||
| int _ = 2, j = 1; | ||
| } | ||
| }"); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardAssignment() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| _ = int.Parse(""""); | ||
|
lameox marked this conversation as resolved.
|
||
| } | ||
| }", | ||
| Keyword("_"), | ||
| Static("Parse"), | ||
| Method("Parse")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInOutDeclaration() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| int.TryParse("""", out var _); | ||
| } | ||
| }", | ||
| Method("TryParse"), | ||
| Static("TryParse"), | ||
| Keyword("var"), | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInOutAssignment() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| int.TryParse("""", out _); | ||
| } | ||
| }", | ||
| Method("TryParse"), | ||
| Static("TryParse"), | ||
| Keyword("_")); | ||
| } | ||
|
|
||
|
lameox marked this conversation as resolved.
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInDeconstructionAssignment() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| (x, _) = (0, 0); | ||
| } | ||
| }", | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInDeconstructionDeclaration() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| (int x, int _) = (0, 0); | ||
| } | ||
| }", | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInPatternMatch() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| bool N(object x) | ||
| { | ||
| return x is int _; | ||
| } | ||
| }", | ||
| Parameter("x"), | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInSwitch() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| bool N(object x) | ||
| { | ||
| switch(x) | ||
| { | ||
| case int _: | ||
| return true; | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
| }", | ||
| Parameter("x"), | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task DiscardInSwitchPatternMatch() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| bool N(object x) | ||
| { | ||
| return x switch | ||
| { | ||
| _ => return true; | ||
| }; | ||
| } | ||
| }", | ||
| Parameter("x"), | ||
| Keyword("_")); | ||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task UnusedUnderscoreParameterInLambda() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| System.Func<int, int> a = (int _) => 0; | ||
| } | ||
| }", | ||
| Namespace("System"), | ||
| Delegate("Func")); | ||
|
JoeRobich marked this conversation as resolved.
|
||
| } | ||
|
|
||
| [Fact, Trait(Traits.Feature, Traits.Features.Classification)] | ||
| public async Task UsedUnderscoreParameterInLambda() | ||
| { | ||
| await TestAsync(@" | ||
| class X | ||
| { | ||
| void N() | ||
| { | ||
| System.Func<int, int> a = (int _) => _; | ||
| } | ||
| }", | ||
| Namespace("System"), | ||
| Delegate("Func"), | ||
| Parameter("_")); | ||
| } | ||
|
|
||
| [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("_")); | ||
| } | ||
|
Comment on lines
+3808
to
+3840
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tests with tuples?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the "_" be highlighted for tuples? Currently they aren't and the original Issue only talked about arguments.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
IMO, yes. If we're going to classify discards this way, we should consistently do it everywhere they come up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. In that case i could use the new implementation and make it deal with Do we have some kind of syntax generator that i can use to generate test cases for most types of syntax constructs containing discards?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
WFM.
Yes. He's called @jcouv :) |
||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here are some more scenarios to test (different contexts where discards are allowed):
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jcouv Was tehre any work done to make
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a little: #38786
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First of all, thank you for providing a list of test cases.
Is this actually a discarding case? for me this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a discarding case:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, to recognize discard parameters in lambdas, the key API is |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
|
||
| #nullable enable | ||
|
|
||
| using System; | ||
| using System.Collections.Immutable; | ||
| using System.Threading; | ||
| using Microsoft.CodeAnalysis.Classification; | ||
| using Microsoft.CodeAnalysis.Classification.Classifiers; | ||
| using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
| using Microsoft.CodeAnalysis.PooledObjects; | ||
| using Roslyn.Utilities; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Classification.Classifiers | ||
| { | ||
| internal class DiscardSyntaxClassifier : AbstractSyntaxClassifier | ||
| { | ||
| public override ImmutableArray<Type> SyntaxNodeTypes { get; } = ImmutableArray.Create( | ||
| typeof(DiscardDesignationSyntax), | ||
| typeof(DiscardPatternSyntax), | ||
| typeof(ParameterSyntax), | ||
| typeof(IdentifierNameSyntax)); | ||
|
|
||
| public override void AddClassifications( | ||
| Workspace workspace, | ||
| SyntaxNode syntax, | ||
| SemanticModel semanticModel, | ||
| ArrayBuilder<ClassifiedSpan> result, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| if (syntax.IsKind(SyntaxKind.DiscardDesignation) || syntax.IsKind(SyntaxKind.DiscardPattern)) | ||
| { | ||
| result.Add(new ClassifiedSpan(syntax.Span, ClassificationTypeNames.Keyword)); | ||
| return; | ||
| } | ||
|
|
||
| switch (syntax) | ||
| { | ||
| case ParameterSyntax parameter when parameter.Identifier.Text == "_": | ||
| var symbol = semanticModel.GetDeclaredSymbol(parameter, cancellationToken); | ||
|
lameox marked this conversation as resolved.
|
||
|
|
||
| if (symbol?.IsDiscard == true) | ||
| { | ||
| result.Add(new ClassifiedSpan(parameter.Identifier.Span, ClassificationTypeNames.Keyword)); | ||
| } | ||
| break; | ||
|
|
||
| case IdentifierNameSyntax identifierName when identifierName.Identifier.Text == "_": | ||
| var symbolInfo = semanticModel.GetSymbolInfo(identifierName, cancellationToken); | ||
|
|
||
| if (symbolInfo.Symbol?.Kind == SymbolKind.Discard) | ||
| { | ||
| result.Add(new ClassifiedSpan(syntax.Span, ClassificationTypeNames.Keyword)); | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.