Make "default" a literal and other tweaks from recent LDM #18213

jcouv · 2017-03-27T22:30:05Z

Using LiteralExpressionSyntax instead of introducing a new syntax node
Disallow in expression trees (filed Expression tree support for default literal #18212 for enabling at some point)
Attaching to C# 7.1 instead of a feature flag
disallow default is T, allow x is default, allow default as RefType (with always-null warning)

@cston @gafter @dotnet/roslyn-compiler for review.

Fixes #17478
Relates to #13602

jcouv · 2017-03-28T15:57:49Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenExprLambdaTests.cs

@@ -5012,49 +5012,6 @@ static void Main(string[] args)
                expectedOutput: expectedOutput);
        }

-        [Fact]
-        public void EnumEqualityWithDefault()


Note: This is replaced by a test in TargetTypedDefaultTests.cs below.

cston · 2017-03-28T17:49:17Z

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

+            if (operand.IsLiteralDefault())
+            {
+                var defaultLiteral = (BoundDefaultLiteral)operand;
+                Debug.Assert((object)defaultLiteral.Type == null && (object)defaultLiteral.ConstantValueOpt == null);


Consider splitting into two asserts.

cston · 2017-03-28T17:51:59Z

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

@@ -16,7 +16,7 @@ public static bool IsLiteralNull(this BoundExpression node)

        public static bool IsLiteralDefault(this BoundExpression node)
        {
-            return node.Kind == BoundKind.DefaultLiteral && (object)node.Type == null;
+            return node.Kind == BoundKind.DefaultLiteral && node.Syntax.Kind() == SyntaxKind.DefaultLiteralExpression;
        }


Why is the second condition needed?

BoundKind.DefaultLiteral used to be BoundKind.DefaultOperator. Maybe I will rename it back.
But to answer your question, IsLiteralDefault should return false for default(int). It should only return true for the default literal.

cston · 2017-03-28T17:54:12Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1475,5 +1475,6 @@ internal enum ErrorCode

        ERR_BadDynamicMethodArgDefault = 9000,
        ERR_DefaultNotValid = 9001,
+        ERR_ExpressionTreeContainsDefaultLiteral = 9002,


Should the previous two values be renamed to use the term DefaultLiteral?

cston · 2017-03-28T17:55:44Z

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

@@ -16,7 +16,7 @@ public static bool IsLiteralNull(this BoundExpression node)

        public static bool IsLiteralDefault(this BoundExpression node)


Perhaps IsDefaultLiteral.

The reason I used this seemingly strange name is because it matches IsLiteralNull (a few lines above).

cston · 2017-03-28T17:56:52Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs

+
+        public override BoundNode VisitDefaultLiteral(BoundDefaultLiteral node)
+        {
+            if (_inExpressionLambda && node.IsLiteralDefault())


Related to earlier question: When will node.IsLiteralDefault() return false?

Why is default not allowed in an expression lambda?

cston · 2017-03-28T18:02:21Z

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

+            string source = @"
+enum E
+{
+    Entry = default


Perhaps test Entry = default + 1 as well.

cston · 2017-03-28T18:11:06Z

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

    {
        System.Console.Write(default as long);
+        System.Console.Write(default as T);
    }
 }";


Are we testing default as T with constraints on T?

jcouv · 2017-03-28T22:54:56Z

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

+enum E
+{
+    DefaultEntry = default,
+    OneEntry = default + 1


Chuck asked why this works. I'll follow up.

I'm going to guess that overload resolution finds a most specific overload of operator + that is applicable, and the result is a constant.

Yes, overload resolution succeeds, then the left and right operands get converted, which gives default an int type.

cston · 2017-03-29T00:18:07Z

LGTM

jcouv · 2017-03-31T01:29:13Z

@gafter @dotnet/roslyn-compiler for a second review. Thanks

AlekseyTs · 2017-04-04T18:30:15Z

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

@@ -773,11 +773,6 @@ private BoundExpression BindDeclarationVariables(TypeSymbol declType, VariableDe
            }
        }

-        private static BoundExpression BindDefaultLiteral(DefaultLiteralSyntax node)


private static BoundExpression BindDefaultLiteral(DefaultLiteralSyntax node) [](start = 8, length = 76)

I would keep this method.

AlekseyTs · 2017-04-04T18:57:44Z

src/Compilers/Test/Utilities/CSharp/TestOptions.cs

        public static readonly CSharpParseOptions ExperimentalParseOptions =
-            new CSharpParseOptions(kind: SourceCodeKind.Regular, documentationMode: DocumentationMode.None, languageVersion: LanguageVersion.CSharp7).WithFeatures(s_experimentalFeatures);
+            new CSharpParseOptions(kind: SourceCodeKind.Regular, documentationMode: DocumentationMode.None, languageVersion: LanguageVersion.CSharp7_1).WithFeatures(s_experimentalFeatures);


CSharp7_1 [](start = 141, length = 9)

Should we simply use Latest here?

AlekseyTs · 2017-04-04T19:01:20Z

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

@@ -3540,8 +3540,7 @@ public void TestTargetTypedDefault()
            var expr = this.ParseExpression(text, TestOptions.ExperimentalParseOptions);


ExperimentalParseOptions [](start = 62, length = 24)

This is the only test in this file that is using ExperimentalParseOptions. Consider changing it to use CSharp7 and have have a test with 7.1 next to other default tests.

I changed it to use 7.1 explicitly (instead of Experimental, which now means Latest). But I didn't move it because this parsing test relies on base type (ParsingTests) for some infrastructure.

I already had a test for diagnostics in 7.0 scenario:

[Fact] public void TestCSharp7() { string source = @" class C { static void Main() { int x = default; } } "; var comp = CreateCompilationWithMscorlib(source); comp.VerifyDiagnostics( // (6,17): error CS8107: Feature 'default literal' is not available in C# 7. Please use language version 7.1 or greater. // int x = default; Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion7, "default").WithArguments("default literal", "7.1").WithLocation(6, 17) ); }

Let me know if that addresses your concern.

AlekseyTs

LGTM

AlekseyTs · 2017-04-04T19:44:37Z

Still LGTM

jcouv · 2017-04-04T19:49:14Z

Thanks :-)

Fix error for missing feature

666a273

dnfclas added the cla-already-signed label Mar 27, 2017

jcouv force-pushed the default-LDM branch 3 times, most recently from 732750c to 86928ce Compare March 28, 2017 04:42

jcouv added 3 commits March 28, 2017 07:45

Disallow default literal in expression tree

fe08789

Use syntax akin of other literals

178e5d5

Update behavior for as and is

14a504c

jcouv force-pushed the default-LDM branch from 86928ce to 14a504c Compare March 28, 2017 14:46

jcouv requested a review from gafter March 28, 2017 15:47

jcouv self-assigned this Mar 28, 2017

jcouv added the Area-Compilers label Mar 28, 2017

jcouv commented Mar 28, 2017

View reviewed changes

cston reviewed Mar 28, 2017

View reviewed changes

jcouv added 2 commits March 28, 2017 13:10

Allow default in expression tree again

61cf9ea

Addressing PR feedback

580a972

jcouv commented Mar 28, 2017

View reviewed changes

Fixing EE code to use renamed bound node

363a70e

jcouv force-pushed the default-LDM branch from fa56967 to 363a70e Compare March 29, 2017 03:07

jcouv force-pushed the default-LDM branch 2 times, most recently from 248f8a4 to 9bf3a6e Compare March 31, 2017 03:07

Add warning for "case default:"

8d5e082

jcouv force-pushed the default-LDM branch from 9bf3a6e to 8d5e082 Compare March 31, 2017 05:27

jcouv assigned gafter Apr 4, 2017

AlekseyTs reviewed Apr 4, 2017

View reviewed changes

AlekseyTs approved these changes Apr 4, 2017

View reviewed changes

Address Aleksey's feedback

a39beb2

jcouv merged commit cc1aeba into dotnet:features/default Apr 4, 2017

jcouv deleted the default-LDM branch April 4, 2017 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make "default" a literal and other tweaks from recent LDM #18213

Make "default" a literal and other tweaks from recent LDM #18213

jcouv commented Mar 27, 2017

jcouv Mar 28, 2017

cston Mar 28, 2017

cston Mar 28, 2017

jcouv Mar 28, 2017

cston Mar 28, 2017

cston Mar 28, 2017

jcouv Mar 28, 2017

cston Mar 28, 2017

cston Mar 28, 2017

cston Mar 28, 2017

cston Mar 28, 2017

jcouv Mar 28, 2017 •

edited

Loading

gafter Mar 29, 2017

jcouv Mar 29, 2017

cston commented Mar 29, 2017

jcouv commented Mar 31, 2017

AlekseyTs Apr 4, 2017

AlekseyTs Apr 4, 2017

AlekseyTs Apr 4, 2017

jcouv Apr 4, 2017

AlekseyTs left a comment

AlekseyTs commented Apr 4, 2017

jcouv commented Apr 4, 2017

		@@ -16,7 +16,7 @@ public static bool IsLiteralNull(this BoundExpression node)

		public static bool IsLiteralDefault(this BoundExpression node)

		@@ -3540,8 +3540,7 @@ public void TestTargetTypedDefault()
		var expr = this.ParseExpression(text, TestOptions.ExperimentalParseOptions);

Make "default" a literal and other tweaks from recent LDM #18213

Make "default" a literal and other tweaks from recent LDM #18213

Conversation

jcouv commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Mar 29, 2017

jcouv commented Mar 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 4, 2017

jcouv commented Apr 4, 2017

jcouv Mar 28, 2017 •

edited

Loading