Merge pull request #9518 from cston/9131

Include HasValue check in BoundSwitchStatement

If the switch expression is a  Nullable<T>  a conditional branch is added for the  HasValue  check.
 That check was added as a separate  BoundStatement  outside of the  BoundSwitchStatement .
 If the lambda rewriter subsequently generated any display class instances, the allocations are added
 as a prologue to the rewritten switch statement but since the conditional branch is outside the switch
 statement, the branch skips the prologue, skipping the allocations. The change is to include the
 conditional branch in the  BoundSwitchStatement  so the branch occurs after the prologue.

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

@@ -296,7 +296,7 @@ internal override BoundSwitchStatement BindSwitchExpressionAndSections(SwitchSta
             // Bind switch section
             ImmutableArray<BoundSwitchSection> boundSwitchSections = BindSwitchSections(node.Sections, originalBinder, diagnostics);
 
-            return new BoundSwitchStatement(node, boundSwitchExpression, constantTargetOpt, Locals, boundSwitchSections, this.BreakLabel, null);
+            return new BoundSwitchStatement(node, null, boundSwitchExpression, constantTargetOpt, Locals, boundSwitchSections, this.BreakLabel, null);
         }
 
         // Bind the switch expression and set the switch governing type

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -670,6 +670,7 @@
   </Node>
 
   <Node Name="BoundSwitchStatement" Base="BoundStatement">
+    <Field Name="LoweredPreambleOpt" Type="BoundStatement" Null="allow"/>
     <Field Name="BoundExpression" Type="BoundExpression"/>
     <Field Name="ConstantTargetOpt" Type="LabelSymbol" Null= "allow"/>
     

src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs

@@ -5,12 +5,9 @@
 using System.Collections.Immutable;
 using System.Diagnostics;
 using System.Linq;
-using System.Reflection.Metadata;
 using Microsoft.CodeAnalysis.CodeGen;
-using Microsoft.CodeAnalysis.Collections;
 using Microsoft.CodeAnalysis.CSharp.Symbols;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
-using Microsoft.CodeAnalysis.Symbols;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
@@ -1023,6 +1020,12 @@ private void EmitCatchBlock(BoundCatchBlock catchBlock)
 
         private void EmitSwitchStatement(BoundSwitchStatement switchStatement)
         {
+            var preambleOpt = switchStatement.LoweredPreambleOpt;
+            if (preambleOpt != null)
+            {
+                EmitStatement(preambleOpt);
+            }
+
             // Switch expression must have a valid switch governing type
             Debug.Assert((object)switchStatement.BoundExpression.Type != null);
             Debug.Assert(switchStatement.BoundExpression.Type.IsValidSwitchGoverningType());
@@ -1564,11 +1567,12 @@ public override BoundNode VisitLabelStatement(BoundLabelStatement node)
             public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
             {
                 var breakLabelClone = GetLabelClone(node.BreakLabel);
+                var preambleOpt = (BoundStatement)this.Visit(node.LoweredPreambleOpt);
 
                 // expressions do not contain labels or branches
                 BoundExpression boundExpression = node.BoundExpression;
                 ImmutableArray<BoundSwitchSection> switchSections = (ImmutableArray<BoundSwitchSection>)this.VisitList(node.SwitchSections);
-                return node.Update(boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, breakLabelClone, node.StringEquality);
+                return node.Update(preambleOpt, boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, breakLabelClone, node.StringEquality);
             }
 
             public override BoundNode VisitSwitchLabel(BoundSwitchLabel node)

src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs

@@ -339,7 +339,7 @@ internal enum ExprContext
         Box
     }
 
-    // Analyses the tree trying to figure which locals may live on stack.
+    // Analyzes the tree trying to figure which locals may live on stack.
     // It is a fairly delicate process and must be very familiar with how CodeGen works.
     // It is essentially a part of CodeGen.
     //
@@ -1364,6 +1364,9 @@ public override BoundNode VisitUnaryOperator(BoundUnaryOperator node)
         public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
         {
             Debug.Assert(EvalStackIsEmpty());
+
+            var preambleOpt = (BoundStatement)this.Visit(node.LoweredPreambleOpt);
+
             DeclareLocals(node.InnerLocals, 0);
 
             // switch needs a byval local or a parameter as a key.
@@ -1396,7 +1399,7 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
                 this.RecordLabel(breakLabel);
             }
 
-            var result = node.Update(boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, breakLabel, node.StringEquality);
+            var result = node.Update(preambleOpt, boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, breakLabel, node.StringEquality);
 
             // implicit control flow
             EnsureOnlyEvalStack();

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncExceptionHandlerRewriter.cs

@@ -692,7 +692,7 @@ private void PopFrame()
         }
 
         /// <summary>
-        /// Analyses method body for try blocks with awaits in finally blocks 
+        /// Analyzes method body for try blocks with awaits in finally blocks 
         /// Also collects labels that such blocks contain.
         /// </summary>
         private sealed class AwaitInFinallyAnalysis : LabelCollector

src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AwaitExpressionSpiller.cs

@@ -508,9 +508,10 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
             EnterStatement(node);
 
             BoundSpillSequenceBuilder builder = null;
+            var preambleOpt = (BoundStatement)this.Visit(node.LoweredPreambleOpt);
             var boundExpression = VisitExpression(ref builder, node.BoundExpression);
             var switchSections = this.VisitList(node.SwitchSections);
-            return UpdateStatement(builder, node.Update(boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, node.BreakLabel, node.StringEquality), substituteTemps: true);
+            return UpdateStatement(builder, node.Update(preambleOpt, boundExpression, node.ConstantTargetOpt, node.InnerLocals, switchSections, node.BreakLabel, node.StringEquality), substituteTemps: true);
         }
 
         public override BoundNode VisitThrowStatement(BoundThrowStatement node)

src/Compilers/CSharp/Portable/Lowering/IteratorRewriter/IteratorMethodToStateMachineRewriter.YieldsInTryAnalysis.cs

@@ -9,7 +9,7 @@ namespace Microsoft.CodeAnalysis.CSharp
     internal partial class IteratorMethodToStateMachineRewriter
     {
         /// <summary>
-        /// Analyses method body for yields in try blocks and labels that they contain.
+        /// Analyzes method body for yields in try blocks and labels that they contain.
         /// </summary>
         private sealed class YieldsInTryAnalysis : LabelCollector
         {
@@ -110,7 +110,7 @@ public override BoundNode VisitExpressionStatement(BoundExpressionStatement node
     }
 
     /// <summary>
-    /// Analyses method body for labels.
+    /// Analyzes method body for labels.
     /// </summary>
     internal abstract class LabelCollector : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
     {

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_SwitchStatement.cs

@@ -83,11 +83,12 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
 
             return rewrittenExpression.Type.IsNullableType() ?
                 MakeSwitchStatementWithNullableExpression(syntax, rewrittenExpression, rewrittenSections, constantTargetOpt, locals, breakLabel, oldNode) :
-                MakeSwitchStatementWithNonNullableExpression(syntax, rewrittenExpression, rewrittenSections, constantTargetOpt, locals, breakLabel, oldNode);
+                MakeSwitchStatementWithNonNullableExpression(syntax, null, rewrittenExpression, rewrittenSections, constantTargetOpt, locals, breakLabel, oldNode);
         }
 
         private BoundStatement MakeSwitchStatementWithNonNullableExpression(
             CSharpSyntaxNode syntax,
+            BoundStatement preambleOpt,
             BoundExpression rewrittenExpression,
             ImmutableArray<BoundSwitchSection> rewrittenSections,
             LabelSymbol constantTargetOpt,
@@ -110,6 +111,7 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
             }
 
             return oldNode.Update(
+                loweredPreambleOpt: preambleOpt,
                 boundExpression: rewrittenExpression,
                 constantTargetOpt: constantTargetOpt,
                 innerLocals: locals,
@@ -157,7 +159,6 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
                 condition: MakeNullCheck(exprSyntax, rewrittenExpression, BinaryOperatorKind.NullableNullEqual),
                 jumpIfTrue: true,
                 label: GetNullValueTargetSwitchLabel(rewrittenSections, breakLabel));
-            statementBuilder.Add(condGotoNullValueTargetLabel);
 
             // Rewrite the switch statement using nullable expression's underlying value as the switch expression.
 
@@ -167,8 +168,15 @@ public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
             rewrittenExpression = callGetValueOrDefault;
 
             // rewrite switch statement
-            BoundStatement rewrittenSwitchStatement = MakeSwitchStatementWithNonNullableExpression(syntax,
-                rewrittenExpression, rewrittenSections, constantTargetOpt, locals, breakLabel, oldNode);
+            BoundStatement rewrittenSwitchStatement = MakeSwitchStatementWithNonNullableExpression(
+                syntax,
+                condGotoNullValueTargetLabel,
+                rewrittenExpression,
+                rewrittenSections,
+                constantTargetOpt,
+                locals,
+                breakLabel,
+                oldNode);
 
             statementBuilder.Add(rewrittenSwitchStatement);
 

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

@@ -146,10 +146,11 @@ public override BoundNode VisitSequence(BoundSequence node)
 
         public override BoundNode VisitSwitchStatement(BoundSwitchStatement node)
         {
+            var preambleOpt = (BoundStatement)this.Visit(node.LoweredPreambleOpt);
             var newInnerLocals = RewriteLocals(node.InnerLocals);
             BoundExpression boundExpression = (BoundExpression)this.Visit(node.BoundExpression);
             ImmutableArray<BoundSwitchSection> switchSections = (ImmutableArray<BoundSwitchSection>)this.VisitList(node.SwitchSections);
-            return node.Update(boundExpression, node.ConstantTargetOpt, newInnerLocals, switchSections, node.BreakLabel, node.StringEquality);
+            return node.Update(preambleOpt, boundExpression, node.ConstantTargetOpt, newInnerLocals, switchSections, node.BreakLabel, node.StringEquality);
         }
 
         public override BoundNode VisitForStatement(BoundForStatement node)

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

@@ -727,6 +727,7 @@ public BoundStatement Switch(BoundExpression ex, params BoundSwitchSection[] sec
             CheckSwitchSections(s);
             return new BoundSwitchStatement(
                 Syntax,
+                null,
                 ex,
                 null,
                 ImmutableArray<LocalSymbol>.Empty,

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

@@ -5224,5 +5224,73 @@ public object Foo<T>()
 }";
             CompileAndVerify(source, new[] { SystemCoreRef });
         }
+
+        [WorkItem(9131, "https://github.com/dotnet/roslyn/issues/9131")]
+        [Fact]
+        public void ClosureInSwitchStatementWithNullableExpression()
+        {
+            string source =
+@"using System;
+class C
+{
+    static void Main()
+    {
+        int? i = null;
+        switch (i)
+        {
+        default:
+            object o = null;
+            Func<object> f = () => o;
+            Console.Write(""{0}"", f() == null);
+            break;
+        case 0:
+            o = 1;
+            break;
+        }
+    }
+}";
+            var compilation = CompileAndVerify(source, expectedOutput: @"True");
+            compilation.VerifyIL("C.Main",
+@"{
+  // Code size       92 (0x5c)
+  .maxstack  3
+  .locals init (int? V_0, //i
+                C.<>c__DisplayClass0_0 V_1, //CS$<>8__locals0
+                int V_2,
+                System.Func<object> V_3) //f
+  IL_0000:  ldloca.s   V_0
+  IL_0002:  initobj    ""int?""
+  IL_0008:  newobj     ""C.<>c__DisplayClass0_0..ctor()""
+  IL_000d:  stloc.1
+  IL_000e:  ldloca.s   V_0
+  IL_0010:  call       ""bool int?.HasValue.get""
+  IL_0015:  brfalse.s  IL_0022
+  IL_0017:  ldloca.s   V_0
+  IL_0019:  call       ""int int?.GetValueOrDefault()""
+  IL_001e:  stloc.2
+  IL_001f:  ldloc.2
+  IL_0020:  brfalse.s  IL_004f
+  IL_0022:  ldloc.1
+  IL_0023:  ldnull
+  IL_0024:  stfld      ""object C.<>c__DisplayClass0_0.o""
+  IL_0029:  ldloc.1
+  IL_002a:  ldftn      ""object C.<>c__DisplayClass0_0.<Main>b__0()""
+  IL_0030:  newobj     ""System.Func<object>..ctor(object, System.IntPtr)""
+  IL_0035:  stloc.3
+  IL_0036:  ldstr      ""{0}""
+  IL_003b:  ldloc.3
+  IL_003c:  callvirt   ""object System.Func<object>.Invoke()""
+  IL_0041:  ldnull
+  IL_0042:  ceq
+  IL_0044:  box        ""bool""
+  IL_0049:  call       ""void System.Console.Write(string, object)""
+  IL_004e:  ret
+  IL_004f:  ldloc.1
+  IL_0050:  ldc.i4.1
+  IL_0051:  box        ""int""
+  IL_0056:  stfld      ""object C.<>c__DisplayClass0_0.o""
+  IL_005b:  ret
+}");
+        }
     }
 }

src/Compilers/VisualBasic/Portable/Analysis/Analyzer.vb

@@ -11,7 +11,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
     Friend Class Analyzer
 
         ''' <summary>
-        ''' Analyses method body for error conditions such as definite assignments, unreachable code etc...
+        ''' Analyzes method body for error conditions such as definite assignments, unreachable code etc...
         ''' 
         ''' This analysis is done when doing the full compile or when responding to GetCompileDiagnostics.
         ''' This method assume that the trees are already bound and will not do any rewriting/lowering

src/Compilers/VisualBasic/Portable/CodeGen/Optimizer/StackScheduler.Analyzer.vb

@@ -27,7 +27,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.CodeGen
         End Enum
 
         ''' <summary>
-        ''' Analyses the tree trying to figure which locals may live on stack. It is 
+        ''' Analyzes the tree trying to figure which locals may live on stack. It is 
         ''' a fairly delicate process and must be very familiar with how CodeGen works. 
         ''' It is essentially a part of CodeGen.
         ''' 

src/Compilers/VisualBasic/Portable/Lowering/LambdaRewriter/LambdaRewriter.Analysis.vb

@@ -130,7 +130,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
             End Sub
 
             ''' <summary>
-            ''' Analyses method body that belongs to the given method symbol.
+            ''' Analyzes method body that belongs to the given method symbol.
             ''' </summary>
             Public Shared Function AnalyzeMethodBody(node As BoundBlock, method As MethodSymbol, symbolsCapturedWithoutCtor As ISet(Of Symbol), diagnostics As DiagnosticBag) As Analysis
                 Debug.Assert(Not node.HasErrors)

src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenClosureLambdaTests.vb

@@ -3887,6 +3887,73 @@ End Class
 </compilation>
             CompileAndVerify(source)
         End Sub
+
+        <Fact>
+        Public Sub ClosureInSwitchStatementWithNullableExpression()
+            Dim verifier = CompileAndVerify(
+<compilation>
+    <file name="a.vb">
+Imports System
+Class C
+    Shared Sub Main()
+        Dim i As Integer? = Nothing
+        Select Case i
+        Case 0
+        Case Else
+            Dim o As Object = Nothing
+            Dim f = Function() o
+            Console.Write("{0}", f() Is Nothing)
+        End Select
+    End Sub
+End Class
+    </file>
+</compilation>, expectedOutput:="True")
+            verifier.VerifyIL("C.Main",
+            <![CDATA[
+{
+  // Code size      104 (0x68)
+  .maxstack  3
+  .locals init (Integer? V_0,
+                Boolean? V_1,
+                VB$AnonymousDelegate_0(Of Object) V_2) //f
+  IL_0000:  ldloca.s   V_0
+  IL_0002:  initobj    "Integer?"
+  IL_0008:  ldloc.0
+  IL_0009:  stloc.0
+  IL_000a:  ldloca.s   V_0
+  IL_000c:  call       "Function Integer?.get_HasValue() As Boolean"
+  IL_0011:  brtrue.s   IL_001e
+  IL_0013:  ldloca.s   V_1
+  IL_0015:  initobj    "Boolean?"
+  IL_001b:  ldloc.1
+  IL_001c:  br.s       IL_002d
+  IL_001e:  ldloca.s   V_0
+  IL_0020:  call       "Function Integer?.GetValueOrDefault() As Integer"
+  IL_0025:  ldc.i4.0
+  IL_0026:  ceq
+  IL_0028:  newobj     "Sub Boolean?..ctor(Boolean)"
+  IL_002d:  stloc.1
+  IL_002e:  ldloca.s   V_1
+  IL_0030:  call       "Function Boolean?.GetValueOrDefault() As Boolean"
+  IL_0035:  brtrue.s   IL_0067
+  IL_0037:  newobj     "Sub C._Closure$__1-0..ctor()"
+  IL_003c:  dup
+  IL_003d:  ldnull
+  IL_003e:  stfld      "C._Closure$__1-0.$VB$Local_o As Object"
+  IL_0043:  ldftn      "Function C._Closure$__1-0._Lambda$__0() As Object"
+  IL_0049:  newobj     "Sub VB$AnonymousDelegate_0(Of Object)..ctor(Object, System.IntPtr)"
+  IL_004e:  stloc.2
+  IL_004f:  ldstr      "{0}"
+  IL_0054:  ldloc.2
+  IL_0055:  callvirt   "Function VB$AnonymousDelegate_0(Of Object).Invoke() As Object"
+  IL_005a:  ldnull
+  IL_005b:  ceq
+  IL_005d:  box        "Boolean"
+  IL_0062:  call       "Sub System.Console.Write(String, Object)"
+  IL_0067:  ret
+}
+]]>)
+        End Sub
     End Class
 End Namespace