Rewrite string.Concat lowering code (#76955)

Our existing lowering strategy for string addition is complex, to say the least. It works in a bottom-up fashion, needing to constantly undo and redo existing work as VisitExpression returns and discovered ever-larger sequences of string additions. This is not easily maintainable, and also makes addressing issues like #74538 extremely difficult. As a first step in that direction, I've rewritten how we lower these to instead approach it top-down; when a + that operates on strings is encountered, we gather all possible operands up front, fold as many constant components as we can, and then emit the final string.Concat call all in one go. This should hopefully be significantly easier to maintain in the future, as well as be a much more flexible base for building further optimizations on, such as using string.Concat(ReadOnlySpan<string>) or DefaultInterpolatedStringHandler in the future.

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

@@ -125,6 +125,12 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
                 return VisitUtf8Addition(node);
             }
 
+            if (IsBinaryStringConcatenation(node))
+            {
+                Debug.Assert(applyParentUnaryOperator is null);
+                return VisitStringConcatenation(node);
+            }
+
             // In machine-generated code we frequently end up with binary operator trees that are deep on the left,
             // such as a + b + c + d ...
             // To avoid blowing the call stack, we make an explicit stack of the binary operators to the left, 
@@ -135,7 +141,7 @@ public BoundExpression VisitBinaryOperator(BoundBinaryOperator node, BoundUnaryO
             for (BoundBinaryOperator? current = node; current != null && current.ConstantValueOpt == null; current = current.Left as BoundBinaryOperator)
             {
                 // The regular visit mechanism will handle this.
-                if (current.InterpolatedStringHandlerData is not null || current.OperatorKind is BinaryOperatorKind.Utf8Addition)
+                if (current.InterpolatedStringHandlerData is not null || current.OperatorKind is BinaryOperatorKind.Utf8Addition || IsBinaryStringConcatenation(current))
                 {
                     Debug.Assert(stack.Count >= 1);
                     break;
@@ -208,7 +214,7 @@ private BoundExpression MakeBinaryOperator(
                     case BinaryOperatorKind.ObjectAndStringConcatenation:
                     case BinaryOperatorKind.StringAndObjectConcatenation:
                     case BinaryOperatorKind.StringConcatenation:
-                        return RewriteStringConcatenation(syntax, operatorKind, loweredLeft, loweredRight, type);
+                        throw ExceptionUtilities.UnexpectedValue(operatorKind);
                     case BinaryOperatorKind.DelegateCombination:
                         return RewriteDelegateOperation(syntax, operatorKind, loweredLeft, loweredRight, type, SpecialMember.System_Delegate__Combine);
                     case BinaryOperatorKind.DelegateRemoval:
@@ -256,7 +262,7 @@ private BoundExpression MakeBinaryOperator(
                     case BinaryOperatorKind.ObjectAndStringConcatenation:
                     case BinaryOperatorKind.StringAndObjectConcatenation:
                     case BinaryOperatorKind.StringConcatenation:
-                        return RewriteStringConcatenation(syntax, operatorKind, loweredLeft, loweredRight, type);
+                        throw ExceptionUtilities.UnexpectedValue(operatorKind);
 
                     case BinaryOperatorKind.StringEqual:
                         return RewriteStringEquality(oldNode, syntax, operatorKind, loweredLeft, loweredRight, type, SpecialMember.System_String__op_Equality);

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

@@ -22,7 +22,6 @@ public override BoundNode VisitCompoundAssignmentOperator(BoundCompoundAssignmen
         private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentOperator node, bool used)
         {
             Debug.Assert(TypeSymbol.Equals(node.Right.Type, node.Operator.RightType, TypeCompareKind.ConsiderEverything2));
-            BoundExpression loweredRight = VisitExpression(node.Right);
 
             var temps = ArrayBuilder<LocalSymbol>.GetInstance();
             var stores = ArrayBuilder<BoundExpression>.GetInstance();
@@ -55,6 +54,8 @@ private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentO
                 // side before storing the lambda to a temp for use in both possible branches.
                 // The first store to memberAccessReceiver has already been taken care of above by TransformCompoundAssignmentLHS
 
+                Debug.Assert(!IsBinaryStringConcatenation(binaryOperator));
+
                 var eventTemps = ArrayBuilder<LocalSymbol>.GetInstance();
                 var sequence = ArrayBuilder<BoundExpression>.GetInstance();
 
@@ -74,6 +75,7 @@ private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentO
                 sequence.Add(nonEventStore);
 
                 // var loweredRight = handler;
+                BoundExpression loweredRight = VisitExpression(node.Right);
                 if (CanChangeValueBetweenReads(loweredRight))
                 {
                     loweredRight = _factory.StoreToTemp(loweredRight, out BoundAssignmentOperator possibleHandlerAssignment);
@@ -88,7 +90,7 @@ private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentO
                     loweredRight);
 
                 // transformedLHS = storeNonEvent + loweredRight
-                rewrittenAssignment = rewriteAssignment(lhsRead);
+                rewrittenAssignment = rewriteAssignment(lhsRead, loweredRight, rightIsVisited: true);
                 Debug.Assert(rewrittenAssignment.Type is { });
 
                 // Final conditional
@@ -98,7 +100,7 @@ private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentO
             }
             else
             {
-                rewrittenAssignment = rewriteAssignment(lhsRead);
+                rewrittenAssignment = rewriteAssignment(lhsRead, node.Right, rightIsVisited: false);
             }
 
             Debug.Assert(rewrittenAssignment.Type is { });
@@ -115,7 +117,7 @@ private BoundExpression VisitCompoundAssignmentOperator(BoundCompoundAssignmentO
             stores.Free();
             return result;
 
-            BoundExpression rewriteAssignment(BoundExpression leftRead)
+            BoundExpression rewriteAssignment(BoundExpression leftRead, BoundExpression right, bool rightIsVisited)
             {
                 SyntaxNode syntax = node.Syntax;
 
@@ -139,7 +141,18 @@ BoundExpression rewriteAssignment(BoundExpression leftRead)
                     RemovePlaceholderReplacement(node.LeftPlaceholder);
                 }
 
-                BoundExpression operand = MakeBinaryOperator(syntax, node.Operator.Kind, opLHS, loweredRight, node.Operator.ReturnType, node.Operator.Method, node.Operator.ConstrainedToTypeOpt, isCompoundAssignment: true);
+                BoundExpression operand;
+                if (IsBinaryStringConcatenation(node.Operator.Kind))
+                {
+                    Debug.Assert(!rightIsVisited);
+                    Debug.Assert(node.Operator.ReturnType is { SpecialType: SpecialType.System_String });
+                    operand = VisitCompoundAssignmentStringConcatenation(opLHS, right, node.Operator.Kind, node.Syntax);
+                }
+                else
+                {
+                    var loweredRight = rightIsVisited ? right : VisitExpression(right);
+                    operand = MakeBinaryOperator(syntax, node.Operator.Kind, opLHS, loweredRight, node.Operator.ReturnType, node.Operator.Method, node.Operator.ConstrainedToTypeOpt, isCompoundAssignment: true);
+                }
 
                 Debug.Assert(node.Left.Type is { });
                 BoundExpression opFinal = operand;