Merge pull request #13501 from VSadov/fix13486

Fix for dropping of sideeffects in multiplication by 0

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

@@ -572,10 +572,10 @@ public override BoundNode VisitCatchBlock(BoundCatchBlock node)
                 var sourceOpt = node.ExceptionSourceOpt;
                 var rewrittenFilter = (BoundExpression)this.Visit(filterOpt);
                 var newFilter = sourceOpt == null ?
-                                _F.Sequence(
+                                _F.MakeSequence(
                                     storePending,
                                     rewrittenFilter) :
-                                _F.Sequence(
+                                _F.MakeSequence(
                                     storePending,
                                     AssignCatchSource((BoundExpression)this.Visit(sourceOpt), currentAwaitCatchFrame),
                                     rewrittenFilter);

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

@@ -965,7 +965,7 @@ public override BoundNode VisitLoweredConditionalAccess(BoundLoweredConditionalA
                 // isNotCalss || {clone = receiver; (object)clone != null}
                 condition = _F.LogicalOr(
                                     isNotClass,
-                                    _F.Sequence(
+                                    _F.MakeSequence(
                                         _F.AssignmentExpression(_F.Local(clone), receiver),
                                         _F.ObjectNotEqual(
                                             _F.Convert(_F.SpecialType(SpecialType.System_Object), _F.Local(clone)),

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

@@ -265,6 +265,22 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
                     case BinaryOperatorKind.BoolAnd:
                         if (loweredRight.ConstantValue == ConstantValue.True) return loweredLeft;
                         if (loweredLeft.ConstantValue == ConstantValue.True) return loweredRight;
+
+                        // Note that we are using IsDefaultValue instead of False.
+                        // That is just to catch cases like default(bool) or others resulting in 
+                        // a default bool value, that we know to be "false"
+                        // bool? generally should not reach here, since it is handled by RewriteLiftedBinaryOperator.
+                        // Regardless, the following code should handle default(bool?) correctly since
+                        // default(bool?) & <expr> == default(bool?)  with sideeffects of <expr>
+                        if (loweredLeft.IsDefaultValue())
+                        {
+                            return _factory.MakeSequence(loweredRight, loweredLeft);
+                        }
+                        if (loweredRight.IsDefaultValue())
+                        {
+                            return _factory.MakeSequence(loweredLeft, loweredRight);
+                        }
+
                         goto default;
 
                     case BinaryOperatorKind.BoolOr:
@@ -390,11 +406,11 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
                     case BinaryOperatorKind.ULongMultiplication:
                         if (loweredLeft.IsDefaultValue())
                         {
-                            return loweredLeft;
+                            return _factory.MakeSequence(loweredRight, loweredLeft);
                         }
                         if (loweredRight.IsDefaultValue())
                         {
-                            return loweredRight;
+                            return _factory.MakeSequence(loweredLeft, loweredRight);
                         }
                         if (loweredLeft.ConstantValue?.UInt64Value == 1)
                         {
@@ -784,7 +800,7 @@ private BoundExpression MakeTruthTestForDynamicLogicalOperator(SyntaxNode syntax
                 else
                 {
                     BoundExpression falseExpr = MakeBooleanConstant(syntax, operatorKind == BinaryOperatorKind.NotEqual);
-                    return _factory.Sequence(maybeNull, falseExpr);
+                    return _factory.MakeSequence(maybeNull, falseExpr);
                 }
             }
 

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

@@ -691,13 +691,36 @@ private BoundExpression BoxReceiver(BoundExpression rewrittenReceiver, NamedType
                         return objCreation.Arguments.Length == 1 && ReadIsSideeffecting(objCreation.Arguments[0]);
                     }
 
-                    return false;
+                    return true;
+
+                case BoundKind.Call:
+                    var call = (BoundCall)expression;
+                    var method = call.Method;
+
+                    // common production of lowered lifted operators
+                    // GetValueOrDefault is known to be not sideeffecting.
+                    if (method.ContainingType?.IsNullableType() == true)
+                    {
+                        if (IsSpecialMember(method, SpecialMember.System_Nullable_T_GetValueOrDefault) ||
+                            IsSpecialMember(method, SpecialMember.System_Nullable_T_get_HasValue))
+                        {
+                            return ReadIsSideeffecting(call.ReceiverOpt);
+                        }
+                    }
+
+                    return true;
 
                 default:
                     return true;
             }
         }
 
+        private static bool IsSpecialMember(MethodSymbol method, SpecialMember specialMember)
+        {
+            method = method.OriginalDefinition;
+            return method.ContainingAssembly?.GetSpecialTypeMember(specialMember) == method;
+        }
+
         // nontrivial literals do not change between reads
         // but may require re-constructing, so it is better 
         // to treat them as potentially changing.

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

@@ -162,7 +162,7 @@ internal BoundExpression RewriteConditionalAccess(BoundConditionalAccess node, b
 
                 case ConditionalAccessLoweringKind.TernaryCaptureReceiverByVal:
                     // capture the receiver into a temp
-                    loweredReceiver = _factory.Sequence(
+                    loweredReceiver = _factory.MakeSequence(
                                             _factory.AssignmentExpression(_factory.Local(temp), loweredReceiver),
                                             _factory.Local(temp));
 
@@ -186,7 +186,7 @@ internal BoundExpression RewriteConditionalAccess(BoundConditionalAccess node, b
 
                         if (temp != null)
                         {
-                            result = _factory.Sequence(temp, result);
+                            result = _factory.MakeSequence(temp, result);
                         }
                     }
                     break;

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

@@ -640,7 +640,7 @@ private BoundExpression MakeImplicitConversion(BoundExpression rewrittenOperand,
             }
 
             var result = MakeTupleCreationExpression(syntax, rewrittenType, fieldAccessorsBuilder.ToImmutableAndFree());
-            return _factory.Sequence(savedTuple.LocalSymbol, assignmentToTemp, result);
+            return _factory.MakeSequence(savedTuple.LocalSymbol, assignmentToTemp, result);
         }
 
         private static bool NullableNeverHasValue(BoundExpression expression)

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

@@ -58,7 +58,7 @@ private BoundExpression LowerDeclarationPattern(BoundDeclarationPattern pattern,
                 Debug.Assert(input.Type == pattern.LocalSymbol.Type);
                 var assignment = _factory.AssignmentExpression(_factory.Local(pattern.LocalSymbol), input);
                 var result = _factory.Literal(true);
-                return _factory.Sequence(assignment, result);
+                return _factory.MakeSequence(assignment, result);
             }
 
             return MakeDeclarationPattern(pattern.Syntax, input, pattern.LocalSymbol, requiresNullTest: true);
@@ -69,7 +69,7 @@ private BoundExpression LowerDeclarationPattern(BoundDeclarationPattern pattern,
         /// </summary>
         BoundExpression LogicalAndForPatterns(BoundExpression left, BoundExpression right)
         {
-            return IsIrrefutablePatternTest(left) ? _factory.Sequence(left, right) : _factory.LogicalAnd(left, right);
+            return IsIrrefutablePatternTest(left) ? _factory.MakeSequence(left, right) : _factory.LogicalAnd(left, right);
         }
 
         /// <summary>
@@ -125,13 +125,13 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                     var result = requiresNullTest
                         ? _factory.ObjectNotEqual(_factory.Local(target), _factory.Null(type))
                         : (BoundExpression)_factory.Literal(true);
-                    return _factory.Sequence(assignment, result);
+                    return _factory.MakeSequence(assignment, result);
                 }
                 else
                 {
                     var assignment = _factory.AssignmentExpression(_factory.Local(target), _factory.As(input, type));
                     var result = _factory.ObjectNotEqual(_factory.Local(target), _factory.Null(type));
-                    return _factory.Sequence(assignment, result);
+                    return _factory.MakeSequence(assignment, result);
                 }
             }
             else if (type.IsNullableType())
@@ -146,7 +146,7 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                 // }
                 var assignment = _factory.AssignmentExpression(_factory.Local(target), _factory.As(input, type));
                 var result = _factory.Call(_factory.Local(target), GetNullableMethod(syntax, type, SpecialMember.System_Nullable_T_get_HasValue));
-                return _factory.Sequence(assignment, result);
+                return _factory.MakeSequence(assignment, result);
             }
             else if (type.IsValueType)
             {
@@ -154,7 +154,7 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                 // is irrefutable, and we can just do the assignment and return true.
                 if (input.Type == type)
                 {
-                    return _factory.Sequence(
+                    return _factory.MakeSequence(
                         _factory.AssignmentExpression(_factory.Local(target), input),
                         _factory.Literal(true));
                 }
@@ -175,7 +175,7 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                         GetNullableMethod(syntax, tmpType, SpecialMember.System_Nullable_T_GetValueOrDefault));
                     var asg2 = _factory.AssignmentExpression(_factory.Local(target), value);
                     var result = requiresNullTest ? MakeNullableHasValue(syntax, input) : _factory.Literal(true);
-                    return _factory.Sequence(asg2, result);
+                    return _factory.MakeSequence(asg2, result);
                 }
                 else
                 {
@@ -186,7 +186,7 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                         GetNullableMethod(syntax, tmpType, SpecialMember.System_Nullable_T_GetValueOrDefault));
                     var asg2 = _factory.AssignmentExpression(_factory.Local(target), value);
                     var result = MakeNullableHasValue(syntax, _factory.Local(tmp));
-                    return _factory.Sequence(tmp, asg1, asg2, result);
+                    return _factory.MakeSequence(tmp, asg1, asg2, result);
                 }
             }
             else // type parameter
@@ -200,7 +200,7 @@ BoundExpression MakeDeclarationPattern(SyntaxNode syntax, BoundExpression input,
                 //     return s;
                 // }
                 return _factory.Conditional(_factory.Is(input, type),
-                    _factory.Sequence(_factory.AssignmentExpression(
+                    _factory.MakeSequence(_factory.AssignmentExpression(
                         _factory.Local(target),
                         _factory.Convert(type, input)),
                         _factory.Literal(true)),

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

@@ -734,30 +734,44 @@ public BoundLocal Local(LocalSymbol local)
             return new BoundLocal(Syntax, local, null, local.Type) { WasCompilerGenerated = true };
         }
 
-        public BoundExpression Sequence(LocalSymbol temp, params BoundExpression[] parts)
+        public BoundExpression MakeSequence(LocalSymbol temp, params BoundExpression[] parts)
         {
-            return Sequence(ImmutableArray.Create<LocalSymbol>(temp), parts);
+            return MakeSequence(ImmutableArray.Create<LocalSymbol>(temp), parts);
         }
 
-        public BoundExpression Sequence(params BoundExpression[] parts)
+        public BoundExpression MakeSequence(params BoundExpression[] parts)
         {
-            return Sequence(ImmutableArray<LocalSymbol>.Empty, parts);
+            return MakeSequence(ImmutableArray<LocalSymbol>.Empty, parts);
         }
 
-        public BoundExpression Sequence(ImmutableArray<LocalSymbol> locals, params BoundExpression[] parts)
+        public BoundExpression MakeSequence(ImmutableArray<LocalSymbol> locals, params BoundExpression[] parts)
         {
             var builder = ArrayBuilder<BoundExpression>.GetInstance();
-            for (int i = 0; i < parts.Length - 1; i++) builder.Add(parts[i]);
+            for (int i = 0; i < parts.Length - 1; i++)
+            {
+                var part = parts[i];
+                if (LocalRewriter.ReadIsSideeffecting(part))
+                {
+                    builder.Add(parts[i]);
+                }
+            }
             var lastExpression = parts[parts.Length - 1];
+
+            if (locals.IsDefaultOrEmpty && builder.Count == 0)
+            {
+                builder.Free();
+                return lastExpression;
+            }
+
             return Sequence(locals, builder.ToImmutableAndFree(), lastExpression);
         }
 
-        public BoundExpression Sequence(BoundExpression[] sideEffects, BoundExpression result, TypeSymbol type = null)
+        public BoundSequence Sequence(BoundExpression[] sideEffects, BoundExpression result, TypeSymbol type = null)
         {
             return new BoundSequence(Syntax, ImmutableArray<LocalSymbol>.Empty, sideEffects.AsImmutableOrNull(), result, type ?? result.Type) { WasCompilerGenerated = true };
         }
 
-        public BoundExpression Sequence(ImmutableArray<LocalSymbol> locals, ImmutableArray<BoundExpression> sideEffects, BoundExpression result)
+        public BoundSequence Sequence(ImmutableArray<LocalSymbol> locals, ImmutableArray<BoundExpression> sideEffects, BoundExpression result)
         {
             return new BoundSequence(Syntax, locals, sideEffects, result, result.Type) { WasCompilerGenerated = true };
         }

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

@@ -15304,5 +15304,189 @@ static void Main()
                 Diagnostic(ErrorCode.ERR_ObjectRequired, "C.InstanceMethod((dynamic)2)").WithArguments("C.InstanceMethod(int)").WithLocation(34, 28)
             );
         }
+
+        [Fact, WorkItem(13486, "https://github.com/dotnet/roslyn/issues/13486")]
+        public void BinaryMulOptimizationsAndSideeffects()
+        {
+            string source = @"
+using System;
+
+class Program
+{
+    static void Main(string[] args)
+    {
+        // should not optimize
+        System.Console.WriteLine(Foo1() * 0);
+        System.Console.WriteLine(0 * Foo1());
+
+        // should optimize
+        var local = 123;
+        System.Console.WriteLine(local * 0);
+        System.Console.WriteLine(0 * default(int?));
+        System.Console.WriteLine(0 * local);
+
+        // should not capture
+        System.Console.WriteLine(((Func<int>)(()=>local * 0))());
+
+        // should not optimize
+        System.Console.WriteLine(Foo2() & false);
+        System.Console.WriteLine(false & Foo2());
+
+        // should optimize
+        var local1 = true;
+        System.Console.WriteLine(local1 & false);
+        System.Console.WriteLine(false & default(bool?));
+        System.Console.WriteLine(false & ((bool?)local1).HasValue);
+        System.Console.WriteLine(false & local1);
+
+        // should optimize
+        System.Console.WriteLine(Foo2() && false);
+        System.Console.WriteLine(Foo2() && true);
+        System.Console.WriteLine(false && Foo2());
+        System.Console.WriteLine(true && Foo2());
+        System.Console.WriteLine(Foo2() || false);
+        System.Console.WriteLine(Foo2() || true);
+        System.Console.WriteLine(false || Foo2());
+        System.Console.WriteLine(true || Foo2());
+    }
+
+    static int Foo1()
+    {
+        System.Console.Write(""Foo1 "");
+        return 42;
+    }
+
+    static bool Foo2()
+    {
+        System.Console.Write(""Foo2 "");
+        return true;
+    }
+}
+";
+            var compilation = CompileAndVerify(source, expectedOutput: @"
+Foo1 0
+Foo1 0
+0
+
+0
+0
+Foo2 False
+Foo2 False
+False
+False
+False
+False
+Foo2 False
+Foo2 True
+False
+Foo2 True
+Foo2 True
+Foo2 True
+Foo2 True
+True
+");
+
+            compilation.VerifyIL("Program.Main",
+    @"
+{
+  // Code size      221 (0xdd)
+  .maxstack  2
+  IL_0000:  call       ""int Program.Foo1()""
+  IL_0005:  pop
+  IL_0006:  ldc.i4.0
+  IL_0007:  call       ""void System.Console.WriteLine(int)""
+  IL_000c:  call       ""int Program.Foo1()""
+  IL_0011:  pop
+  IL_0012:  ldc.i4.0
+  IL_0013:  call       ""void System.Console.WriteLine(int)""
+  IL_0018:  ldc.i4.0
+  IL_0019:  call       ""void System.Console.WriteLine(int)""
+  IL_001e:  ldnull
+  IL_001f:  call       ""void System.Console.WriteLine(object)""
+  IL_0024:  ldc.i4.0
+  IL_0025:  call       ""void System.Console.WriteLine(int)""
+  IL_002a:  ldsfld     ""System.Func<int> Program.<>c.<>9__0_0""
+  IL_002f:  dup
+  IL_0030:  brtrue.s   IL_0049
+  IL_0032:  pop
+  IL_0033:  ldsfld     ""Program.<>c Program.<>c.<>9""
+  IL_0038:  ldftn      ""int Program.<>c.<Main>b__0_0()""
+  IL_003e:  newobj     ""System.Func<int>..ctor(object, System.IntPtr)""
+  IL_0043:  dup
+  IL_0044:  stsfld     ""System.Func<int> Program.<>c.<>9__0_0""
+  IL_0049:  callvirt   ""int System.Func<int>.Invoke()""
+  IL_004e:  call       ""void System.Console.WriteLine(int)""
+  IL_0053:  call       ""bool Program.Foo2()""
+  IL_0058:  pop
+  IL_0059:  ldc.i4.0
+  IL_005a:  call       ""void System.Console.WriteLine(bool)""
+  IL_005f:  call       ""bool Program.Foo2()""
+  IL_0064:  pop
+  IL_0065:  ldc.i4.0
+  IL_0066:  call       ""void System.Console.WriteLine(bool)""
+  IL_006b:  ldc.i4.0
+  IL_006c:  call       ""void System.Console.WriteLine(bool)""
+  IL_0071:  ldc.i4.0
+  IL_0072:  box        ""bool""
+  IL_0077:  call       ""void System.Console.WriteLine(object)""
+  IL_007c:  ldc.i4.0
+  IL_007d:  call       ""void System.Console.WriteLine(bool)""
+  IL_0082:  ldc.i4.0
+  IL_0083:  call       ""void System.Console.WriteLine(bool)""
+  IL_0088:  call       ""bool Program.Foo2()""
+  IL_008d:  brfalse.s  IL_0092
+  IL_008f:  ldc.i4.0
+  IL_0090:  br.s       IL_0093
+  IL_0092:  ldc.i4.0
+  IL_0093:  call       ""void System.Console.WriteLine(bool)""
+  IL_0098:  call       ""bool Program.Foo2()""
+  IL_009d:  call       ""void System.Console.WriteLine(bool)""
+  IL_00a2:  ldc.i4.0
+  IL_00a3:  call       ""void System.Console.WriteLine(bool)""
+  IL_00a8:  call       ""bool Program.Foo2()""
+  IL_00ad:  call       ""void System.Console.WriteLine(bool)""
+  IL_00b2:  call       ""bool Program.Foo2()""
+  IL_00b7:  call       ""void System.Console.WriteLine(bool)""
+  IL_00bc:  call       ""bool Program.Foo2()""
+  IL_00c1:  brtrue.s   IL_00c6
+  IL_00c3:  ldc.i4.1
+  IL_00c4:  br.s       IL_00c7
+  IL_00c6:  ldc.i4.1
+  IL_00c7:  call       ""void System.Console.WriteLine(bool)""
+  IL_00cc:  call       ""bool Program.Foo2()""
+  IL_00d1:  call       ""void System.Console.WriteLine(bool)""
+  IL_00d6:  ldc.i4.1
+  IL_00d7:  call       ""void System.Console.WriteLine(bool)""
+  IL_00dc:  ret
+}
+");
+        }
+
+        [Fact, WorkItem(0, "http://stackoverflow.com/questions/39254676/roslyn-compiler-optimizing-away-function-call-multiplication-with-zero?stw=2")]
+        public void SideEffectOptimizedAway()
+        {
+            var source =
+@"
+using System;
+using System.Collections.Generic;
+using System.Linq;
+
+class Program
+{
+    public static void Main(string[] args)
+    {
+        Stack<long> s = new Stack<long>();
+
+        s.Push(1);           // stack contains [1]
+        s.Push(s.Pop() * 0); // stack should contain [0]
+
+        Console.WriteLine(string.Join(""|"", s.Reverse()));
+    }
+}
+";
+            CompileAndVerify(source, additionalRefs: new[] { SystemRef, SystemCoreRef },
+                expectedOutput: "0");
+        }
+
     }
 }