Prototype for "default": semantic model, constant value and more tests #16425

jcouv · 2017-01-11T06:23:53Z

Continuing prototyping for #13602

Adding some tests for semantic model
Allow target-typed literal to be seen as a constant
Adding tests corresponding to scenarios where null gets special treatment (throw null, null == null, null as Some Type, ...)

As a refresher, the first part of this prototype work was in PR #13603

CC @dotnet/roslyn-compiler for review. This is for features/default branch. No rush.

jcouv · 2017-01-27T21:36:34Z

@dotnet/roslyn-compiler for review (post-RTM change).

jcouv · 2017-02-01T21:53:44Z

@gafter Could you do the first review, when you get some time?

gafter · 2017-02-02T01:27:47Z

Yes

gafter

The set of changes in this PR look good to me. However, it appears to be adding to an existing prototype that I haven't reviewed.

gafter · 2017-02-02T18:45:56Z

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

@@ -553,7 +553,7 @@ private BoundExpression BindExpressionInternal(ExpressionSyntax node, Diagnostic
                    return BindDefaultExpression((DefaultExpressionSyntax)node, diagnostics);

                case SyntaxKind.DefaultLiteral:
-                    return new BoundDefaultOperator((DefaultLiteralSyntax)node);
+                    return new BoundDefaultOperator((DefaultLiteralSyntax)node, ConstantValue.DefaultLiteral, type: null);


Please extract into method BindDefaultLiteral for consistency (all other expression forms delegate to a separate bind method).

Also, BoundDefaultOperator should probably be named BoundDefaultLiteral.

gafter · 2017-02-02T19:00:48Z

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

@@ -4834,7 +4832,6 @@ private bool IsUsingAliasInScope(string name)
                return BadExpression(node, boundLeft);
            }

-            // PROTOTYPE(default) unify with case above
            // No member accesses on default
            if (boundLeft.IsLiteralDefault())


I wonder if these two cases (default and unbound lambda) can be combined by testingfor (object)leftType == null and in that case report the diagnostic using boundLeft.Display.

I addressed your other feedback in latest commit, but I couldn't quite see how to factor as you suggested here.

gafter · 2017-02-02T19:04:51Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -975,7 +975,7 @@ internal static bool HasImplicitConstantExpressionConversion(BoundExpression sou
        {
            var constantValue = source.ConstantValue;

-            if (constantValue == null)
+            if (constantValue == null || constantValue.IsDefaultLiteral)


Perhaps change constantValue.IsDefaultLiteral to (object)source.Type == null?

jcouv · 2017-02-08T18:18:22Z

@cston I was hoping you could do the second review, since you had reviewed the first part of this prototype work. Thanks

cston · 2017-02-08T18:39:13Z

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

+        private static BoundExpression BindDefaultLiteral(DefaultLiteralSyntax node)
+        {
+            return new BoundDefaultLiteral(node, ConstantValue.DefaultLiteral, type: null);
+        }


Is (T)default a constant? Is const object o = (T)default; legal?

I will check. I have a test for int x = (short)default; (legal), but not with a generic cast.
My expectation is that (T)default should behave the same as default(T).

What about scenarios when it is of type user-defined struct? Is it supposed to have constant value then?

cston · 2017-02-08T18:56:04Z

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

@@ -588,7 +588,7 @@ public override BoundNode VisitNameOfOperator(BoundNameOfOperator node)

        public override BoundNode VisitNullCoalescingOperator(BoundNullCoalescingOperator node)
        {
-            if (_inExpressionLambda && node.LeftOperand.IsLiteralNull())
+            if (_inExpressionLambda && (node.LeftOperand.IsLiteralNull() || node.LeftOperand.IsLiteralDefault()))
            {
                Error(ErrorCode.ERR_ExpressionTreeContainsBadCoalesce, node.LeftOperand);


Is there a test for default?

Yes. All the tests are in TargetTypedDefaultTests.cs, which is updated as part of this PR (although GitHub doesn't expand it by default).

cston · 2017-02-08T20:41:35Z

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

+            var def = nodes.OfType<DefaultLiteralSyntax>().Single();
+            Assert.Null(model.GetTypeInfo(def).Type);
+            Assert.Null(model.GetSymbolInfo(def).Symbol);
+            Assert.Null(model.GetSymbolInfo(def).Symbol);


Duplicated line.

cston · 2017-02-08T20:44:40Z

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

+            comp.VerifyDiagnostics(
+                // (6,27): error CS0446: Foreach cannot operate on a 'default'. Did you intend to invoke the 'default'?
+                //         foreach (int x in default) { }
+                Diagnostic(ErrorCode.ERR_AnonMethGrpInForEach, "default").WithArguments("default").WithLocation(6, 27),


Did you intend to invoke the 'default'? seems odd.

cston · 2017-02-08T20:45:26Z

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

+            compilation.VerifyDiagnostics(
+                // (5,35): error CS0186: Use of null is not valid in this context
+                //         var q = from x in default select x;
+                Diagnostic(ErrorCode.ERR_NullNotValid, "select x").WithLocation(5, 35)


Should the message be Use of default is not valid ...?

Should this be ERR_DefaultNotValid?

cston · 2017-02-08T20:52:46Z

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

+                // (6,30): error CS0077: The as operator must be used with a reference type or nullable type ('long' is a non-nullable value type)
+                //         System.Console.Write(default as long);
+                Diagnostic(ErrorCode.ERR_AsMustHaveReferenceType, "default as long").WithArguments("long").WithLocation(6, 30)
+                );


Are there tests for default as C and default is C?

Are we testing nameof(default)?

Test for yield return default;?

InvocationOnDefault has nameof(default) (illegal). DefaultInAsOperator has default as long (illegal).
I'll add tests for the other suggestions.

cston · 2017-02-08T20:57:28Z

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

+            var def = nodes.OfType<DefaultLiteralSyntax>().Single();
+            Assert.Null(model.GetTypeInfo(def).Type);
+            Assert.Null(model.GetSymbolInfo(def).Symbol);
+            Assert.Null(model.GetSymbolInfo(def).Symbol);


Duplicated line.

gafter · 2017-02-09T20:42:03Z

@jcouv This needs a specification, please.

jcouv · 2017-02-13T20:31:27Z

Filed issue #17111 for writing a spec.

cston · 2017-02-13T21:52:27Z

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

+                // (8,15): error CS0283: The type 'S' cannot be declared const
+                //         const S y = (S)default;
+                Diagnostic(ErrorCode.ERR_BadConstType, "S").WithArguments("S").WithLocation(8, 15)
+                );


Are const object y = (T)default; or const object y = (S)default; allowed?

cston · 2017-02-13T21:54:54Z

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

+";
+            var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.ExperimentalParseOptions);
+            comp.VerifyDiagnostics();
+        }


Consider testing IEnumerable as well.

cston · 2017-02-13T21:59:37Z

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

+        }
+
+        [Fact]
+        public void CS0403ERR_TypeVarCanBeDefault()


The name of the method refers to an error, although no errors are reported.

cston · 2017-02-13T22:02:12Z

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

+            var conversion = nodes.OfType<CastExpressionSyntax>().Single();
+            var conversionTypeInfo = model.GetTypeInfo(conversion);
+            Assert.Equal("System.Int16", conversionTypeInfo.Type.ToTestDisplayString());
+            Assert.Equal("System.Int32", conversionTypeInfo.ConvertedType.ToTestDisplayString());


Is new object[default] supported?

Yes, see IndexingIntoArray which has x[default] (legal).

Oops, sorry, I misunderstood. I tested access, but not array construction. I'll add.

jcouv · 2017-02-13T23:17:23Z

Thanks for the test suggestions. I pushed another batch.

cston · 2017-02-14T18:14:27Z

LGTM

jcouv · 2017-02-14T18:30:11Z

Thanks for the review and feedback. I think this completes the implementation of the prototype.
I'll make sure to follow-up with a spec document.

dotnet#16425) Conflicts: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs src/Compilers/CSharp/Portable/Binder/Binder_Query.cs src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

jcouv added the Area-Compilers label Jan 11, 2017

jcouv self-assigned this Jan 11, 2017

dnfclas added the cla-already-signed label Jan 11, 2017

jcouv force-pushed the more-default branch from f4cb8f6 to c1ee4f7 Compare January 17, 2017 03:57

Test for semantic model, constant value and more tests for default

4dc80f1

jcouv force-pushed the more-default branch from c1ee4f7 to 4dc80f1 Compare January 20, 2017 03:38

jcouv requested a review from gafter January 23, 2017 23:01

jcouv assigned jcouv and gafter and unassigned jcouv Jan 27, 2017

gafter approved these changes Feb 2, 2017

View reviewed changes

jcouv mentioned this pull request Feb 2, 2017

Prototype: target typed "default" #13603

Merged

gafter removed their assignment Feb 3, 2017

PR feedback (1)

79effee

jcouv force-pushed the more-default branch from 1beae53 to 79effee Compare February 7, 2017 00:05

cston reviewed Feb 8, 2017

View reviewed changes

jcouv added the 4 - In Review A fix for the issue is submitted for review. label Feb 11, 2017

PR feedback (2)

89b5d07

cston reviewed Feb 13, 2017

View reviewed changes

jcouv closed this Feb 14, 2017

jcouv reopened this Feb 14, 2017

dnfclas added the cla-already-signed label Feb 14, 2017

More tests from feedback

28dba9e

jcouv force-pushed the more-default branch from 0f2b604 to 28dba9e Compare February 14, 2017 17:19

jcouv added 2 commits February 14, 2017 09:23

Adding test for array construction

79353be

Using wrong error message for default in query

af7bb3d

jcouv merged commit 582255b into dotnet:features/default Feb 14, 2017

jcouv deleted the more-default branch February 14, 2017 18:30

Prototype for "default": semantic model, constant value and more tests #16425

Prototype for "default": semantic model, constant value and more tests #16425

Conversation

jcouv commented Jan 11, 2017 • edited Loading

jcouv commented Jan 27, 2017

jcouv commented Feb 1, 2017

gafter commented Feb 2, 2017

gafter left a 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 commented Feb 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Feb 9, 2017

jcouv commented Feb 13, 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

jcouv commented Feb 13, 2017

cston commented Feb 14, 2017

jcouv commented Feb 14, 2017

jcouv commented Jan 11, 2017 •

edited

Loading