Do not remove parentheses from arithmetic ops in 'checked' contexts

src/Analyzers/CSharp/Tests/RemoveUnnecessaryParentheses/RemoveUnnecessaryExpressionParenthesesTests.cs

@@ -2753,5 +2753,43 @@ public void M()
 }
 ", offeredWhenRequireForClarityIsEnabled: true);
         }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)]
+        [WorkItem(45100, "https://github.com/dotnet/roslyn/issues/45100")]
+        public async Task TestArithmeticOverflow1()
+        {
+            await TestMissingAsync(
+@"class C
+{
+    void M(int a)
+    {
+        checked
+        {
+            return a + $$(int.MaxValue + -int.MaxValue);
+        }
+    }
+}", parameters: new TestParameters(options: RemoveAllUnnecessaryParentheses));
+        }
+
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)]
+        [WorkItem(45100, "https://github.com/dotnet/roslyn/issues/45100")]
+        public async Task TestArithmeticOverflow2()
+        {
+            await TestInRegularAndScript1Async(
+@"class C
+{
+    void M(int a)
+    {
+        return a + $$(int.MaxValue + -int.MaxValue);
+    }
+}",
+@"class C
+{
+    void M(int a)
+    {
+        return a + int.MaxValue + -int.MaxValue;
+    }
+}", parameters: new TestParameters(options: RemoveAllUnnecessaryParentheses));
+        }
     }
 }

src/Analyzers/VisualBasic/Tests/RemoveUnnecessaryParentheses/RemoveUnnecessaryParenthesesTests.vb

@@ -18,6 +18,9 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.RemoveUnnecessaryP
     Partial Public Class RemoveUnnecessaryParenthesesTests
         Inherits AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest
 
+        Private Shared ReadOnly CheckOverflow As CompilationOptions = New VisualBasicCompilationOptions(OutputKind.ConsoleApplication, checkOverflow:=True)
+        Private Shared ReadOnly DoNotCheckOverflow As CompilationOptions = New VisualBasicCompilationOptions(OutputKind.ConsoleApplication, checkOverflow:=False)
+
         Friend Overrides Function CreateDiagnosticProviderAndFixer(Workspace As Workspace) As (DiagnosticAnalyzer, CodeFixProvider)
             Return (New VisualBasicRemoveUnnecessaryParenthesesDiagnosticAnalyzer(), New VisualBasicRemoveUnnecessaryParenthesesCodeFixProvider())
         End Function
@@ -32,13 +35,18 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.RemoveUnnecessaryP
 
         Private Shadows Async Function TestAsync(initial As String, expected As String,
                                                  offeredWhenRequireAllParenthesesForClarityIsEnabled As Boolean,
-                                                 Optional ByVal index As Integer = 0) As Task
-            Await TestInRegularAndScriptAsync(initial, expected, options:=RemoveAllUnnecessaryParentheses, index:=index)
+                                                 Optional index As Integer = 0,
+                                                 Optional checkOverflow As Boolean = True) As Task
+            Dim compilationOptions = If(checkOverflow,
+                RemoveUnnecessaryParenthesesTests.CheckOverflow,
+                RemoveUnnecessaryParenthesesTests.DoNotCheckOverflow)
+
+            Await TestInRegularAndScriptAsync(initial, expected, options:=RemoveAllUnnecessaryParentheses, index:=index, compilationOptions:=compilationOptions)
 
             If (offeredWhenRequireAllParenthesesForClarityIsEnabled) Then
-                Await TestInRegularAndScriptAsync(initial, expected, options:=MyBase.RequireAllParenthesesForClarity, index:=index)
+                Await TestInRegularAndScriptAsync(initial, expected, options:=RequireAllParenthesesForClarity, index:=index, compilationOptions:=compilationOptions)
             Else
-                Await TestMissingAsync(initial, parameters:=New TestParameters(options:=MyBase.RequireAllParenthesesForClarity))
+                Await TestMissingAsync(initial, parameters:=New TestParameters(options:=RequireAllParenthesesForClarity, compilationOptions:=compilationOptions))
             End If
         End Function
 
@@ -114,7 +122,7 @@ end class", parameters:=New TestParameters(options:=RequireOtherBinaryParenthese
         End Function
 
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)>
-        Public Async Function TestArithmeticNotRequiredForClarityWhenPrecedenceStaysTheSame1() As Task
+        Public Async Function TestArithmeticNotRequiredForClarityWhenPrecedenceStaysTheSame1_DoNotCheckOverflow() As Task
             Await TestAsync(
 "class C
     sub M()
@@ -125,7 +133,17 @@ end class",
     sub M()
         dim x = 1 + 2 + 3
     end sub
-end class", offeredWhenRequireAllParenthesesForClarityIsEnabled:=True)
+end class", offeredWhenRequireAllParenthesesForClarityIsEnabled:=True, checkOverflow:=False)
+        End Function
+
+        <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)>
+        Public Async Function TestArithmeticNotRequiredForClarityWhenPrecedenceStaysTheSame1_CheckOverflow() As Task
+            Await TestMissingAsync(
+"class C
+    sub M()
+        dim x = 1 + $$(2 + 3)
+    end sub
+end class", New TestParameters(options:=RequireArithmeticBinaryParenthesesForClarity))
         End Function
 
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)>
@@ -587,7 +605,7 @@ end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses), firstL
 
         <WorkItem(27925, "https://github.com/dotnet/roslyn/issues/27925")>
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)>
-        Public Async Function TestUnnecessaryParenthesisDiagnosticInNestedExpression() As Task
+        Public Async Function TestUnnecessaryParenthesisDiagnosticInNestedExpression_DoNotCheckOverflow() As Task
             Dim outerParentheticalExpressionDiagnostic = GetRemoveUnnecessaryParenthesesDiagnostic("(1 + (2 + 3) + 4)", 2, 16)
             Dim innerParentheticalExpressionDiagnostic = GetRemoveUnnecessaryParenthesesDiagnostic("(2 + 3)", 2, 21)
             Dim expectedDiagnostics = New DiagnosticDescription() {outerParentheticalExpressionDiagnostic, innerParentheticalExpressionDiagnostic}
@@ -596,12 +614,12 @@ end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses), firstL
     sub M()
         dim x = [|(1 + (2 + 3) + 4)|]
     end sub
-end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses), expectedDiagnostics)
+end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses, compilationOptions:=DoNotCheckOverflow), expectedDiagnostics)
         End Function
 
         <WorkItem(27925, "https://github.com/dotnet/roslyn/issues/27925")>
         <Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryParentheses)>
-        Public Async Function TestUnnecessaryParenthesisDiagnosticInNestedMultiLineExpression() As Task
+        Public Async Function TestUnnecessaryParenthesisDiagnosticInNestedMultiLineExpression_DoNotCheckOverflow() As Task
             Dim outerFirstLineParentheticalExpressionDiagnostic = GetRemoveUnnecessaryParenthesesDiagnostic("(1 + 2 +", 2, 16)
             Dim innerParentheticalExpressionDiagnostic = GetRemoveUnnecessaryParenthesesDiagnostic("(3 + 4)", 3, 12)
             Dim expectedDiagnostics = New DiagnosticDescription() {outerFirstLineParentheticalExpressionDiagnostic, innerParentheticalExpressionDiagnostic}
@@ -612,7 +630,7 @@ end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses), expect
             (3 + 4) +
             5 + 6)|]
     end sub
-end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses), expectedDiagnostics)
+end class", New TestParameters(options:=RemoveAllUnnecessaryParentheses, compilationOptions:=DoNotCheckOverflow), expectedDiagnostics)
         End Function
 
         <WorkItem(27925, "https://github.com/dotnet/roslyn/issues/39529")>

src/Workspaces/Core/Portable/LanguageServices/SemanticsFactsService/SemanticFacts/AbstractSemanticFactsService.cs

@@ -2,8 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
+using System;
 using System.Threading;
 
 namespace Microsoft.CodeAnalysis.LanguageServices

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ParenthesizedExpressionSyntaxExtensions.cs

@@ -8,7 +8,7 @@
 using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
-using Microsoft.CodeAnalysis.Extensions;
+using Microsoft.CodeAnalysis.LanguageServices;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Roslyn.Utilities;
@@ -417,9 +417,8 @@ private static bool RemovalChangesAssociation(
                         node.Expression.Kind() == parentBinaryExpression.Kind() &&
                         parentBinaryExpression.Right == node)
                     {
-                        return !node.IsSafeToChangeAssociativity(
-                            node.Expression, parentBinaryExpression.Left,
-                            parentBinaryExpression.Right, semanticModel);
+                        return !CSharpSemanticFacts.Instance.IsSafeToChangeAssociativity(
+                            (BinaryExpressionSyntax)node.Expression, parentBinaryExpression, semanticModel);
                     }
 
                     // Null-coalescing is right associative; removing parens from the LHS changes the association.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs

@@ -30,6 +30,8 @@ private CSharpSemanticFacts()
         {
         }
 
+        public ISyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;
+
         public bool SupportsImplicitInterfaceImplementation => true;
 
         public bool ExposesAnonymousFunctionParameterNames => false;

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/CompilerExtensions.projitems

@@ -253,7 +253,6 @@
     <Compile Include="$(MSBuildThisFileDirectory)Extensions\ImmutableArrayExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Extensions\LinkedListExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Extensions\LocationExtensions.cs" />
-    <Compile Include="$(MSBuildThisFileDirectory)Extensions\ParenthesizedExpressionSyntaxExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)FlowAnalysis\CustomDataFlowAnalysis.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)FlowAnalysis\DataFlowAnalyzer.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)FlowAnalysis\FlowCaptureKind.cs" />
@@ -377,6 +376,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Options\RoamingProfileStorageLocation.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Services\SelectedMembers\AbstractSelectedMembers.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Services\SemanticFacts\ForEachSymbols.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Services\SemanticFacts\ISemanticFactsExtensions.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Services\SyntaxFacts\AbstractDocumentationCommentService.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Services\FileBannerFacts\AbstractFileBannerFacts.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Services\HeaderFacts\AbstractHeaderFacts.cs" />

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SemanticFacts/ISemanticFacts.cs

@@ -13,6 +13,8 @@ namespace Microsoft.CodeAnalysis.LanguageServices
 {
     internal partial interface ISemanticFacts
     {
+        ISyntaxFacts SyntaxFacts { get; }
+
         /// <summary>
         /// True if this language supports implementing an interface by signature only. If false,
         /// implementations must specific explicitly which symbol they're implementing.

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/ParenthesizedExpressionSyntaxExtensions.cs → src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Services/SemanticFacts/ISemanticFactsExtensions.cs

@@ -3,14 +3,18 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Diagnostics.CodeAnalysis;
+using Microsoft.CodeAnalysis.Operations;
 
-namespace Microsoft.CodeAnalysis.Extensions
+namespace Microsoft.CodeAnalysis.LanguageServices
 {
-    internal static class CommonParenthesizedExpressionSyntaxExtensions
+    internal static class ISemanticFactsExtensions
     {
-        public static bool IsSafeToChangeAssociativity(
-            this SyntaxNode parenthesizedExpression, SyntaxNode innerExpression,
-            SyntaxNode parentBinaryLeft, SyntaxNode parentBinaryRight, SemanticModel semanticModel)
+        public static bool IsSafeToChangeAssociativity<TBinaryExpressionSyntax>(
+            this ISemanticFacts semanticFacts,
+            TBinaryExpressionSyntax innerBinary,
+            TBinaryExpressionSyntax parentBinary,
+            SemanticModel semanticModel)
+            where TBinaryExpressionSyntax : SyntaxNode
         {
             // 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 
@@ -21,28 +25,24 @@ public static bool IsSafeToChangeAssociativity(
 
             // First, does the binary expression result in an operator overload being 
             // called?
-            var symbolInfo = semanticModel.GetSymbolInfo(innerExpression);
+            var symbolInfo = semanticModel.GetSymbolInfo(innerBinary);
             if (AnySymbolIsUserDefinedOperator(symbolInfo))
-            {
                 return false;
-            }
 
             // Second, check the type and converted type of the binary expression.
             // Are they the same?
-            var innerTypeInfo = semanticModel.GetTypeInfo(innerExpression);
+            var innerTypeInfo = semanticModel.GetTypeInfo(innerBinary);
             if (innerTypeInfo.Type != null && innerTypeInfo.ConvertedType != null)
             {
                 if (!innerTypeInfo.Type.Equals(innerTypeInfo.ConvertedType))
-                {
                     return false;
-                }
             }
 
             // It's not safe to change associativity for dynamic variables as the actual type isn't known. See https://github.com/dotnet/roslyn/issues/47365
             if (innerTypeInfo.Type is IDynamicTypeSymbol)
-            {
                 return false;
-            }
+
+            semanticFacts.SyntaxFacts.GetPartsOfBinaryExpression(parentBinary, out var parentBinaryLeft, out var parentBinaryRight);
 
             // Only allow us to change associativity if all the types are the same.
             // for example, if we have: int + (int + long)  then we don't want to
@@ -59,23 +59,34 @@ public static bool IsSafeToChangeAssociativity(
                 return false;
             }
 
-            // Floating point is not safe to change associativity of.  For example,
-            // if the user has "large * (large * small)" then this will become 
-            // "(large * large) * small.  And that could easily overflow to Inf (and 
-            // other badness).
-            var parentBinary = parenthesizedExpression.Parent;
-            if (parentBinary is null)
-            {
-                return false;
-            }
-
+            // Floating point is not safe to change associativity of.  For example, if the user has "large * (large *
+            // small)" then this will become "(large * large) * small.  And that could easily overflow to Inf (and other
+            // badness).
             var outerTypeInfo = semanticModel.GetTypeInfo(parentBinary);
             if (IsFloatingPoint(innerTypeInfo) || IsFloatingPoint(outerTypeInfo))
-            {
                 return false;
+
+            if (semanticModel.GetOperation(parentBinary) is IBinaryOperation parentBinaryOp &&
+                semanticModel.GetOperation(innerBinary) is IBinaryOperation innerBinaryOp)
+            {
+                if ((parentBinaryOp.IsChecked || innerBinaryOp.IsChecked) &&
+                    (IsArithmetic(parentBinaryOp) || IsArithmetic(innerBinaryOp)))
+                {
+                    // For checked operations, we can't change which type of operator we're performing in a row as that
+                    // could lead to overflow if we end up doing something like an addition prior to a subtraction.
+                    return false;
+                }
             }
 
             return true;
+
+            static bool IsArithmetic(IBinaryOperation op)
+            {
+                return op.OperatorKind is BinaryOperatorKind.Add or
+                    BinaryOperatorKind.Subtract or
+                    BinaryOperatorKind.Multiply or
+                    BinaryOperatorKind.Divide;
+            }
         }
 
         private static bool AnySymbolIsUserDefinedOperator(SymbolInfo symbolInfo)

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Extensions/ParenthesizedExpressionSyntaxExtensions.vb

@@ -4,7 +4,7 @@
 
 Imports System.Runtime.CompilerServices
 Imports System.Threading
-Imports Microsoft.CodeAnalysis.Extensions
+Imports Microsoft.CodeAnalysis.LanguageServices
 Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
 
 Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
@@ -397,9 +397,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Extensions
                             If IsAssociative(parentBinaryExpression.Kind) AndAlso
                                expression.Kind = parentExpression.Kind Then
 
-                                Return node.IsSafeToChangeAssociativity(
-                                    node.Expression, parentBinaryExpression.Left,
-                                    parentBinaryExpression.Right, semanticModel)
+                                Return VisualBasicSemanticFacts.Instance.IsSafeToChangeAssociativity(
+                                    binaryExpression, parentBinaryExpression, semanticModel)
                             End If
                         End If
 

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Services/SemanticFacts/VisualBasicSemanticFacts.vb

@@ -19,6 +19,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
         Private Sub New()
         End Sub
 
+        Public ReadOnly Property SyntaxFacts As ISyntaxFacts Implements ISemanticFacts.SyntaxFacts
+            Get
+                Return VisualBasicSyntaxFacts.Instance
+            End Get
+        End Property
+
         Public ReadOnly Property SupportsImplicitInterfaceImplementation As Boolean Implements ISemanticFacts.SupportsImplicitInterfaceImplementation
             Get
                 Return False

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/CSharp/LanguageServices/CSharpSemanticFactsService.cs

@@ -11,7 +11,6 @@
 using Microsoft.CodeAnalysis.CSharp.Extensions;
 using Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery;
 using Microsoft.CodeAnalysis.CSharp.LanguageServices;
-using Microsoft.CodeAnalysis.CSharp.Shared.Extensions;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.LanguageServices;
 using Microsoft.CodeAnalysis.Operations;
@@ -24,7 +23,7 @@ internal sealed partial class CSharpSemanticFactsService : AbstractSemanticFacts
     {
         internal static readonly CSharpSemanticFactsService Instance = new();
 
-        protected override ISyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;
+        public override ISyntaxFacts SyntaxFacts => CSharpSyntaxFacts.Instance;
         protected override ISemanticFacts SemanticFacts => CSharpSemanticFacts.Instance;
 
         private CSharpSemanticFactsService()

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/LanguageServices/SemanticsFactsService/AbstractSemanticFactsService.cs

@@ -16,7 +16,7 @@ namespace Microsoft.CodeAnalysis.LanguageServices
 {
     internal abstract partial class AbstractSemanticFactsService : ISemanticFacts
     {
-        protected abstract ISyntaxFacts SyntaxFacts { get; }
+        public abstract ISyntaxFacts SyntaxFacts { get; }
         protected abstract ISemanticFacts SemanticFacts { get; }
 
         protected abstract SyntaxToken ToIdentifierToken(string identifier);

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/VisualBasic/LanguageServices/VisualBasicSemanticFactsService.vb

@@ -33,7 +33,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
 
         Public Shared ReadOnly Instance As New VisualBasicSemanticFactsService()
 
-        Protected Overrides ReadOnly Property SyntaxFacts As ISyntaxFacts = VisualBasicSyntaxFacts.Instance
+        Public Overrides ReadOnly Property SyntaxFacts As ISyntaxFacts = VisualBasicSyntaxFacts.Instance
         Protected Overrides ReadOnly Property SemanticFacts As ISemanticFacts = VisualBasicSemanticFacts.Instance
 
         Private Sub New()