From 948ac1bc575b602adceae8fe0c932828f94f37c1 Mon Sep 17 00:00:00 2001
From: Cyrus Najmabadi <cyrus.najmabadi@gmail.com>
Date: Thu, 17 Dec 2020 11:33:32 -0800
Subject: [PATCH] Do not introduce conditional conversions when they are not
 legal.

---
 .../RemoveUnnecessaryCastTests.cs             | 39 +++++++++++++++++++
 .../CSharp/Utilities/SpeculationAnalyzer.cs   | 16 +++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/src/Analyzers/CSharp/Tests/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.cs
index f99fe7cefd5..6a43f501a23 100644
--- a/src/Analyzers/CSharp/Tests/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.cs
+++ b/src/Analyzers/CSharp/Tests/RemoveUnnecessaryCast/RemoveUnnecessaryCastTests.cs
@@ -8417,5 +8417,44 @@ public static nint N(nint a, int b)
 
             await test.RunAsync();
         }
+
+        [WorkItem(50000, "https://github.com/dotnet/roslyn/issues/50000")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryCast)]
+        public async Task KeepNecessaryCastIfRemovalWouldCreateIllegalConditionalExpression()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+class C
+{
+    ushort Goo(string s)
+        => s is null ? (ushort)1234 : ushort.Parse(s);
+}
+",
+                LanguageVersion = LanguageVersion.CSharp8,
+            }.RunAsync();
+        }
+
+        [WorkItem(50000, "https://github.com/dotnet/roslyn/issues/50000")]
+        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsRemoveUnnecessaryCast)]
+        public async Task RemoveUnnecessaryCastWhenConditionalExpressionIsLegal()
+        {
+            await new VerifyCS.Test
+            {
+                TestCode = @"
+class C
+{
+    ushort Goo(string s)
+        => s is null ? [|(ushort)|]1234 : ushort.Parse(s);
+}",
+                FixedCode = @"
+class C
+{
+    ushort Goo(string s)
+        => s is null ? 1234 : ushort.Parse(s);
+}",
+                LanguageVersion = LanguageVersion.CSharp9,
+            }.RunAsync();
+        }
     }
 }
diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs
index c21c10819ac..cf9876fc777 100644
--- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs
+++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs
@@ -342,6 +342,17 @@ protected override bool ReplacementChangesSemanticsForNodeLanguageSpecific(Synta
             {
                 var newExpression = (ConditionalExpressionSyntax)currentReplacedNode;
 
+                // If we removed a cast, causing a conditional expression to now use a conditional *conversion*, then that is always
+                // an error before CSharp9, and should not be allowed.
+                var originalConditionalConversion = this.OriginalSemanticModel.GetConversion(originalExpression);
+                var newConditionalConversion = this.SpeculativeSemanticModel.GetConversion(newExpression);
+                if (newConditionalConversion.IsConditionalExpression &&
+                    !originalConditionalConversion.IsConditionalExpression &&
+                    !ConditionalExpressionConversionsAreAllowed(originalExpression))
+                {
+                    return true;
+                }
+
                 if (originalExpression.Condition != previousOriginalNode)
                 {
                     ExpressionSyntax originalOtherPartOfConditional, newOtherPartOfConditional;
@@ -719,9 +730,12 @@ private bool ReplacementBreaksQueryClause(QueryClauseSyntax originalClause, Quer
         private static bool IsPotentiallyTargetTypedConditionalExpression(ExpressionSyntax expressionSyntax)
         {
             return expressionSyntax is ConditionalExpressionSyntax &&
-                   ((CSharpParseOptions)expressionSyntax.SyntaxTree.Options).LanguageVersion >= LanguageVersion.CSharp9;
+                   ConditionalExpressionConversionsAreAllowed(expressionSyntax);
         }
 
+        private static bool ConditionalExpressionConversionsAreAllowed(ExpressionSyntax expressionSyntax)
+            => ((CSharpParseOptions)expressionSyntax.SyntaxTree.Options).LanguageVersion >= LanguageVersion.CSharp9;
+
         protected override bool ReplacementIntroducesErrorType(ExpressionSyntax originalExpression, ExpressionSyntax newExpression)
         {
             // The base implementation will see that the type of the new expression may potentially change to null,
-- 
GitLab