Initial work for C# 9 pattern-matching changes #39913

gafter · 2019-11-21T01:11:14Z

This implements

Parts of the spec in Proposed changes for Pattern Matching in C# 9.0 - Draft Specification csharplang#2850
- Parenthesized Patterns (All but exhaustive testing)
- Relational Patterns (Syntax and Local binding only)
- Pattern Combinators (Syntax and Local binding only)
- No work on the Open Issues section (e.g. type flow for the and combinator)
The spec in Champion: Type Patterns (VS 16.8, .NET 5) csharplang#2925
- All but exhaustive testing
Not implemented: handling designators under not and or, decision tree changes, exhaustiveness
Does not handle Handle array type patterns in switch statements #40149
- e.g. A pattern written (A[]) should be capable of matching a value of type A[].

Does not yet pass all tests.

…nge.

…into patterns3-parse1

Related to dotnet#40015

gafter · 2019-12-02T19:04:40Z

@agocke @RikkiGibson Can you please review this? I'd be happy to walk through it in person if that helps. #Resolved

RikkiGibson · 2019-12-02T20:26:12Z

src/Compilers/CSharp/Test/Symbol/Symbols/ConversionTests.cs

@@ -9,6 +9,7 @@
 using Xunit;
 using System.Collections.Generic;
 using System;
+using System.Net.Sockets;


appears to be added by mistake #Closed

RikkiGibson · 2019-12-02T21:47:01Z

src/Compilers/CSharp/Portable/Compilation/CSharpSemanticModel.cs

@@ -4012,7 +4012,7 @@ private static ImmutableArray<MethodSymbol> FilterOverriddenOrHiddenMethods(Immu
                        // chose for M.
                        var call = (BoundCall)boundNodeForSyntacticParent;
                        InvocationExpressionSyntax invocation = call.Syntax as InvocationExpressionSyntax;
-                        if (invocation != null && invocation.Expression.SkipParens() == boundNode.Syntax.SkipParens() && (object)call.Method != null)
+                        if (invocation != null && invocation.Expression.SkipParens() == (boundNode.Syntax as ExpressionSyntax).SkipParens() && (object)call.Method != null)


(boundNode.Syntax as ExpressionSyntax) [](start = 88, length = 38)

Consider making this a cast #Closed

RikkiGibson · 2019-12-02T21:56:27Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

@@ -9,6 +9,7 @@ namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax

    internal partial class LanguageParser : SyntaxParser
    {
+
        /// <summary>


nit: empty line above #Closed

RikkiGibson · 2019-12-02T21:56:42Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

@@ -442,33 +281,28 @@ private PatternSyntax ParsePatternContinued(TypeSyntax type, Precedence preceden
                    propertyPatternClause0 == null &&
                    designation0 == null &&
                    subPatterns.Count == 1 &&
-                    subPatterns[0].NameColon == null &&
-                    subPatterns.SeparatorCount == 0)
+                    subPatterns[0].NameColon == null


nit: the ')' should be at the end of this line #Closed

RikkiGibson · 2019-12-02T21:57:03Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationNodes.cs

@@ -28,6 +28,24 @@ internal sealed class CSharpLazyNoneOperation : LazyNoneOperation
    }


+    internal sealed class CSharpLazyNonePatternOperation : LazyNoneOperation, IPatternOperation


Is this a placeholder until the IOperation part of the feature is done? If so consider adding a PROTOTYPE comment. #Closed

The PROTOTYPE comments are in CSharpOperationFactory around line 293.

In reply to: 352880196 [](ancestors = 352880196)

RikkiGibson · 2019-12-02T21:57:18Z

src/Test/Utilities/Portable/TestResource.resx

@@ -209,6 +209,9 @@ namespace My
            }
        }

+        public static bool IsAsciiLetter(char c) => c is (>= 'a' and &lt;= 'z') or (>= 'A' and &lt;= 'Z');


This appears to be only partially XML escaped. Is it only necessary to escape the '<' and not the '>'? #Closed

Correct.

In reply to: 352880294 [](ancestors = 352880294)

RikkiGibson · 2019-12-02T21:57:27Z

...tures/CSharpTest/Completion/CompletionProviders/PropertySubPatternCompletionProviderTests.cs

@@ -484,7 +484,7 @@ class D
            await VerifyNoItemsExistAsync(markup);
        }

-        [Fact]
+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/40015")]


Should these have PROTOTYPE comments? #Closed

RikkiGibson · 2019-12-02T21:57:35Z

src/EditorFeatures/CSharpTest/Completion/CompletionProviders/SymbolCompletionProviderTests.cs

@@ -1319,7 +1319,7 @@ public async Task PositionalPatternFirstPosition()
            await VerifyItemExistsAsync(AddUsingDirectives("using System;", AddInsideMethod(content)), @"System");
        }

-        [Fact, Trait(Traits.Feature, Traits.Features.Completion)]
+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/40015"), Trait(Traits.Feature, Traits.Features.Completion)]


Should this also have a prototype comment? #Closed

Only needed when there is no tracking issue.

In reply to: 352880444 [](ancestors = 352880444)

RikkiGibson · 2019-12-02T21:57:39Z

src/EditorFeatures/CSharpTest2/Recommendations/WhenKeywordRecommenderTests.cs

@@ -191,15 +191,15 @@ public async Task TestForCatchClause_NotAfterFilter3()
        public async Task TestForSwitchCase_NotInEmptySwitchStatement() =>
            await VerifyAbsenceAsync(AddInsideMethod(@"switch (1) { $$ }"));

-        [Fact, Trait(Traits.Feature, Traits.Features.KeywordRecommending)]
+        [Fact(Skip = "https://github.com/dotnet/roslyn/issues/40015"), Trait(Traits.Feature, Traits.Features.KeywordRecommending)]


Should these also have prototype comments? #Closed

No, since we have an open issue to track this.

In reply to: 352880470 [](ancestors = 352880470)

RikkiGibson · 2019-12-02T21:57:45Z

        x.ToString(); // warn

nit: please remove this comment to help make things clear. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs:136 in 6d450fb. [](commit_id = 6d450fb, deletion_comment = True)

RikkiGibson · 2019-12-02T22:04:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+
+            // fall back to a type pattern if it fails as a constant pattern.
+            if (!wasExpression && !IsUnderscore(innerExpression) &&
+                ((CSharpParseOptions)node.SyntaxTree.Options).IsFeatureEnabled(MessageID.IDS_FeatureTypePattern))


We should attempt to bind a type pattern here regardless of the language version, and then issue a LangVersion diagnostic if the feature is not enabled. #Closed

RikkiGibson · 2019-12-02T22:06:09Z

Done with review pass (iteration 8) #Closed

gafter · 2019-12-03T20:28:41Z

@RikkiGibson I think I've responded to all of your comments. Do you have anything else?

@agocke Can you please review this? #Resolved

RikkiGibson

gafter · 2019-12-04T21:46:34Z

@agocke Can you please review this?

agocke · 2019-12-05T23:10:56Z

@gafter Could you summarize what things you would expect to be changed after this PR? Which parts of the language changes are included in this PR? #Resolved

agocke · 2019-12-05T23:56:10Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+                    constantDiagnostics.Free();
+                    hasErrors |= CheckValidPatternType(innerExpression, inputType, boundType.Type, patternTypeWasInSource: true, diagnostics: diagnostics);
+                    CheckFeatureAvailability(innerExpression, MessageID.IDS_FeatureExpressionVariablesInQueriesAndInitializers, diagnostics);
+                    return new BoundTypePattern(node, boundType, inputType, hasErrors);


Consider getting rid of this early return. I found it made the code flow more confusing for me and it was harder to validate that we were calling the right diagnostics freeing code in the right places. #Resolved

Sure, but I don't think you'll like it better.

In reply to: 354607869 [](ancestors = 354607869)

I think it's a little bit clearer on the freeing, and a little less clear on behavior. I don't feel strongly either way.

agocke · 2019-12-05T23:59:41Z

src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs

+        private BinaryOperatorKind TokenKindToBinaryOperatorKind(SyntaxKind kind)
+        {
+            return kind switch


Consider

private BinaryOperatorKind TokenKindToBinaryOperatorKind(SyntaxKind kind) => kind switch { ``` #Resolved

agocke · 2019-12-06T00:10:15Z

src/Compilers/CSharp/Test/Syntax/Parsing/NullableParsingTests.cs

@@ -717,15 +717,15 @@ public void DeclarationPattern_NullableType()
        public void DeclarationPattern_NullableArray()
        {
            UsingStatement("switch (e) { case T[]? t: break; }",
-                // (1,21): error CS0443: Syntax error; value expected
+                // (1,19): error CS8652: The feature 'type pattern' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.


This seems worrisome. I thought we disambiguated ? by saying that it wasn't part of a type in a pattern, and thus it would parse to ?: in legacy cases. Should this still be a ?:? #ByDesign

The change here is caused by T[]? previously being treated as the start of an expression starting with an index operator. Now we disambiguate it as a type pattern. Either way, the ? isn't part of the pattern, but is an unexpected token that results in a syntax error. Both interpretations result in a syntax error, so we prefer the type interpretation (which is the change)

In reply to: 354611441 [](ancestors = 354611441)

gafter · 2019-12-06T00:41:10Z

@agocke This implements

Parts of the spec in Proposed changes for Pattern Matching in C# 9.0 - Draft Specification csharplang#2850
- Parenthesized Patterns (All but exhaustive testing)
- Relational Patterns (Syntax and Local binding only)
- Pattern Combinators (Syntax and Local binding only)
- No work on the Open Issues section (e.g. type flow for the and combinator)
The spec in Champion: Type Patterns (VS 16.8, .NET 5) csharplang#2925
- All but exhaustive testing
Does not handle Handle array type patterns in switch statements #40149
- A pattern written (A[]) should be capable of matching a value of type A[].
  #Resolved

agocke · 2019-12-06T18:24:53Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                return false;
+            }
+
+            switch (this.PeekToken(1).Kind)


This seems like we're testing if we're in a type-only context. Is that correct? If so, don't we have a more general helper for this? If not, how do we know this list is exhaustive? #ByDesign

No, this is checking whether or not a token is named var and in a syntactic context such that if it were interpreted as a type, it could be the lazily evaluated type var. It is mainly trying to rule out contexts in which it is definitely not the var type.

In reply to: 354968841 [](ancestors = 354968841)

agocke · 2019-12-06T18:32:12Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

+                ConstantPatternSyntax cp when ConvertExpressionToType(cp.Expression, out NameSyntax type) => type,
+                TypePatternSyntax tp => tp.Type,
+                DiscardPatternSyntax dp => _syntaxFactory.IdentifierName(ConvertToIdentifier(dp.UnderscoreToken)),
+                var p => p,


_ => pattern? #ByDesign

I prefer this form, so that the result is entirely a function of the already computed input.

In reply to: 354971890 [](ancestors = 354971890)

gafter · 2019-12-13T21:40:52Z

@agocke I think I've responded to all of your comments. Do you have any others?

agocke

LGTM

…ns3-parse1

jasonmalinowski · 2019-12-16T21:52:38Z

@gafter: this is now tagging a lot of commits for unrelated things, is this is just a git mistake or something that somebody needs to review?

gafter · 2019-12-16T21:54:19Z

oops.

RikkiGibson · 2019-12-17T20:46:45Z

Looks like GitHub is reporting that roslyn-integration-CI failed, but it actually passed.

alrz · 2020-02-27T15:33:18Z

src/Compilers/CSharp/Portable/Parser/LanguageParser_Patterns.cs

-            {
-                return false;
+                var orToken = ConvertToKeyword(this.EatToken());
+                var right = ParseConjunctivePattern(precedence, afterIs, whenIsKeyword);


@gafter Conjunctive patterns are right-associative, I don't think that's by-design?

Neal Gafter and others added 7 commits November 7, 2019 16:51

Syntax model changes to support C# 9.0 pattern-matching changes.

96cb8e7

Parsing for new pattern forms.

039254e

Does not yet pass all tests.

Test changes to accommodate pattern-matching changes for C# 9.0

5593671

Bind the new pattern forms enough so that existing tests pass.

f1dad8f

Disambiguate pattern combinators per LDM to minimize any breaking cha…

a13f0ce

…nge.

Merge branch 'features/patterns3' of https://github.com/dotnet/roslyn …

d7b59fd

…into patterns3-parse1

Use recently introduced IsUnderscoreToken() helper.

c77c2ec

gafter requested a review from a team as a code owner November 21, 2019 01:11

gafter added this to the Compiler.Next milestone Nov 21, 2019

gafter added Area-Compilers Feature - Pattern Matching Pattern Matching labels Nov 21, 2019

gafter self-assigned this Nov 21, 2019

Skip tests broken by language changes.

6d450fb

Related to dotnet#40015

gafter requested a review from a team as a code owner November 26, 2019 01:23

gafter requested review from RikkiGibson and agocke December 2, 2019 19:04

RikkiGibson reviewed Dec 2, 2019

View reviewed changes

Minor changes per code review.

e04aa2e

RikkiGibson approved these changes Dec 3, 2019

View reviewed changes

agocke reviewed Dec 6, 2019

View reviewed changes

Minor changes suggested in review.

bb3e545

agocke approved these changes Dec 13, 2019

View reviewed changes

Merge branch 'master' of https://github.com/dotnet/roslyn into patter…

1773c04

…ns3-parse1

gafter requested review from a team as code owners December 16, 2019 21:50

gafter merged commit 3bd9308 into dotnet:features/patterns3 Dec 17, 2019

alrz reviewed Feb 27, 2020

View reviewed changes

		@@ -28,6 +28,24 @@ internal sealed class CSharpLazyNoneOperation : LazyNoneOperation
		}


		internal sealed class CSharpLazyNonePatternOperation : LazyNoneOperation, IPatternOperation

Initial work for C# 9 pattern-matching changes #39913

Initial work for C# 9 pattern-matching changes #39913

Conversation

gafter commented Nov 21, 2019 • edited Loading

gafter commented Dec 2, 2019 • edited Loading

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Dec 2, 2019 • edited Loading

RikkiGibson Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

RikkiGibson commented Dec 2, 2019 • edited Loading

gafter commented Dec 3, 2019 • edited by RikkiGibson Loading

RikkiGibson left a comment

Choose a reason for hiding this comment

gafter commented Dec 4, 2019

agocke commented Dec 5, 2019 • edited by gafter Loading

agocke Dec 5, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Dec 5, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

agocke Dec 6, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Dec 6, 2019 • edited Loading

agocke Dec 6, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agocke Dec 6, 2019 • edited by gafter Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Dec 13, 2019

agocke left a comment

Choose a reason for hiding this comment

jasonmalinowski commented Dec 16, 2019

gafter commented Dec 16, 2019

RikkiGibson commented Dec 17, 2019

Choose a reason for hiding this comment

gafter commented Nov 21, 2019 •

edited

Loading

gafter commented Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson commented Dec 2, 2019 •

edited

Loading

RikkiGibson Dec 2, 2019 •

edited

Loading

RikkiGibson commented Dec 2, 2019 •

edited

Loading

gafter commented Dec 3, 2019 •

edited by RikkiGibson

Loading

agocke commented Dec 5, 2019 •

edited by gafter

Loading

agocke Dec 5, 2019 •

edited by gafter

Loading

agocke Dec 5, 2019 •

edited by gafter

Loading

agocke Dec 6, 2019 •

edited by gafter

Loading

gafter commented Dec 6, 2019 •

edited

Loading

agocke Dec 6, 2019 •

edited by gafter

Loading

agocke Dec 6, 2019 •

edited by gafter

Loading