Skip to content
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

Introduce a DiscardSyntaxClassifier #40396

Merged
merged 25 commits into from
Jan 27, 2020
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
543803d
Introduce a DiscardParameterHighlighter which fixes #39768
lameox Dec 14, 2019
31feeb8
Clean up usings. Add copyright headers.
lameox Dec 15, 2019
e6d2199
Use the generic AbstractKeywordHighlighter<TNode> as base class.
lameox Dec 15, 2019
2bed547
Fixed formatting
lameox Dec 15, 2019
8bd63d9
Removed IsKind() check before type checking, merged ifs
lameox Dec 15, 2019
d23ec64
Fixed return types in test cases.
lameox Dec 16, 2019
8786800
Renamed the highlighter and test to DiscardArgumentHighlighter instea…
lameox Dec 16, 2019
3552b39
Additional test for a @_ that is already in scope and used as the out…
lameox Dec 16, 2019
e71d97c
Removed DiscardArgumentHighlighter(Tests). The fix needs to happen in…
lameox Dec 17, 2019
03b7756
Introduced a new DiscardSyntaxClassifier which operates on ArgumentSy…
lameox Dec 17, 2019
d366c3c
Extended the DiscardSyntaxClassifier so it covers the other discard p…
lameox Dec 20, 2019
8e93d34
switched to the more general base class (AbstractSyntaxClassifier)
lameox Jan 2, 2020
bf47f1b
removed he special case for a single underscore parameter that isn't …
lameox Jan 8, 2020
74d8060
Added an aditional test for lambdas with inferred type
lameox Jan 8, 2020
3a493ea
Refactoring of the DiscardSyntaxClassifier:
lameox Jan 8, 2020
8b77049
avoid binding all identifier names
lameox Jan 8, 2020
16ba315
inline ClassifySymbol
lameox Jan 8, 2020
d498eb8
throw unreachable exception instead of asserting
lameox Jan 8, 2020
25900e9
simplified parameter handling. Inlined it as well.
lameox Jan 8, 2020
3bed577
added check for identifier text on parameter, moved the checks into t…
lameox Jan 8, 2020
7bdbfb9
fixed possible NPE
lameox Jan 8, 2020
5ab4a04
added tests & code for discards in variable declarators
lameox Jan 15, 2020
b948f05
Update src/Workspaces/CSharp/Portable/Classification/SyntaxClassifica…
lameox Jan 15, 2020
e914514
removed most of the last commit (handling declarators) since they are…
lameox Jan 16, 2020
2122217
Update src/EditorFeatures/CSharpTest/Classification/SemanticClassifie…
lameox Jan 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3612,5 +3612,186 @@ void M<T>() where T : notnull { }
TypeParameter("T"),
Keyword("notnull"));
}

[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.
Show resolved Hide 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.
Show resolved Hide 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("_"));
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests with tuples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

IMO, yes. If we're going to classify discards this way, we should consistently do it everywhere they come up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 DiscardPatternSyntax and DiscardDesignationSyntax which, according to the Synatx.xml should be the only two types we actually need to 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?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 17, 2019

Choose a reason for hiding this comment

The 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 DiscardPatternSyntax and DiscardDesignationSyntax which, according to the Synatx.xml should be the only two types we actually need to deal with.

WFM.

Do we have some kind of syntax generator that i can use to generate test cases for most types of syntax constructs containing discards?

Yes. He's called @jcouv :)

}
Copy link
Member

Choose a reason for hiding this comment

The 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):

  • out variable declarations, such as bool found = TryGetValue(out var _) or bool found = TryGetValue(out _)
  • deconstruction assignments, such as (x, _) = deconstructable;
  • deconstruction declarations, such as (var x, var _) = deconstructable;
  • is patterns, such as x is int _
  • switch/case patterns, such as case int _:
  • lambda parameters, such as (_, _) => body; or (int _, int _) => body;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcouv Was tehre any work done to make _ in a lambda a discard?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a little: #38786

Copy link
Contributor Author

@lameox lameox Dec 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for providing a list of test cases.

lambda parameters, such as (_, _) => body; or (int _, int _) => body;

Is this actually a discarding case? for me this Func<int, int, int> a = (int _, int __) => { return _ + __; }; compiles and I can use the "discard" in the body. I also get a CS0100 if i define it whith two single underscores.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a discarding case: Func<int, int, int> a = (int _, int __) => { return _ + __; };
But this is: Func<int, int, int> a = (int _, int _) => { return /* cannot reference either discards here */; };. This requires language version Preview (otherwise it's a compilation error).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, to recognize discard parameters in lambdas, the key API is IParameterSymbol.IsDiscard.

Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ public CSharpSyntaxClassificationService(HostLanguageServices languageServices)
new NameSyntaxClassifier(),
new OperatorOverloadSyntaxClassifier(),
new SyntaxTokenClassifier(),
new UsingDirectiveSyntaxClassifier()
new UsingDirectiveSyntaxClassifier(),
new DiscardSyntaxClassifier()
});
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// 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(LambdaExpressionSyntax),
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 LambdaExpressionSyntax lambda:
{
var symbolInfo = semanticModel.GetSymbolInfo(lambda, cancellationToken);
ClassifyLambdaParameter(symbolInfo, result);
}
break;

case IdentifierNameSyntax identifier:
if (identifier.Identifier.Text == "_")
{
var symbolInfo = semanticModel.GetSymbolInfo(identifier, cancellationToken);

if (symbolInfo.Symbol?.Kind == SymbolKind.Discard)
{
result.Add(new ClassifiedSpan(syntax.Span, ClassificationTypeNames.Keyword));
}
}
break;

default:
throw ExceptionUtilities.Unreachable;
};
}

private void ClassifyLambdaParameter(SymbolInfo symbolInfo, ArrayBuilder<ClassifiedSpan> result)
{
// classify lambda parameters of the forms
// (int _, int _) => ...
// or
// (_, _) => ...
// which aren't covered by TryClassifySymbol

if (!(symbolInfo.Symbol is IMethodSymbol symbol))
{
return;
}

foreach (var parameter in symbol.Parameters)
lameox marked this conversation as resolved.
Show resolved Hide resolved
{
if (parameter.IsDiscard)
{
result.Add(new ClassifiedSpan(parameter.Locations[0].SourceSpan, ClassificationTypeNames.Keyword));
}
}
}
}
}