Merge pull request #17241 from DustinCampbell/fix-remote-parentheses

Fix parenthesis simplification around binary expressions

src/EditorFeatures/CSharpTest/CodeActions/InlineTemporary/InlineTemporaryTests.cs

@@ -4157,5 +4157,31 @@ public class KVP<T1, T2>
 
             await TestAsync(code, expected, index: 0);
         }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)]
+        [WorkItem(11958, "https://github.com/dotnet/roslyn/issues/11958")]
+        public async Task EnsureParenthesesInStringConcatenation()
+        {
+            var code = @"
+class C
+{
+    void M()
+    {
+        int [||]i = 1 + 2;
+        string s = ""a"" + i;
+    }
+}";
+
+            var expected = @"
+class C
+{
+    void M()
+    {
+        string s = ""a"" + (1 + 2);
+    }
+}";
+
+            await TestAsync(code, expected, index: 0, compareTokens: false);
+        }
     }
 }

src/Workspaces/CSharp/Portable/Extensions/ParenthesizedExpressionSyntaxExtensions.cs

@@ -9,7 +9,7 @@ namespace Microsoft.CodeAnalysis.CSharp.Extensions
 {
     internal static class ParenthesizedExpressionSyntaxExtensions
     {
-        public static bool CanRemoveParentheses(this ParenthesizedExpressionSyntax node)
+        public static bool CanRemoveParentheses(this ParenthesizedExpressionSyntax node, SemanticModel semanticModel)
         {
             var expression = node.Expression;
             var parentExpression = node.Parent as ExpressionSyntax;
@@ -154,7 +154,7 @@ public static bool CanRemoveParentheses(this ParenthesizedExpressionSyntax node)
             // - If the parent is not an expression, do not remove parentheses
             // - Otherwise, parentheses may be removed if doing so does not change operator associations.
             return parentExpression != null
-                ? !RemovalChangesAssociation(node, expression, parentExpression)
+                ? !RemovalChangesAssociation(node, expression, parentExpression, semanticModel)
                 : false;
         }
 
@@ -221,7 +221,7 @@ private static bool RemovalMayIntroduceInterpolationAmbiguity(ParenthesizedExpre
             return false;
         }
 
-        private static bool RemovalChangesAssociation(ParenthesizedExpressionSyntax node, ExpressionSyntax expression, ExpressionSyntax parentExpression)
+        private static bool RemovalChangesAssociation(ParenthesizedExpressionSyntax node, ExpressionSyntax expression, ExpressionSyntax parentExpression, SemanticModel semanticModel)
         {
             var precedence = expression.GetOperatorPrecedence();
             var parentPrecedence = parentExpression.GetOperatorPrecedence();
@@ -252,14 +252,43 @@ private static bool RemovalChangesAssociation(ParenthesizedExpressionSyntax node
                     return false;
                 }
 
-                var parentBinaryExpression = parentExpression as BinaryExpressionSyntax;
-                if (parentBinaryExpression != null)
+                if (parentExpression is BinaryExpressionSyntax parentBinaryExpression)
                 {
                     // If both the expression and its parent are binary expressions and their kinds
                     // are the same, check to see if they are commutative (e.g. + or *).
                     if (parentBinaryExpression.IsKind(SyntaxKind.AddExpression, SyntaxKind.MultiplyExpression) &&
                         node.Expression.Kind() == parentBinaryExpression.Kind())
                     {
+                        // At this point, we know our parenthesized expression contains a binary expression.
+                        var binaryExpression = (BinaryExpressionSyntax)node.Expression;
+
+                        // Now we'll perform a few semantic checks to determine whether removal of the parentheses
+                        // might break semantics. Note that we'll try and be fairly conservative with these. For example,
+                        // we'll assume that failing any of these checks results in the parentheses being declared as
+                        // necessary -- even if they could be removed depending on whether the parenthesized expression
+                        // appears on the left or right side of the parent binary expression.
+
+                        // First, does the binary expression result in an operator overload being called?
+                        var symbolInfo = semanticModel.GetSymbolInfo(binaryExpression);
+                        if (symbolInfo.Symbol != null)
+                        {
+                            if (symbolInfo.Symbol is IMethodSymbol methodSymbol &&
+                                methodSymbol.MethodKind == MethodKind.UserDefinedOperator)
+                            {
+                                return true;
+                            }
+                        }
+
+                        // Second, check the type and converted type of the binary expression. Are they the same?
+                        var typeInfo = semanticModel.GetTypeInfo(binaryExpression);
+                        if (typeInfo.Type != null && typeInfo.ConvertedType != null)
+                        {
+                            if (!typeInfo.Type.Equals(typeInfo.ConvertedType))
+                            {
+                                return true;
+                            }
+                        }
+
                         return false;
                     }
 
@@ -273,8 +302,7 @@ private static bool RemovalChangesAssociation(ParenthesizedExpressionSyntax node
                     return parentBinaryExpression.Right == node;
                 }
 
-                var parentAssignmentExpression = parentExpression as AssignmentExpressionSyntax;
-                if (parentAssignmentExpression != null)
+                if (parentExpression is AssignmentExpressionSyntax parentAssignmentExpression)
                 {
                     // Assignment expressions are right associative; removing parens from the LHS changes the association.
                     return parentAssignmentExpression.Left == node;
@@ -332,8 +360,7 @@ private static bool RemovalMayIntroduceCommaListAmbiguity(ParenthesizedExpressio
                 //   {x < (x), x > (1 + 2)}
                 //   {x < x, (x) > (1 + 2)}
 
-                var binaryExpression = node.Parent as BinaryExpressionSyntax;
-                if (binaryExpression != null &&
+                if (node.Parent is BinaryExpressionSyntax binaryExpression &&
                     binaryExpression.IsKind(SyntaxKind.LessThanExpression, SyntaxKind.GreaterThanExpression) &&
                     (binaryExpression.IsParentKind(SyntaxKind.Argument) || binaryExpression.Parent is InitializerExpressionSyntax))
                 {
@@ -404,8 +431,7 @@ private static bool IsPreviousExpressionPotentiallyAmbiguous(ExpressionSyntax no
             if (node.IsParentKind(SyntaxKind.Argument))
             {
                 var argument = (ArgumentSyntax)node.Parent;
-                var argumentList = argument.Parent as ArgumentListSyntax;
-                if (argumentList != null)
+                if (argument.Parent is ArgumentListSyntax argumentList)
                 {
                     var argumentIndex = argumentList.Arguments.IndexOf(argument);
                     if (argumentIndex > 0)
@@ -414,9 +440,8 @@ private static bool IsPreviousExpressionPotentiallyAmbiguous(ExpressionSyntax no
                     }
                 }
             }
-            else if (node.Parent is InitializerExpressionSyntax)
+            else if (node.Parent is InitializerExpressionSyntax initializer)
             {
-                var initializer = (InitializerExpressionSyntax)node.Parent;
                 var expressionIndex = initializer.Expressions.IndexOf(node);
                 if (expressionIndex > 0)
                 {
@@ -444,8 +469,7 @@ private static bool IsNextExpressionPotentiallyAmbiguous(ExpressionSyntax node)
             if (node.IsParentKind(SyntaxKind.Argument))
             {
                 var argument = (ArgumentSyntax)node.Parent;
-                var argumentList = argument.Parent as ArgumentListSyntax;
-                if (argumentList != null)
+                if (argument.Parent is ArgumentListSyntax argumentList)
                 {
                     var argumentIndex = argumentList.Arguments.IndexOf(argument);
                     if (argumentIndex >= 0 && argumentIndex < argumentList.Arguments.Count - 1)
@@ -454,9 +478,8 @@ private static bool IsNextExpressionPotentiallyAmbiguous(ExpressionSyntax node)
                     }
                 }
             }
-            else if (node.Parent is InitializerExpressionSyntax)
+            else if (node.Parent is InitializerExpressionSyntax initializer)
             {
-                var initializer = (InitializerExpressionSyntax)node.Parent;
                 var expressionIndex = initializer.Expressions.IndexOf(node);
                 if (expressionIndex >= 0 && expressionIndex < initializer.Expressions.Count - 1)
                 {

src/Workspaces/CSharp/Portable/Simplification/CSharpParenthesesReducer.cs

@@ -23,7 +23,7 @@ public override IExpressionRewriter CreateExpressionRewriter(OptionSet optionSet
             OptionSet optionSet,
             CancellationToken cancellationToken)
         {
-            if (node.CanRemoveParentheses())
+            if (node.CanRemoveParentheses(semanticModel))
             {
                 // TODO(DustinCa): We should not be skipping elastic trivia below.
                 // However, the formatter seems to mess up trailing trivia in some