More tests and suggestions on default literal #18604

jcouv · 2017-04-11T17:42:06Z

@cston I'll stop by to discuss

cston · 2017-04-11T23:37:11Z

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

+";
+
+            var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular7_1, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics(); // Should give an error. Follow-up issue: https://github.com/dotnet/roslyn/issues/18609


using (null) { } and using (default(System.IDisposable)) { } do not generate errors.

cston · 2017-04-11T23:43:20Z

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

+                //         F(async () => await default);
+                Diagnostic(ErrorCode.ERR_CantInferMethTypeArgs, "F").WithArguments("C.F<T>(System.Threading.Tasks.Task<T>)").WithLocation(8, 9)
+                );
+        }


Consider testing:

using System.Threading.Tasks; class C { static async Task<T> F<T>() { await Task.Delay(0); return default; } }

cston · 2017-04-12T04:25:10Z

LGTM

jcouv · 2017-04-12T16:46:47Z

@dotnet/roslyn-compiler for review. This is mostly adding tests from discussion with Chuck so that I can merge "default" back into master.

cston · 2017-04-13T20:59:32Z

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

+";
+            var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular7_1, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics();
+            CompileAndVerify(comp, expectedOutput: "reached");


Perhaps add Console.WriteLine to operator==.

gafter

A couple of issues noted in this review:

gafter · 2017-04-13T21:32:24Z

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

+                // (14,17): error CS1031: Type expected
+                //         default();
+                Diagnostic(ErrorCode.ERR_TypeExpected, ")").WithLocation(14, 17),
+                // (6,17): error CS8119: The switch expression must be a value; found default.


I think the word default in this diagnostic should have some kind of quotes around it. It isn't a word of the sentence, it is referring to syntax in the source. #Resolved

gafter · 2017-04-13T21:35:05Z

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

+                // (16,19): error CS1059: The operand of an increment or decrement operator must be a variable, property or indexer
+                //         int i = ++default;
+                Diagnostic(ErrorCode.ERR_IncrementLvalueExpected, "default"),
+                // (17,26): error CS0828: Cannot assign default to anonymous type property


Here too. #Resolved

gafter · 2017-04-13T21:49:16Z

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

+";
+            var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular7_1, options: TestOptions.DebugExe);
+            comp.VerifyDiagnostics(
+                // (11,13): error CS0023: Operator '!' cannot be applied to operand of type 'default'


Looking at the specification, this is incorrect:

For an operation of the form !x, unary operator overload resolution (§13.4.4) is applied to select a specific operator implementation. The operand is converted to the parameter type of the selected operator, and the type of the result is the return type of the operator. Only one predefined logical negation operator exists:

bool operator !(bool x);

This operator computes the logical negation of the operand: If the operand is true, the result is false. If the operand is false, the result is true.

Lifted (§13.4.8) forms of the unlifted predefined logical negation operator defined above are also predefined.
#Resolved

gafter · 2017-04-13T21:55:27Z

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

+    {
+        M();
+    }
+    static void M(System.Threading.CancellationToken x = default)


YES! #Resolved

jcouv · 2017-04-13T23:54:58Z

@cston This is ready for review.
Fixed EE scenario, added some tests, fixed unary operator case. It turns out the problem with default in interpolated strings and in using also repro'es with null (tracked by follow-up issue).

cston · 2017-04-14T04:04:12Z

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

@@ -2180,7 +2180,7 @@ private BoundExpression BindUnaryOperatorCore(CSharpSyntaxNode node, string oper
        {
            UnaryOperatorKind kind = SyntaxKindToUnaryOperatorKind(node.Kind());

-            bool isOperandTypeNull = (object)operand.Type == null;
+            bool isOperandTypeNull = operand.IsLiteralNull();
            if (isOperandTypeNull)


Are we reporting an error for an operand with no type, for instance BoundMethodGroup?

Yes, added test for method group and null.

cston · 2017-04-14T04:07:47Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/LocalsTests.cs

+                Assert.Equal(default(DkmEvaluationResultCategory), resultProperties.Category); // Not Data
+                Assert.Equal(default(DkmEvaluationResultAccessType), resultProperties.AccessType); // Not Public
+                Assert.Equal(default(DkmEvaluationResultStorageType), resultProperties.StorageType);
+                Assert.Equal(default(DkmEvaluationResultTypeModifierFlags), resultProperties.ModifierFlags); // Not Virtual


Please call VerifyIL().

cston · 2017-04-14T04:10:04Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/LocalsTests.cs

+
+                var testData3 = new CompilationTestData();
+                context.CompileExpression("default", DkmEvaluationFlags.None, ImmutableArray<Alias>.Empty, out error, testData3);
+                Assert.Null(error);


What is the target type?

It's object.
The reason is that the EE adds a conversion:

private static BoundStatement BindExpression(Binder binder, ExpressionSyntax syntax, DiagnosticBag diagnostics, out ResultProperties resultProperties) { ... var expressionType = expression.Type; if ((object)expressionType == null) { expression = binder.CreateReturnConversion( syntax, diagnostics, expression, RefKind.None, binder.Compilation.GetSpecialType(SpecialType.System_Object)); if (diagnostics.HasAnyErrors()) { resultProperties = default(ResultProperties); return null; } }

Thanks. Consider adding Assert.Equal(SpecialType.Object, method.ReturnType.SpecialType); here in that case.

cston · 2017-04-14T04:18:46Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/LocalsTests.cs

@@ -3766,6 +3766,65 @@ static void Main()
            });
        }

+        [Fact]
+        public void AssignDefaultToLocal()


Consider moving to ExpressionCompilerTests. LocalsTests was, at least initially, intended for testing CompileGetLocals.

gafter

LGTM

cston · 2017-04-14T20:07:39Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/SyntaxHelpers.cs

@@ -210,7 +210,7 @@ private static InternalSyntax.ExpressionSyntax ParseDebuggerExpressionInternal(S
        private static StatementSyntax ParseDebuggerStatement(string text)
        {
            var source = SourceText.From(text);
-            using (var lexer = new InternalSyntax.Lexer(source, CSharpParseOptions.Default))
+            using (var lexer = new InternalSyntax.Lexer(source, CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest)))


Please create a helper method for ParseOptions.

cston · 2017-04-14T20:08:29Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs

+}");
+
+                testData = new CompilationTestData();
+                context.CompileExpression("a = default;", DkmEvaluationFlags.None, ImmutableArray<Alias>.Empty, out error, testData);


Consider testing a non-expression statement (perhaps "int b = default;") to avoid having these succeed by falling back to expression parsing.

cston · 2017-04-14T20:21:21Z

src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/SyntaxHelpers.cs

@@ -194,9 +194,11 @@ private static ExpressionSyntax ParseDebuggerExpression(string text, bool consum
            return expression.MakeDebuggerExpression(source);
        }

+        static CSharpParseOptions GetCSharpParseOptions() => CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest);


Consider making this a static readonly field.

cston · 2017-04-14T20:21:46Z

LGTM

jcouv added 2 commits April 11, 2017 10:36

More tests on default literal

7437e5b

Feedback on default literal

f03bb81

jcouv self-assigned this Apr 11, 2017

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

jcouv added Area-Compilers and removed cla-already-signed labels Apr 11, 2017

jcouv added 2 commits April 11, 2017 13:56

More suggested tests

023c00d

Test speculative model on default

da8a4ce

cston reviewed Apr 11, 2017

View reviewed changes

Test returning default from async method

48f7f42

jcouv assigned gafter and unassigned gafter Apr 12, 2017

jcouv added 2 commits April 13, 2017 11:24

Add test for when(false)

56acf31

Add tests from team review

075a817

cston reviewed Apr 13, 2017

View reviewed changes

Test refinements

223155a

gafter suggested changes Apr 13, 2017

View reviewed changes

gafter removed their assignment Apr 13, 2017

jcouv added 2 commits April 13, 2017 15:38

Add EE tests and make EE use LanguageVersion.Latest

47a1ef3

Address feedback from Neal

e58ed32

Add comparison tests with null

880e32c

jcouv assigned gafter Apr 14, 2017

cston reviewed Apr 14, 2017

View reviewed changes

jcouv force-pushed the default-tests branch from d642592 to 019f190 Compare April 14, 2017 17:40

Move EE test and address feedback from Chuck

9b752ec

jcouv force-pushed the default-tests branch from 019f190 to 9b752ec Compare April 14, 2017 17:42

gafter approved these changes Apr 14, 2017

View reviewed changes

gafter removed their assignment Apr 14, 2017

cston reviewed Apr 14, 2017

View reviewed changes

Add helper method and test EE with a proper statement

d608da9

cston reviewed Apr 14, 2017

View reviewed changes

Using static field instead

587e7a8

jcouv merged commit 3f19fb5 into dotnet:features/default Apr 14, 2017

jcouv deleted the default-tests branch April 14, 2017 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More tests and suggestions on default literal #18604

More tests and suggestions on default literal #18604

jcouv commented Apr 11, 2017

cston Apr 11, 2017

cston Apr 11, 2017

cston commented Apr 12, 2017

jcouv commented Apr 12, 2017

cston Apr 13, 2017

gafter left a comment

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

jcouv commented Apr 13, 2017

cston Apr 14, 2017

jcouv Apr 14, 2017

cston Apr 14, 2017

cston Apr 14, 2017

jcouv Apr 14, 2017

cston Apr 14, 2017

cston Apr 14, 2017

gafter left a comment

cston Apr 14, 2017

cston Apr 14, 2017

cston Apr 14, 2017

cston commented Apr 14, 2017

More tests and suggestions on default literal #18604

More tests and suggestions on default literal #18604

Conversation

jcouv commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Apr 12, 2017

jcouv commented Apr 12, 2017

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

gafter Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

jcouv commented Apr 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

Choose a reason for hiding this comment

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

cston commented Apr 14, 2017

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading

gafter Apr 13, 2017 •

edited

Loading