Forbid 'default' as a case constant or a pattern. by gafter · Pull Request #23629 · dotnet/roslyn

gafter · 2017-12-07T01:31:16Z

Has been reviewed by LDM and compat council.

Customer scenario

The C# language design team wants to forbid the use of a default expression default (without the type between parens) as a constant pattern for pattern-matching, to make room for evolution of that feature in C# 8.0. We are treating this as a bug fix against the default literal feature that was introduced in C# 7.1, and as such we want to get the change in as soon as possible.

Bugs this fixes

Fixes #23499

Workarounds, if any

N/A

Risk

Low, though there is a slight compat risk if people have already written code like case default:. We think that is unlikely, as it produced a warning before.

Performance impact

Only trivial additional complexity in the compiler.

Is this a regression from a previous update?

N/A

Root cause analysis

We did not predict the future well enough. 😉

How was the bug found?

We discovered this while designing recursive patterns.

Test documentation updated?

N/A

@dotnet/roslyn-compiler @jcouv Please review for 15.6

Has been reviewed by LDM and compat council. Fixes dotnet#23499

alrz · 2017-12-07T03:08:15Z

(int)default should be valid? #Resolved

gafter · 2017-12-07T04:09:49Z

@alrz Yes, (int)default is fine. #Resolved

jcouv · 2017-12-07T18:17:34Z

-            bool wasExpression;
-            return BindConstantPattern(node, operandType, node.Expression, hasErrors, diagnostics, out wasExpression, wasSwitchCase);
+            var innerExpression = patternExpression.SkipParens();
+            if (innerExpression.Kind() == SyntaxKind.DefaultLiteralExpression)


Any chance innerExpression could be null here? (case (): maybe?) #Resolved

No. That is a positional pattern, but even if parsed as an expression the parser will insert a missing identifier. #Resolved

jcouv · 2017-12-07T18:27:17Z

+}
+namespace System
+{
+    public struct ValueTuple<T1, T2>


Does this test actually need ValueTuple type? #Resolved

jcouv · 2017-12-07T18:28:06Z

+                //         System.Console.Write($"{hello is default} {nullString is default} {two is default} {zero is default}");
+                Diagnostic(ErrorCode.ERR_DefaultInPattern, "default").WithLocation(10, 101)
+                );
+            //CompileAndVerify(comp, expectedOutput: "False True False True");


Could remove the old code #Resolved

AlekseyTs · 2017-12-08T00:24:19Z

+                Diagnostic(ErrorCode.ERR_DefaultInSwitch, "default").WithLocation(12, 18)
                );
-            CompileAndVerify(comp, expectedOutput: "default");
+            //CompileAndVerify(comp, expectedOutput: "default");


//CompileAndVerify(comp, expectedOutput: "default"); [](start = 12, length = 52)

Please remove commented out code. #Closed

AlekseyTs · 2017-12-08T00:24:35Z

+                Diagnostic(ErrorCode.ERR_DefaultInSwitch, "default").WithLocation(12, 18)
                );
-            CompileAndVerify(comp, expectedOutput: "default");
+            //CompileAndVerify(comp, expectedOutput: "default");


//CompileAndVerify(comp, expectedOutput: "default"); [](start = 12, length = 52)

Please remove commented out code. #Closed

AlekseyTs · 2017-12-08T00:25:02Z

+                //             case (default):
+                Diagnostic(ErrorCode.ERR_DefaultInSwitch, "default").WithLocation(12, 19)
+                );
+            //CompileAndVerify(comp, expectedOutput: "default");


//CompileAndVerify(comp, expectedOutput: "default"); [](start = 12, length = 52)

Please remove commented out code. #Closed

AlekseyTs · 2017-12-08T00:25:28Z

+                //             case (default):
+                Diagnostic(ErrorCode.ERR_DefaultInSwitch, "default").WithLocation(12, 19)
+                );
+            //CompileAndVerify(comp, expectedOutput: "default");


//CompileAndVerify(comp, expectedOutput: "default"); [](start = 12, length = 52)

Please remove commented out code. #Closed

AlekseyTs · 2017-12-08T00:28:20Z

Done with review pass (iteration 3) #Closed

gafter · 2017-12-09T03:17:44Z

This PR has been modified to remove some unneeded lines per review comments. Reviewers, (@jcouv and @AlekseyTs), do you have any other issues? #Resolved

AlekseyTs

LGTM (iteration 4)

jcouv

LGTM Thanks

jcouv · 2017-12-09T20:29:32Z

@MeiChin-Tsai for ask-mode approval for 15.6. Thanks

MeiChin-Tsai · 2017-12-10T04:59:38Z

Approved. Not all job is clean.

gafter · 2017-12-10T12:46:47Z

Test windows_debug_unit64_prtest please

stephentoub · 2018-03-21T02:36:52Z

We think that is unlikely, as it produced a warning before.

FWIW, I don't believe we were seeing any warnings for this in corefx, where we did take a dependency on this in several places (and where we compile with warnings-as-errors). We just removed those as they broke the build when we upgraded to a newer compiler.
dotnet/corefx#28284

jcouv · 2018-03-28T22:41:02Z

@gafter From this PR, it indeed looks like we forgot to document the breaking change.

gafter · 2018-03-28T23:44:01Z

Please see #25800

gafter added Area-Compilers Bug Feature - Default Interface Impl Default Interface Implementation Feature - Pattern Matching Pattern Matching labels Dec 7, 2017

gafter added this to the 15.6 milestone Dec 7, 2017

gafter requested review from a team and jcouv December 7, 2017 01:31

gafter self-assigned this Dec 7, 2017

Forbid 'default' as a case constant or a pattern.

306d9ad

Has been reviewed by LDM and compat council. Fixes dotnet#23499

gafter force-pushed the master-23499 branch from 17de769 to 306d9ad Compare December 7, 2017 02:00

Update expected diagnostics.

43fdebf

Refine which cases get which diagnostic.

2dca501

jcouv reviewed Dec 7, 2017

View reviewed changes

AlekseyTs reviewed Dec 8, 2017

View reviewed changes

gafter requested a review from a team as a code owner December 9, 2017 03:11

Remove some unneeded lines per code review.

b0d7c27

gafter force-pushed the master-23499 branch from 30fbb1c to b0d7c27 Compare December 9, 2017 03:15

gafter removed the request for review from a team December 9, 2017 03:18

AlekseyTs approved these changes Dec 9, 2017

View reviewed changes

jcouv approved these changes Dec 9, 2017

View reviewed changes

jcouv removed the Feature - Default Interface Impl Default Interface Implementation label Dec 9, 2017

MeiChin-Tsai added the Approved to merge label Dec 10, 2017

gafter merged commit ac2da0f into dotnet:master Dec 10, 2017

Conversation

gafter commented Dec 7, 2017

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

alrz commented Dec 7, 2017 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Dec 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Dec 7, 2017 • 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 Dec 7, 2017 • 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!

jcouv Dec 7, 2017 • 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 Dec 7, 2017 • 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!

AlekseyTs Dec 8, 2017 • 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!

AlekseyTs Dec 8, 2017 • 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!

AlekseyTs Dec 8, 2017 • 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!

AlekseyTs Dec 8, 2017 • 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!

AlekseyTs commented Dec 8, 2017 • edited by gafter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 9, 2017

Uh oh!

MeiChin-Tsai commented Dec 10, 2017

Uh oh!

gafter commented Dec 10, 2017

Uh oh!

stephentoub commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Mar 28, 2018

Uh oh!

gafter commented Mar 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

alrz commented Dec 7, 2017 •

edited by gafter

Loading

gafter commented Dec 7, 2017 •

edited

Loading

jcouv Dec 7, 2017 •

edited by gafter

Loading

gafter Dec 7, 2017 •

edited

Loading

jcouv Dec 7, 2017 •

edited by gafter

Loading

jcouv Dec 7, 2017 •

edited by gafter

Loading

AlekseyTs Dec 8, 2017 •

edited

Loading

AlekseyTs Dec 8, 2017 •

edited

Loading

AlekseyTs Dec 8, 2017 •

edited

Loading

AlekseyTs Dec 8, 2017 •

edited

Loading

AlekseyTs commented Dec 8, 2017 •

edited by gafter

Loading

gafter commented Dec 9, 2017 •

edited

Loading

stephentoub commented Mar 21, 2018 •

edited

Loading