Improve and clarify the logic when speculatively comparing old/new code to ensure semantics are the same.

src/Analyzers/CSharp/Analyzers/UseCollectionExpression/CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer.cs

@@ -25,7 +25,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UseCollectionExpression;
 internal sealed partial class CSharpUseCollectionExpressionForArrayDiagnosticAnalyzer
     : AbstractBuiltInCodeStyleDiagnosticAnalyzer
 {
-    private static readonly LiteralExpressionSyntax s_nullLiteralExpression = LiteralExpression(SyntaxKind.NullLiteralExpression);
+    private static readonly CollectionExpressionSyntax s_emptyCollectionExpression = CollectionExpression();
 
     public override DiagnosticAnalyzerCategory GetAnalyzerCategory()
         => DiagnosticAnalyzerCategory.SemanticSpanAnalysis;
@@ -214,16 +214,17 @@ private static void AnalyzeArrayInitializer(SyntaxNodeAnalysisContext context)
         }
 
         // Looks good as something to replace.  Now check the semantics of making the replacement to see if there would
-        // any issues.  To keep things simple, all we do is replace the existing expression with the `null` literal.
-        // This is a similarly 'untyped' literal (like a collection-expression is), so it tells us if the new code will
-        // have any issues moving to something untyped.  This will also tell us if we have any ambiguities (because
-        // there are multiple destination types that could accept the collection expression).
+        // any issues.  To keep things simple, all we do is replace the existing expression with the `[]` literal. This
+        // will tell us if we have problems assigning a collection expression to teh target type.
+        //
+        // Note: this does mean certain unambiguous cases with overloads (like `Goo(int[] values)` vs `Goo(string[]
+        // values)`) will not get simplification.  We can revisit this in the future to see if that warrants a more
+        // expensive check that involves checking the consitutuent elements of the literal.
         var speculationAnalyzer = new SpeculationAnalyzer(
             topmostExpression,
-            s_nullLiteralExpression,
+            s_emptyCollectionExpression,
             semanticModel,
             cancellationToken,
-            skipVerificationForReplacedNode: true,
             failOnOverloadResolutionFailuresInOriginalCode: true);
 
         if (speculationAnalyzer.ReplacementChangesSemantics())

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

@@ -85,7 +85,6 @@ private static bool CanReplaceWithDefaultLiteralSlow(
             var speculationAnalyzer = new SpeculationAnalyzer(
                 defaultExpression, s_defaultLiteralExpression, semanticModel,
                 cancellationToken,
-                skipVerificationForReplacedNode: false,
                 failOnOverloadResolutionFailuresInOriginalCode: true);
 
             return !speculationAnalyzer.ReplacementChangesSemantics();

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs

@@ -43,11 +43,6 @@ internal class SpeculationAnalyzer : AbstractSpeculationAnalyzer<
         /// <param name="newExpression">New expression to replace the original expression.</param>
         /// <param name="semanticModel">Semantic model of <paramref name="expression"/> node's syntax tree.</param>
         /// <param name="cancellationToken">Cancellation token.</param>
-        /// <param name="skipVerificationForReplacedNode">
-        /// True if semantic analysis should be skipped for the replaced node and performed starting from parent of the original and replaced nodes.
-        /// This could be the case when custom verifications are required to be done by the caller or
-        /// semantics of the replaced expression are different from the original expression.
-        /// </param>
         /// <param name="failOnOverloadResolutionFailuresInOriginalCode">
         /// True if semantic analysis should fail when any of the invocation expression ancestors of <paramref name="expression"/> in original code has overload resolution failures.
         /// </param>
@@ -56,9 +51,8 @@ public SpeculationAnalyzer(
             ExpressionSyntax newExpression,
             SemanticModel semanticModel,
             CancellationToken cancellationToken,
-            bool skipVerificationForReplacedNode = false,
             bool failOnOverloadResolutionFailuresInOriginalCode = false)
-            : base(expression, newExpression, semanticModel, cancellationToken, skipVerificationForReplacedNode, failOnOverloadResolutionFailuresInOriginalCode)
+            : base(expression, newExpression, semanticModel, skipVerificationForReplacedNode: false, failOnOverloadResolutionFailuresInOriginalCode, cancellationToken)
         {
         }
 
@@ -81,14 +75,12 @@ protected override SyntaxNode GetSemanticRootForSpeculation(ExpressionSyntax exp
 
         public static bool CanSpeculateOnNode(SyntaxNode node)
         {
-            return (node is StatementSyntax && node.Kind() != SyntaxKind.Block) ||
-                node is TypeSyntax ||
-                node is CrefSyntax ||
-                node.Kind() == SyntaxKind.Attribute ||
-                node.Kind() == SyntaxKind.ThisConstructorInitializer ||
-                node.Kind() == SyntaxKind.BaseConstructorInitializer ||
-                node.Kind() == SyntaxKind.EqualsValueClause ||
-                node.Kind() == SyntaxKind.ArrowExpressionClause;
+            return node is StatementSyntax(kind: not SyntaxKind.Block) or TypeSyntax or CrefSyntax ||
+                node.Kind() is SyntaxKind.Attribute or
+                               SyntaxKind.ThisConstructorInitializer or
+                               SyntaxKind.BaseConstructorInitializer or
+                               SyntaxKind.EqualsValueClause or
+                               SyntaxKind.ArrowExpressionClause;
         }
 
         protected override void ValidateSpeculativeSemanticModel(SemanticModel speculativeSemanticModel, SyntaxNode nodeToSpeculate)
@@ -506,7 +498,7 @@ newSwitchLabels[i] is CaseSwitchLabelSyntax newSwitchLabel &&
             }
             else if (currentOriginalNode.Kind() == SyntaxKind.ImplicitArrayCreationExpression)
             {
-                return !TypesAreCompatible((ImplicitArrayCreationExpressionSyntax)currentOriginalNode, (ImplicitArrayCreationExpressionSyntax)currentReplacedNode);
+                return !TypesAreCompatible((ExpressionSyntax)currentOriginalNode, (ExpressionSyntax)currentReplacedNode);
             }
             else if (currentOriginalNode is AnonymousObjectMemberDeclaratorSyntax originalAnonymousObjectMemberDeclarator)
             {
@@ -722,28 +714,65 @@ private bool ReplacementBreaksQueryClause(QueryClauseSyntax originalClause, Quer
                 !SymbolInfosAreCompatible(originalClauseInfo.OperationInfo, newClauseInfo.OperationInfo);
         }
 
-        protected override bool ReplacementIntroducesErrorType(ExpressionSyntax originalExpression, ExpressionSyntax newExpression)
+        protected override bool ReplacementIntroducesDisallowedNullType(
+            ExpressionSyntax originalExpression,
+            ExpressionSyntax newExpression,
+            TypeInfo originalTypeInfo,
+            TypeInfo newTypeInfo)
         {
-            // The base implementation will see that the type of the new expression may potentially change to null,
-            // because the expression has no type but can be converted to a conditional expression type. In that case,
-            // we don't want to consider the null type to be an error type.
-            if (newExpression.IsKind(SyntaxKind.ConditionalExpression) &&
-                ConditionalExpressionConversionsAreAllowed(newExpression) &&
-                this.SpeculativeSemanticModel.GetConversion(newExpression).IsConditionalExpression)
-            {
+            // If the base check is fine with the nullability of types before/after, then we're good and there are no
+            // more checks we need to do.
+            if (!base.ReplacementIntroducesDisallowedNullType(originalExpression, newExpression, originalTypeInfo, newTypeInfo))
                 return false;
-            }
 
-            // Similar to above, it's fine for a switch expression to potentially change to having a 'null' direct type
-            // (as long as a target-typed switch-expression conversion happened).  Note: unlike above, we don't have to
-            // check a language version since switch expressions always supported target-typed conversion.
-            if (newExpression.IsKind(SyntaxKind.SwitchExpression) &&
-                this.SpeculativeSemanticModel.GetConversion(newExpression).IsSwitchExpression)
+            // If, however, the base check is not ok.  That means that the old expression had an initial type, but the
+            // new expression does not.  This may or may not be ok depending on the construct.  If it's a supported
+            // construct then we want to check the new constructs converted type against the old construct's original
+            // type to make sure those still match.  If so, this change is fine.
+            if (IsSupportedConstructWithNullType() &&
+                SymbolsAreCompatible(originalTypeInfo.Type, newTypeInfo.ConvertedType))
             {
                 return false;
             }
 
-            return base.ReplacementIntroducesErrorType(originalExpression, newExpression);
+            return true;
+
+            bool IsSupportedConstructWithNullType()
+            {
+                // A conditional expression may become untyped if it now involves a conditional conversion.  For example:
+                //
+                //      int? s = x ? 0 : null;
+                //
+                // In this case, the null type is allowed if we do have a conditional-expression-conversion *and* the
+                // converted type matches the original type.
+                if (newExpression.IsKind(SyntaxKind.ConditionalExpression) &&
+                    ConditionalExpressionConversionsAreAllowed(newExpression) &&
+                    this.SpeculativeSemanticModel.GetConversion(newExpression).IsConditionalExpression)
+                {
+                    return true;
+                }
+
+                // Similar to above, it's fine for a switch expression to potentially change to having a 'null' direct type
+                // (as long as a target-typed switch-expression conversion happened).  Note: unlike above, we don't have to
+                // check a language version since switch expressions always supported target-typed conversion.
+                if (newExpression.IsKind(SyntaxKind.SwitchExpression) &&
+                    this.SpeculativeSemanticModel.GetConversion(newExpression).IsSwitchExpression &&
+                    SymbolsAreCompatible(originalTypeInfo.Type, newTypeInfo.ConvertedType))
+                {
+                    return true;
+                }
+
+                // Similar to above, it's fine for a collection expression to have a a 'null' direct type (as long as a
+                // target-typed collection-expression conversion happened).  Note: unlike above, we don't have to check
+                // a language version since collection expressions always supported collection-expression-conversions.
+                if (newExpression.IsKind(SyntaxKind.CollectionExpression) &&
+                    this.SpeculativeSemanticModel.GetConversion(newExpression).IsCollectionExpression)
+                {
+                    return true;
+                }
+
+                return false;
+            }
         }
 
         protected override bool ConversionsAreCompatible(SemanticModel originalModel, ExpressionSyntax originalExpression, SemanticModel newModel, ExpressionSyntax newExpression)

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AbstractSpeculationAnalyzer.cs

@@ -74,9 +74,9 @@ public AbstractSpeculationAnalyzer(
             TExpressionSyntax expression,
             TExpressionSyntax newExpression,
             SemanticModel semanticModel,
-            CancellationToken cancellationToken,
-            bool skipVerificationForReplacedNode = false,
-            bool failOnOverloadResolutionFailuresInOriginalCode = false)
+            bool skipVerificationForReplacedNode,
+            bool failOnOverloadResolutionFailuresInOriginalCode,
+            CancellationToken cancellationToken)
         {
             _expression = expression;
             _newExpressionForReplace = newExpression;
@@ -211,22 +211,24 @@ private void EnsureSpeculativeSemanticModel()
 
         #region Semantic comparison helpers
 
-        protected virtual bool ReplacementIntroducesErrorType(TExpressionSyntax originalExpression, TExpressionSyntax newExpression)
+        protected virtual bool ReplacementIntroducesDisallowedNullType(
+            TExpressionSyntax originalExpression,
+            TExpressionSyntax newExpression,
+            TypeInfo originalTypeInfo,
+            TypeInfo newTypeInfo)
         {
-            RoslynDebug.AssertNotNull(originalExpression);
-            Debug.Assert(this.SemanticRootOfOriginalExpression.DescendantNodesAndSelf().Contains(originalExpression));
-            RoslynDebug.AssertNotNull(newExpression);
-            Debug.Assert(this.SemanticRootOfReplacedExpression.DescendantNodesAndSelf().Contains(newExpression));
-
-            var originalTypeInfo = this.OriginalSemanticModel.GetTypeInfo(originalExpression);
-            var newTypeInfo = this.SpeculativeSemanticModel.GetTypeInfo(newExpression);
+            // If the original expression had no type, it's fine for the new one to have no type either.
             if (originalTypeInfo.Type == null)
-            {
                 return false;
-            }
 
-            return newTypeInfo.Type == null ||
-                (newTypeInfo.Type.IsErrorType() && !originalTypeInfo.Type.IsErrorType());
+            // If the original had a type, but the new expression doesn't, this is *normally* bad.  However, there are
+            // some cases where it is ok (untyped language expressions that have natural conversions to typed
+            // expressions).  Subclasses can override this for those cases.
+            if (newTypeInfo.Type == null)
+                return true;
+
+            // Otherwise, things look ok so far.
+            return false;
         }
 
         protected bool TypesAreCompatible(TExpressionSyntax originalExpression, TExpressionSyntax newExpression)
@@ -238,7 +240,19 @@ protected bool TypesAreCompatible(TExpressionSyntax originalExpression, TExpress
 
             var originalTypeInfo = this.OriginalSemanticModel.GetTypeInfo(originalExpression);
             var newTypeInfo = this.SpeculativeSemanticModel.GetTypeInfo(newExpression);
-            return SymbolsAreCompatible(originalTypeInfo.Type, newTypeInfo.Type);
+            if (SymbolsAreCompatible(originalTypeInfo.Type, newTypeInfo.Type))
+                return true;
+
+            // types changed between the old and new expression (specifically, the new type became null, while the
+            // original type was not).  That's ok in some circumstance.  Check for those and allow in that specific
+            // case.
+            if (originalTypeInfo.Type != null && newTypeInfo.Type == null &&
+                !ReplacementIntroducesDisallowedNullType(originalExpression, newExpression, originalTypeInfo, newTypeInfo))
+            {
+                return true;
+            }
+
+            return false;
         }
 
         protected bool ConvertedTypesAreCompatible(TExpressionSyntax originalExpression, TExpressionSyntax newExpression)
@@ -581,11 +595,23 @@ private bool ReplacementChangesSemanticsForNode(SyntaxNode currentOriginalNode,
             else if (currentOriginalNode is TExpressionSyntax originalExpression)
             {
                 var newExpression = (TExpressionSyntax)currentReplacedNode;
-                if (!ImplicitConversionsAreCompatible(originalExpression, newExpression) ||
-                    ReplacementIntroducesErrorType(originalExpression, newExpression))
-                {
+                if (!ImplicitConversionsAreCompatible(originalExpression, newExpression))
+                    return true;
+
+                RoslynDebug.AssertNotNull(originalExpression);
+                Debug.Assert(this.SemanticRootOfOriginalExpression.DescendantNodesAndSelf().Contains(originalExpression));
+                RoslynDebug.AssertNotNull(newExpression);
+                Debug.Assert(this.SemanticRootOfReplacedExpression.DescendantNodesAndSelf().Contains(newExpression));
+
+                var originalTypeInfo = this.OriginalSemanticModel.GetTypeInfo(originalExpression);
+                var newTypeInfo = this.SpeculativeSemanticModel.GetTypeInfo(newExpression);
+
+                // If we didn't have an error before, but now we got one, that's bad and should block conversion in all cases.
+                if (newTypeInfo.Type.IsErrorType() && !originalTypeInfo.Type.IsErrorType())
+                    return true;
+
+                if (ReplacementIntroducesDisallowedNullType(originalExpression, newExpression, originalTypeInfo, newTypeInfo))
                     return true;
-                }
             }
 
             return false;

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBasic/Utilities/SpeculationAnalyzer.vb

@@ -42,10 +42,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Utilities
         ''' True if semantic analysis should fail when any of the invocation expression ancestors of <paramref name="expression"/> in original code has overload resolution failures.
         ''' </param>        
         Public Sub New(expression As ExpressionSyntax, newExpression As ExpressionSyntax, semanticModel As SemanticModel, cancellationToken As CancellationToken, Optional skipVerificationForReplacedNode As Boolean = False, Optional failOnOverloadResolutionFailuresInOriginalCode As Boolean = False)
-            MyBase.New(expression, newExpression, semanticModel, cancellationToken, skipVerificationForReplacedNode, failOnOverloadResolutionFailuresInOriginalCode)
+            MyBase.New(expression, newExpression, semanticModel, skipVerificationForReplacedNode, failOnOverloadResolutionFailuresInOriginalCode, cancellationToken)
         End Sub
 
         Protected Overrides ReadOnly Property SyntaxFactsService As CodeAnalysis.LanguageService.ISyntaxFacts = VisualBasicSyntaxFacts.Instance
+
         Protected Overrides Function CanAccessInstanceMemberThrough(expression As ExpressionSyntax) As Boolean
             ' vb can reference an instance member by just writing `.X` (when in a 'with' block), or by writing Me.X,
             ' MyBase.X and MyClass.X (the latter is not just for accessing static members).