Prototype: target typed "default" #13603

jcouv · 2016-09-04T21:16:24Z

Allow int x = default; and M(default); where the type of default is inferred.

Design notes (updated):

Introduces a new syntax node for target-typed default.
During initial binding, this is represented by a BoundDefaultOperator without a type.
A new kind of conversion is introduced (DefaultLiteral). "default" can convert to any struct and non-nullable type.
During lowering, this conversion produces a BoundDefaultOperator with the target type of the conversion.
"default" contributes to overload resolution, by excluding parameters that are nullable.
Some language question recorded in Prototype: target typed "default" #13602

CC @dotnet/roslyn-compiler for review. This is low-pri (not for C# 7).

gafter · 2016-09-05T03:29:53Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

@@ -918,6 +918,21 @@
      <summary>Creates an DefaultExpressionSyntax node.</summary>
    </FactoryComment>
  </Node>
+  <Node Name="TargetTypedDefaultExpressionSyntax" Base="ExpressionSyntax">


TargetTypedDefaultExpressionSyntax [](start = 14, length = 34)

I suggest a more likely name in the grammar is "default literal". #Resolved

That sounds better ;-) I was just hesitant to use the term literal. #Resolved

gafter · 2016-09-05T03:36:11Z

These cases should be unified. The problem is that the left-hand-side doesn't have a type. Use BoundExpression.Display for the string form of the problematic expression.

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:4864 in 14592f6. [](commit_id = 14592f6, deletion_comment = False)

gafter · 2016-09-05T03:37:04Z

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

@@ -114,7 +114,7 @@ private static ImmutableArray<MethodSymbol> GetOriginalMethods(OverloadResolutio
            boundExpression.WasCompilerGenerated = true;

            var analyzedArguments = AnalyzedArguments.GetInstance();
-            Debug.Assert(!args.Any(e => e.Kind == BoundKind.OutVarLocalPendingInference || e.Kind == BoundKind.OutDeconstructVarPendingInference));
+            Debug.Assert(!args.Any(e => e.Kind == BoundKind.OutVarLocalPendingInference || e.Kind == BoundKind.OutDeconstructVarPendingInference || e.Kind == BoundKind.DefaultPendingInference));
            analyzedArguments.Arguments.AddRange(args);


In a bad invocation, it should be capable of remaining in the bound tree. #Resolved

gafter · 2016-09-12T18:56:44Z

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

@@ -4830,6 +4835,15 @@ private bool IsUsingAliasInScope(string name)
                return BadExpression(node, boundLeft);
            }

+            // PROTOTYPE(default) unify with case above
+            // No member accesses on default


I would expect this case (and the following one handling UnboundLambda) to be handled in the call to MakeMemberAccessValue. That call should never return a typeless expression.

I looked into this and it was thorny.
What I tried was to have MakeMemberAccessValue return a BadExpression and report an error for those two cases. But it made the diagnostics significantly worse in a number of cases (especially lambda). I'll stop by to discuss tomorrow.

gafter · 2016-09-12T19:14:10Z

jcouv · 2016-09-16T16:57:03Z

@AlekseyTs @cston If you had some time, would you mind doing a second code review. This is going to features/default branch.

cston · 2016-09-19T16:46:40Z

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

+            if (boundLeft.IsLiteralDefault())
+            {
+                DiagnosticInfo diagnosticInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadUnaryOp, SyntaxFacts.GetText(operatorToken.Kind()), "default");
+                diagnostics.Add(new CSDiagnostic(diagnosticInfo, operatorToken.GetLocation()));


diagnostics.Add(ErrorCode.ERR_BadUnaryOp, ...);

cston · 2016-09-19T18:22:41Z

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

+                //    return;
+                //}
+
+                if (!targetType.IsStructType() || targetType.IsNullableType())


Would targetType.IsReferenceType cover both type parameters and actual types?

Why not support default for reference types?

Whether default should be allowed on reference types is listed as an open LDM question. But the more I discuss with folks and the clearer it is that folks want to allow it. I'll remove this check.

cston · 2016-09-20T05:30:47Z

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

+                return Conversion.DefaultLiteral;
+            }
+
+            return Conversion.NoConversion;


Why are valid conversions limited to struct types?

The idea was that you should use null for ref types and default for struct types. But there is also an argument that default should be allowed anywhere default(type) was previous allowed.
I'll remove

cston · 2016-09-20T05:32:51Z

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

@@ -14,6 +14,11 @@ public static bool IsLiteralNull(this BoundExpression node)
            return node.Kind == BoundKind.Literal && node.ConstantValue.Discriminator == ConstantValueTypeDiscriminator.Null;
        }

+        public static bool IsLiteralDefault(this BoundExpression node)
+        {
+            return node.Kind == BoundKind.DefaultOperator && (object)node.Type == null;


Will node.Type == null in error situations such as default(System)? Should this method check syntax instead?

I tried that and found that node.Type will be ErrorType in that scenario.

cston · 2016-09-20T05:43:15Z

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

+";
+            var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.ExperimentalParseOptions, options: TestOptions.DebugExe);
+            comp.VerifyEmitDiagnostics();
+            CompileAndVerify(comp, expectedOutput: "0");


What is the type of default in int x = (short)default;?

Good question. I expect it should be short. I'll put a PROTOTYPE test to remind to check that once semantic model work is done.

jcouv · 2016-09-20T18:11:07Z

@gafter @cston, I removed the constraint that default should only work on struct types.

jcouv · 2016-09-20T22:42:56Z

@cston Would you have time to finish the review? The review feedback so far was addressed :-)

cston · 2016-09-21T17:23:44Z

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

+            // No member accesses on default
+            if (boundLeft.IsLiteralDefault())
+            {
+                DiagnosticInfo diagnosticInfo = new CSDiagnosticInfo(ErrorCode.ERR_BadUnaryOp, SyntaxFacts.GetText(operatorToken.Kind()), "default");
                diagnostics.Add(new CSDiagnostic(diagnosticInfo, operatorToken.GetLocation()));


diagnostics.Add(ErrorCode.ERR_BadUnaryOp, ...);

Fixed. Thanks

cston · 2016-09-21T17:28:55Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

+      <summary>Class which represents the syntax node for a default literal.</summary>
+    </TypeComment>
+    <FactoryComment>
+      <summary>Creates an DefaultLiteralSyntax node.</summary>


Creates a ...

Fixed this one and others

cston · 2016-09-21T17:36:31Z

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

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


Minor point: int? is not a reference type.

cston · 2016-09-21T17:46:33Z

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

+                //         default.ToString();
+                Diagnostic(ErrorCode.ERR_BadUnaryOp, ".").WithArguments(".", "default").WithLocation(6, 16)
+                );
+        }


Perhaps test: default[0], nameof(default), throw default;

I always love test ideas :-) Added!

cston · 2016-09-21T17:51:50Z

LGTM

jcouv · 2017-02-02T20:12:26Z

@gafter This is the first part of the prototype on "default", which you had reviewed. The second part is #16425

gafter · 2017-02-03T04:48:40Z

Oh yeah

Conflicts: src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionKind.cs src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionKindExtensions.cs src/Compilers/CSharp/Portable/Errors/MessageID.cs src/Compilers/CSharp/Portable/Parser/LanguageParser.cs src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt src/Compilers/CSharp/Portable/Syntax/Syntax.xml src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs src/Compilers/CSharp/Test/Semantic/CSharpCompilerSemanticTest.csproj src/Compilers/CSharp/Test/Syntax/Parsing/ExpressionParsingTests.cs src/Test/Utilities/Desktop/TestResource.resx src/Test/Utilities/Shared/Traits/CompilerFeature.cs

jcouv added 2 commits September 3, 2016 14:08

Parsing for target-typed default expression

611fd9f

Binding for target-typed default

ba9090a

jcouv added Language-C# Area-Compilers labels Sep 4, 2016

jcouv self-assigned this Sep 4, 2016

dnfclas added the cla-already-signed label Sep 4, 2016

jcouv force-pushed the features/default branch 2 times, most recently from e8ebe81 to 78446ac Compare September 4, 2016 22:04

Overload resolution and various tests

14592f6

jcouv force-pushed the features/default branch from 78446ac to 14592f6 Compare September 4, 2016 22:11

gafter reviewed Sep 5, 2016
View reviewed changes

Feedback for PR (1)

3a0e1a2

jcouv force-pushed the features/default branch from 4d9a6ca to 3a0e1a2 Compare September 6, 2016 04:34

More tests

293bc3d

gafter reviewed Sep 12, 2016
View reviewed changes

gafter removed their assignment Sep 12, 2016

PR feedback (1)

d89b2e9

jcouv force-pushed the features/default branch 2 times, most recently from 201c702 to d89b2e9 Compare September 13, 2016 19:49

cston reviewed Sep 19, 2016

View reviewed changes

PR feedback (1)

91fbc91

cston reviewed Sep 20, 2016

View reviewed changes

PR feedback (2)

b56e513

jcouv force-pushed the features/default branch from 782cb20 to b56e513 Compare September 20, 2016 18:12

cston reviewed Sep 21, 2016

View reviewed changes

PR feedback (3)

b0dc521

jcouv merged commit 4ecd4cd into dotnet:features/default Sep 21, 2016

jcouv deleted the features/default branch September 21, 2016 19:40

jcouv mentioned this pull request Feb 2, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prototype: target typed "default" #13603

Prototype: target typed "default" #13603

jcouv commented Sep 4, 2016 •

edited

Loading

gafter Sep 5, 2016 •

edited

Loading

jcouv Sep 5, 2016 •

edited by gafter

Loading

gafter commented Sep 5, 2016

gafter Sep 5, 2016 •

edited

Loading

gafter Sep 12, 2016

jcouv Sep 13, 2016

gafter commented Sep 12, 2016

jcouv commented Sep 16, 2016

cston Sep 19, 2016

jcouv Sep 20, 2016

cston Sep 19, 2016

cston Sep 20, 2016

jcouv Sep 20, 2016

cston Sep 20, 2016

jcouv Sep 20, 2016

cston Sep 20, 2016

jcouv Sep 20, 2016

cston Sep 20, 2016

jcouv Sep 20, 2016

jcouv commented Sep 20, 2016

jcouv commented Sep 20, 2016

cston Sep 21, 2016

jcouv Sep 21, 2016

cston Sep 21, 2016

jcouv Sep 21, 2016

cston Sep 21, 2016

jcouv Sep 21, 2016

cston Sep 21, 2016

jcouv Sep 21, 2016

cston commented Sep 21, 2016

jcouv commented Feb 2, 2017

gafter commented Feb 3, 2017

Prototype: target typed "default" #13603

Prototype: target typed "default" #13603

Conversation

jcouv commented Sep 4, 2016 • edited Loading

gafter Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

jcouv Sep 5, 2016 • edited by gafter Loading

Choose a reason for hiding this comment

gafter commented Sep 5, 2016

gafter Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Sep 12, 2016

jcouv commented Sep 16, 2016

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 commented Sep 20, 2016

jcouv commented Sep 20, 2016

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

cston commented Sep 21, 2016

jcouv commented Feb 2, 2017

gafter commented Feb 3, 2017

jcouv commented Sep 4, 2016 •

edited

Loading

gafter Sep 5, 2016 •

edited

Loading

jcouv Sep 5, 2016 •

edited by gafter

Loading

gafter Sep 5, 2016 •

edited

Loading