Warn for is-pattern using a relational pattern with a known result. by gafter · Pull Request #42501 · dotnet/roslyn

gafter · 2020-03-17T17:48:05Z

Relates to #40727 (patterns for C# 9)

…into patterns3-is-always-true-false

gafter · 2020-03-20T18:09:32Z

@agocke @RikkiGibson Can you please review this small pattern-matching change?

cston · 2020-03-20T19:41:22Z

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

+            else if (values?.Complement().IsEmpty != true)
+            {
+                tests.Add(new Tests.One(new BoundDagRelationalTest(rel.Syntax, rel.Relation, rel.ConstantValue, output, rel.HasErrors)));
+            }


Consider wrapping block in if (fac is { }) { ... } to avoid multiple ?. operators and == true. #ByDesign

I tried that, but ended up having to repeat line 625 in multiple else clauses.

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

cston · 2020-03-20T19:41:56Z

src/Compilers/CSharp/Portable/BoundTree/BoundDecisionDag.cs

+                        case BoundDagRelationalTest d:
+                            var f = ValueSetFactory.ForSpecialType(input.Type.SpecialType);
+                            if (f is null) return null;
+                            // TODO: When ValueSetFactory has a method for comparing two values, use it.


TODO [](start = 31, length = 4)

PROTOTYPE? #ByDesign

No. The code works correctly as is. This is a suggestion for a future optimization, which I expect to happen after merging into master.

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

Should this be also filed as a follow-up issue? #Resolved

That wouldn't be particularly helpful to me, though I don't object to it.

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

cston · 2020-03-20T20:50:31Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests2.cs

+}
+";
+            CreateCompilation(source, options: TestOptions.ReleaseDll, parseOptions: TestOptions.RegularWithPatternCombinators).VerifyDiagnostics(
+                    // (7,18): error CS8120: The switch case has already been handled by a previous case.


error CS8120: The switch case has already been handled by a previous case. [](start = 31, length = 74)

This diagnostic message is confusing since there is no previous case. #Resolved

Not sure what to do about this, as this is the message used (since C# 7.0) when a switch case is unreachable. What do you suggest? Give a different message if it is the first case? Reword it to be technically correct "Any value that would match this case has been handled by a previous case." ?

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

Or perhaps "Unreachable case. If there are values that satisfy this case, those values have been handled in previous cases."

In reply to: 395908208 [](ancestors = 395908208,395883716)

cston · 2020-03-20T20:51:39Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests2.cs

+                    // (7,18): error CS8120: The switch case has already been handled by a previous case.
+                    //             case < int.MinValue: break; // 1
+                    Diagnostic(ErrorCode.ERR_SwitchCaseSubsumed, "< int.MinValue").WithLocation(7, 18),
+                    // (9,18): error CS8120: The switch case has already been handled by a previous case.


error CS8120: The switch case has already been handled by a previous case. [](start = 31, length = 74)

This message is confusing since the previous cases do not handle this (out of range) case. #Resolved

jcouv

LGTM Thanks (iteration 4)

… impossible to match.

gafter · 2020-03-24T18:27:00Z

@cston Do these error messages look better?

…into patterns3-is-always-true-false

…arning per design review.

gafter · 2020-03-25T22:13:28Z

@cston @jcouv @agocke @RikkiGibson Would you please have a look at the last iteration of this PR? It now gives warnings for more scenarios of always-true is-pattern expressions as requested at the feature design review yesterday.

jcouv · 2020-03-25T22:40:21Z

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests3.cs

                // (6,29): error CS8521: Pattern-matching is not permitted for pointer types.
                //     bool M1(int* p) => p is null; // 1
                Diagnostic(ErrorCode.ERR_PointerTypeInPatternMatching, "null").WithLocation(6, 29),
+                // (7,24): warning CS8794: An expression of type 'int*' always matches the provided pattern.


nit: should/could we try and avoid the cascade here? The error (not pattern-matching on pointer type) seems more fundamental

jcouv · 2020-03-25T22:41:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesVsPatterns.cs

            c.VerifyTypes();
            c.VerifyDiagnostics(
+                // (6,13): warning CS8794: An expression of type 'object' always matches the provided pattern.
+                //         if (x is var _)


This should be added to the breaking changes document and thread.

jcouv

LGTM Thanks (iteration 8), modulo documentation of breaking change (such as warning on var _)

gafter · 2020-03-25T23:13:28Z

@jcouv I removed the breaking change.

jcouv

LGTM Thanks (iteration 9)

Warn for is-pattern using a relational pattern with a known result.

8e5126d

gafter added Area-Compilers Feature - Pattern Matching Pattern Matching labels Mar 17, 2020

gafter added this to the Compiler.Net5 milestone Mar 17, 2020

gafter requested review from RikkiGibson and agocke March 17, 2020 17:48

gafter requested a review from a team as a code owner March 17, 2020 17:48

gafter self-assigned this Mar 17, 2020

Neal Gafter added 3 commits March 18, 2020 13:30

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

81d5459

…into patterns3-is-always-true-false

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

c02a2c7

…into patterns3-is-always-true-false

Fixup after merge from features/patterns3

7d4d669

gafter force-pushed the patterns3-is-always-true-false branch from 47db30f to 7d4d669 Compare March 20, 2020 18:02

cston reviewed Mar 20, 2020

View reviewed changes

cston approved these changes Mar 20, 2020

View reviewed changes

jcouv approved these changes Mar 24, 2020

View reviewed changes

jcouv self-assigned this Mar 24, 2020

Neal Gafter added 2 commits March 24, 2020 10:58

Adjust subsumption diagnostics for clarity given that patterns can be…

702515a

… impossible to match.

Update xlf files.

b30b15a

cston approved these changes Mar 24, 2020

View reviewed changes

gafter closed this Mar 24, 2020

gafter reopened this Mar 24, 2020

Neal Gafter added 2 commits March 25, 2020 09:41

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

21e3571

…into patterns3-is-always-true-false

Expand the set of is-pattern forms that result in an always-matched w…

e381464

…arning per design review.

jcouv reviewed Mar 25, 2020

View reviewed changes

jcouv approved these changes Mar 25, 2020

View reviewed changes

Don't warn for is-pattern with a discard.

149ffce

gafter mentioned this pull request Mar 25, 2020

Test Plan for C# 9 pattern-matching improvements. #40727

Open

34 tasks

jcouv approved these changes Mar 26, 2020

View reviewed changes

gafter merged commit 7fcd921 into dotnet:features/patterns3 Mar 26, 2020

Conversation

gafter commented Mar 17, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Mar 20, 2020

Uh oh!

cston Mar 20, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

cston Mar 20, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 24, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

cston Mar 20, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

cston Mar 20, 2020 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented Mar 24, 2020

Uh oh!

gafter commented Mar 25, 2020

Uh oh!

jcouv Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 25, 2020

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

gafter commented Mar 25, 2020

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gafter commented Mar 17, 2020 •

edited by jcouv

Loading

cston Mar 20, 2020 •

edited by gafter

Loading

cston Mar 20, 2020 •

edited by gafter

Loading

jcouv Mar 24, 2020 •

edited by gafter

Loading

cston Mar 20, 2020 •

edited by gafter

Loading

gafter Mar 20, 2020 •

edited

Loading

cston Mar 20, 2020 •

edited by gafter

Loading