PR feedback (1)

dotnet · Nov 8, 2016 · 7400a9f · 7400a9f
1 parent 31e9421
commit 7400a9f
Show file tree

Hide file tree

Showing 16 changed files with 127 additions and 75 deletions.
diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
@@ -16,6 +16,10 @@ namespace Microsoft.CodeAnalysis.CSharp
     /// </summary>
     internal partial class Binder
     {
+        /// <summary>
+        /// Only handles assignment-only or declaration-only deconstructions at this point.
+        /// Issue https://github.com/dotnet/roslyn/issues/15050 tracks allowing mixed deconstructions
+        /// </summary>
         private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
         {
             var left = node.Left;
@@ -26,7 +30,6 @@ private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Diag
                 return BindDeconstructionDeclaration(node, left, right, diagnostics);
             }
 
-            // We only parse assignment-only or declaration-only deconstructions at this point
             AssertDeconstructionIsAssignment(left);
 
             var tuple = (TupleExpressionSyntax)left;

diff --git a/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs b/src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs
@@ -445,6 +445,15 @@ protected override LocalSymbol MakeOutVariable(DeclarationExpressionSyntax node,
                                             SingleVariableDesignationSyntax designation,
                                             AssignmentExpressionSyntax deconstruction)
         {
+            NamedTypeSymbol container = _scopeBinder.ContainingType;
+
+            if ((object)container != null && container.IsScriptClass &&
+                (object)_scopeBinder.LookupDeclaredField(designation) != null)
+            {
+                // This is a field declaration
+                return null;
+            }
+
             return SourceLocalSymbol.MakeDeconstructionLocal(
                                       containingSymbol: _scopeBinder.ContainingMemberOrLambda,
                                       scopeBinder: _scopeBinder,

diff --git a/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs b/src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs
@@ -252,17 +252,6 @@ private static Binder GetEnclosingBinder(SyntaxNode node, int position, Binder r
                 {
                     binder = rootBinder.GetBinder(current);
                 }
-                else if ((kind == SyntaxKind.DeclarationExpression || kind == SyntaxKind.TupleExpression) &&
-                             (current.Parent as ForEachVariableStatementSyntax)?.Variable == current)
-                {
-                    binder = rootBinder.GetBinder(current.Parent);
-                }
-                else if ((kind == SyntaxKind.DeclarationExpression || kind == SyntaxKind.TupleExpression) &&
-                             (current.Parent is AssignmentExpressionSyntax) &&
-                             (current.Parent.Parent as ForStatementSyntax)?.Initializers.Contains(current.Parent) == true)
-                {
-                    binder = rootBinder.GetBinder(current.Parent.Parent);
-                }
                 else
                 {
                     // If this ever breaks, make sure that all callers of
@@ -321,7 +310,8 @@ private static Binder AdjustBinderForPositionWithinStatement(int position, Binde
                 case SyntaxKind.ForEachStatement:
                 case SyntaxKind.ForEachVariableStatement:
                     var foreachStmt = (CommonForEachStatementSyntax)stmt;
-                    if (LookupPosition.IsBetweenTokens(position, foreachStmt.OpenParenToken, foreachStmt.Statement.GetFirstToken()))
+                    var start = stmt.Kind() == SyntaxKind.ForEachVariableStatement ? foreachStmt.InKeyword : foreachStmt.OpenParenToken;
+                    if (LookupPosition.IsBetweenTokens(position, start, foreachStmt.Statement.GetFirstToken()))
                     {
                         binder = binder.GetBinder(foreachStmt.Expression);
                         Debug.Assert(binder != null);
@@ -1457,7 +1447,7 @@ private static Binder GetQueryEnclosingBinder(int position, CSharpSyntaxNode sta
                 }
             }
 
-            done:
+done:
             return GetEnclosingBinder(AdjustStartingNodeAccordingToNewRoot(startingNode, queryClause.Syntax),
                                       position, queryClause.Binder, queryClause.Syntax);
         }
@@ -1690,28 +1680,34 @@ internal protected virtual CSharpSyntaxNode GetBindableSyntaxNode(CSharpSyntaxNo
         /// If this declaration is part of a deconstruction, find the deconstruction.
         /// Returns null otherwise.
         /// </summary>
-        private AssignmentExpressionSyntax GetContainingDeconstruction(ExpressionSyntax expr)
+        private static AssignmentExpressionSyntax GetContainingDeconstruction(ExpressionSyntax expr)
         {
-            Debug.Assert(expr.Kind() == SyntaxKind.TupleExpression || expr.Kind() == SyntaxKind.DeclarationExpression);
-
-            if (expr.Parent.Kind() == SyntaxKind.Argument)
+            while (true)
             {
-                if (expr.Parent.Parent.Kind() == SyntaxKind.TupleExpression)
+                Debug.Assert(expr.Kind() == SyntaxKind.TupleExpression || expr.Kind() == SyntaxKind.DeclarationExpression);
+                var parent = expr.Parent;
+                if (parent == null) { return null; }
+
+                if (parent.Kind() == SyntaxKind.Argument)
                 {
-                    return GetContainingDeconstruction((TupleExpressionSyntax)expr.Parent.Parent);
+                    if (parent.Parent?.Kind() == SyntaxKind.TupleExpression)
+                    {
+                        expr = (TupleExpressionSyntax)parent.Parent;
+                        continue;
+                    }
+                    else
+                    {
+                        return null;
+                    }
                 }
-                else
+                else if (parent.Kind() == SyntaxKind.SimpleAssignmentExpression &&
+                        (object)((AssignmentExpressionSyntax)parent).Left == expr)
                 {
-                    return null;
+                    return (AssignmentExpressionSyntax)parent;
                 }
-            }
-            else if (expr.Parent.Kind() == SyntaxKind.SimpleAssignmentExpression &&
-                    (object)((AssignmentExpressionSyntax)expr.Parent).Left == expr)
-            {
-                return (AssignmentExpressionSyntax)expr.Parent;
-            }
 
-            return null;
+                return null;
+            }
         }
 
         /// <summary>

diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/VariablesDeclaredWalker.cs
@@ -170,10 +170,14 @@ protected override void VisitLvalue(BoundLocal node)
         private void CheckOutVarDeclaration(BoundLocal node)
         {
             if (IsInside &&
-                !node.WasCompilerGenerated && node.Syntax.Kind() == SyntaxKind.DeclarationExpression &&
-                ((SingleVariableDesignationSyntax)((DeclarationExpressionSyntax)node.Syntax).Designation).Identifier == node.LocalSymbol.IdentifierToken)
+                !node.WasCompilerGenerated && node.Syntax.Kind() == SyntaxKind.DeclarationExpression)
             {
-                _variablesDeclared.Add(node.LocalSymbol);
+                var declaration = (DeclarationExpressionSyntax)node.Syntax;
+                if (((SingleVariableDesignationSyntax)declaration.Designation).Identifier == node.LocalSymbol.IdentifierToken &&
+                    ((ArgumentSyntax)declaration.Parent).RefOrOutKeyword.Kind() == SyntaxKind.OutKeyword)
+                {
+                    _variablesDeclared.Add(node.LocalSymbol);
+                }
             }
         }
 

diff --git a/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs b/src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
@@ -11,6 +11,7 @@
 namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax
 {
     using Microsoft.CodeAnalysis.Syntax.InternalSyntax;
+    using System.Linq;
 
     internal partial class LanguageParser : SyntaxParser
     {
@@ -7534,6 +7535,19 @@ private StatementSyntax ParseEmbeddedStatement(bool complexCheck)
                 case SyntaxKind.LocalFunctionStatement:
                     statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt);
                     break;
+                case SyntaxKind.ExpressionStatement:
+                    // Deconstruction-declaration is only allowed as top-level statement
+                    // see https://github.com/dotnet/roslyn/issues/15049
+                    var expression = ((ExpressionStatementSyntax)statement).Expression;
+                    if (expression.Kind == SyntaxKind.SimpleAssignmentExpression)
+                    {
+                        var assignment = (AssignmentExpressionSyntax)expression;
+                        if (assignment.Left.EnumerateNodes().Any(x => x.RawKind == (int)SyntaxKind.DeclarationExpression))
+                        {
+                            statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt);
+                        }
+                    }
+                    break;
             }
 
             return statement;

diff --git a/src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt b/src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt
@@ -157,17 +157,17 @@ Microsoft.CodeAnalysis.CSharp.SyntaxKind.CasePatternSwitchLabel = 9009 -> Micros
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.ConstantPattern = 9002 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.DeclarationExpression = 9040 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.DeclarationPattern = 9000 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.ForEachVariableStatement = 8934 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.ForEachVariableStatement = 8929 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.IsPatternExpression = 8657 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.LocalFunctionStatement = 8830 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.ParenthesizedVariableDesignation = 8931 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.ParenthesizedVariableDesignation = 8928 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.RefExpression = 9050 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.RefType = 9051 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.SingleVariableDesignation = 8930 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.SingleVariableDesignation = 8927 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.ThrowExpression = 9052 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleElement = 8926 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleExpression = 8927 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
-Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleType = 8925 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleElement = 8925 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleExpression = 8926 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
+Microsoft.CodeAnalysis.CSharp.SyntaxKind.TupleType = 8924 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 Microsoft.CodeAnalysis.CSharp.SyntaxKind.WhenClause = 9013 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
 abstract Microsoft.CodeAnalysis.CSharp.Syntax.BaseMethodDeclarationSyntax.ExpressionBody.get -> Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax
 abstract Microsoft.CodeAnalysis.CSharp.Syntax.CommonForEachStatementSyntax.CloseParenToken.get -> Microsoft.CodeAnalysis.SyntaxToken

diff --git a/src/Compilers/CSharp/Portable/Syntax/Syntax.xml b/src/Compilers/CSharp/Portable/Syntax/Syntax.xml
@@ -2110,7 +2110,10 @@
     <Field Name="OpenParenToken" Type="SyntaxToken" Override="true">
       <Kind Name="OpenParenToken"/>
     </Field>
-    <Field Name="Variable" Type="ExpressionSyntax"/>
+    <Field Name="Variable" Type="ExpressionSyntax">
+      <Kind Name="DeclarationExpression"/>
+      <Kind Name="TupleExpression"/>
+    </Field>
     <Field Name="InKeyword" Type="SyntaxToken" Override="true">
       <Kind Name="InKeyword"/>
     </Field>

diff --git a/src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs b/src/Compilers/CSharp/Portable/Syntax/SyntaxExtensions.cs
@@ -206,23 +206,26 @@ internal static SyntaxNode SkipParens(this SyntaxNode expression)
             return expression;
         }
 
-        private static bool IsDeconstructionDeclaration(this ExpressionSyntax self)
+        /// <summary>
+        /// Is this expression composed only of declaration expressions nested in tuple expressions?
+        /// </summary>
+        private static bool IsDeconstructionDeclarationLeft(this ExpressionSyntax self)
         {
             switch (self.Kind())
             {
                 case SyntaxKind.DeclarationExpression:
                     return true;
                 case SyntaxKind.TupleExpression:
                     var tuple = (TupleExpressionSyntax)self;
-                    return tuple.Arguments.All(a => IsDeconstructionDeclaration(a.Expression));
+                    return tuple.Arguments.All(a => IsDeconstructionDeclarationLeft(a.Expression));
                 default:
                     return false;
             }
         }
 
         internal static bool IsDeconstructionDeclaration(this AssignmentExpressionSyntax self)
         {
-            return self.Left.IsDeconstructionDeclaration(); 
+            return self.Left.IsDeconstructionDeclarationLeft();
         }
 
         private static bool IsInContextWhichNeedsDynamicAttribute(CSharpSyntaxNode node)

diff --git a/src/Compilers/CSharp/Portable/Syntax/SyntaxFacts.cs b/src/Compilers/CSharp/Portable/Syntax/SyntaxFacts.cs
@@ -401,9 +401,8 @@ internal static bool IsVarOrPredefinedType(this Syntax.InternalSyntax.SyntaxToke
 
         internal static bool IsDeclarationExpressionType(SyntaxNode node, out DeclarationExpressionSyntax parent)
         {
-            var component = node.Parent as DeclarationExpressionSyntax;
-            parent = component as DeclarationExpressionSyntax;
-            return node == component?.Type;
+            parent = node.Parent as DeclarationExpressionSyntax;
+            return node == parent?.Type;
         }
     }
 }
diff --git a/src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs b/src/Compilers/CSharp/Portable/Syntax/SyntaxKind.cs
@@ -539,12 +539,12 @@ public enum SyntaxKind : ushort
         // Changes after C# 6
 
         // tuples
-        TupleType = 8925,
-        TupleElement = 8926,
-        TupleExpression = 8927,
-        SingleVariableDesignation = 8930,
-        ParenthesizedVariableDesignation = 8931,
-        ForEachVariableStatement = 8934,
+        TupleType = 8924,
+        TupleElement = 8925,
+        TupleExpression = 8926,
+        SingleVariableDesignation = 8927,
+        ParenthesizedVariableDesignation = 8928,
+        ForEachVariableStatement = 8929,
 
         // patterns (for pattern-matching)
         DeclarationPattern = 9000,

diff --git a/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs b/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs
@@ -1163,6 +1163,40 @@ public void Deconstruct(out int a, out int b)
                 );
         }
 
+        [Fact]
+        public void MixedDeconstructionCannotBeParsed()
+        {
+            string source = @"
+class C
+{
+    public static void Main()
+    {
+        int x;
+        (x, int y) = new C();
+    }
+
+    public void Deconstruct(out int a, out int b)
+    {
+        a = 1;
+        b = 2;
+    }
+}
+";
+
+            var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
+            comp.VerifyDiagnostics(
+                // (7,10): error CS1031: Type expected
+                //         (x, int y) = new C();
+                Diagnostic(ErrorCode.ERR_TypeExpected, "x").WithLocation(7, 10),
+                // (7,10): error CS0128: A local variable or function named 'x' is already defined in this scope
+                //         (x, int y) = new C();
+                Diagnostic(ErrorCode.ERR_LocalDuplicate, "x").WithArguments("x").WithLocation(7, 10),
+                // (6,13): warning CS0168: The variable 'x' is declared but never used
+                //         int x;
+                Diagnostic(ErrorCode.WRN_UnreferencedVar, "x").WithArguments("x").WithLocation(6, 13)
+                );
+        }
+
         [Fact]
         public void DeconstructionWithTupleNamesCannotBeParsed()
         {

diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/DeconstructionTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/DeconstructionTests.cs
@@ -2434,7 +2434,7 @@ static void Test(int arg1, (byte, byte) arg2)
         }
 
         [Fact]
-        public void DeclarationCanBeEmbedded()
+        public void DeclarationCannotBeEmbedded()
         {
             var source = @"
 class C1
@@ -2445,30 +2445,12 @@ void M()
             var (x, y) = (1, 2);
     }
 }
-";
-            var comp = CreateCompilationWithMscorlib(source, references: s_valueTupleRefs);
-            comp.VerifyDiagnostics();
-        }
-
-        [Fact]
-        public void EmbeddedDeclarationIsScoped()
-        {
-            var source = @"
-class C1
-{
-    void M()
-    {
-        if (true)
-            var (x, y) = (1, 2);
-        System.Console.WriteLine(x);
-    }
-}
 ";
             var comp = CreateCompilationWithMscorlib(source, references: s_valueTupleRefs);
             comp.VerifyDiagnostics(
-                // (8,34): error CS0103: The name 'x' does not exist in the current context
-                //         System.Console.WriteLine(x);
-                Diagnostic(ErrorCode.ERR_NameNotInContext, "x").WithArguments("x").WithLocation(8, 34)
+                // (7,13): error CS1023: Embedded statement cannot be a declaration or labeled statement
+                //             var (x, y) = (1, 2);
+                Diagnostic(ErrorCode.ERR_BadEmbeddedStmt, "var (x, y) = (1, 2);").WithLocation(7, 13)
                 );
         }
 

diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/OutVarTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/OutVarTests.cs
@@ -8981,6 +8981,9 @@ static bool TakeOutParam(object y, out object x)
             var compilation = CreateCompilationWithMscorlib45(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef },
                                                               options: TestOptions.DebugExe, parseOptions: TestOptions.Regular);
             compilation.VerifyDiagnostics(
+                // (11,13): error CS1023: Embedded statement cannot be a declaration or labeled statement
+                //             var (d, dd) = (TakeOutParam(true, out var x1), x1);
+                Diagnostic(ErrorCode.ERR_BadEmbeddedStmt, "var (d, dd) = (TakeOutParam(true, out var x1), x1);").WithLocation(11, 13),
                 // (13,9): error CS0103: The name 'x1' does not exist in the current context
                 //         x1++;
                 Diagnostic(ErrorCode.ERR_NameNotInContext, "x1").WithArguments("x1").WithLocation(13, 9)

diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_Scope.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests_Scope.cs
@@ -6756,6 +6756,9 @@ void Test1()
             var compilation = CreateCompilationWithMscorlib45(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef },
                                                               options: TestOptions.DebugExe, parseOptions: TestOptions.Regular);
             compilation.VerifyDiagnostics(
+                // (11,13): error CS1023: Embedded statement cannot be a declaration or labeled statement
+                //             var (d, dd) = ((true is var x1), x1);
+                Diagnostic(ErrorCode.ERR_BadEmbeddedStmt, "var (d, dd) = ((true is var x1), x1);").WithLocation(11, 13),
                 // (13,9): error CS0103: The name 'x1' does not exist in the current context
                 //         x1++;
                 Diagnostic(ErrorCode.ERR_NameNotInContext, "x1").WithArguments("x1").WithLocation(13, 9)

diff --git a/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs b/src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/EvaluationContext.cs
@@ -303,6 +303,7 @@ private static MethodSymbol GetSynthesizedMethod(CommonPEModuleBuilder moduleBui
                 statementDiagnostics.Free();
 
                 // Prefer to parse expression statements (except deconstruction-declarations) as expressions.
+                // Once https://github.com/dotnet/roslyn/issues/15049 is fixed, we should parse d-declarations as expressions.
                 var isExpressionStatement = statementSyntax.IsKind(SyntaxKind.ExpressionStatement);
                 var isDeconstructionDeclaration = isExpressionStatement &&
                                                   IsDeconstructionDeclaration((ExpressionStatementSyntax)statementSyntax);

diff --git a/src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs b/src/Workspaces/CSharp/Portable/LanguageServices/CSharpTypeInferenceService.TypeInferrer.cs
@@ -879,8 +879,6 @@ private IEnumerable<TypeInferenceInfo> InferTypeInBinaryOrAssignmentExpression(E
                 if (operatorToken.Kind() == SyntaxKind.EqualsToken &&
                     (left.Kind() == SyntaxKind.TupleExpression || left.Kind() == SyntaxKind.DeclarationExpression))
                 {
-                    // TODO REVIEW Once GetTypeInfo works on the left-hand-side expression in a deconstruction declaration,
-                    // this may not be needed
                     return InferTypeInVariableComponentAssignment(left);
                 }