Fix crash in use-null-prop fixer (#79340)

Author: CyrusNajmabadi

src/Analyzers/CSharp/Analyzers/UseNullPropagation/CSharpUseNullPropagationDiagnosticAnalyzer.cs

@@ -27,6 +27,8 @@ internal sealed class CSharpUseNullPropagationDiagnosticAnalyzer :
         IfStatementSyntax,
         ExpressionStatementSyntax>
 {
+    public static readonly CSharpUseNullPropagationDiagnosticAnalyzer Instance = new();
+
     protected override SyntaxKind IfStatementSyntaxKind
         => SyntaxKind.IfStatement;
 

src/Analyzers/CSharp/CodeFixes/UseNullPropagation/CSharpUseNullPropagationCodeFixProvider.cs

@@ -17,6 +17,7 @@ namespace Microsoft.CodeAnalysis.CSharp.UseNullPropagation;
 [method: ImportingConstructor]
 [method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
 internal sealed class CSharpUseNullPropagationCodeFixProvider() : AbstractUseNullPropagationCodeFixProvider<
+    CSharpUseNullPropagationDiagnosticAnalyzer,
     SyntaxKind,
     ExpressionSyntax,
     StatementSyntax,
@@ -31,6 +32,9 @@ internal sealed class CSharpUseNullPropagationCodeFixProvider() : AbstractUseNul
     ExpressionStatementSyntax,
     BracketedArgumentListSyntax>
 {
+    protected override CSharpUseNullPropagationDiagnosticAnalyzer Analyzer
+        => CSharpUseNullPropagationDiagnosticAnalyzer.Instance;
+
     private static BlockSyntax ReplaceBlockStatements(BlockSyntax block, StatementSyntax newInnerStatement)
         => block.WithStatements([newInnerStatement, .. block.Statements.Skip(1).Select(s => s.WithAdditionalAnnotations(Formatter.Annotation))]);
 

src/Analyzers/CSharp/Tests/UseNullPropagation/UseNullPropagationTests.cs

@@ -2628,4 +2628,40 @@ class C
             """,
             OutputKind.ConsoleApplication,
             LanguageVersionExtensions.CSharpNext);
+
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/79338")]
+    public Task TestNestedNullPropagation_DifferentForms()
+        => TestInRegularAndScript1Async(
+            """
+            using System;
+            
+            class C
+            {
+                string S;
+
+                void M(C c)
+                {
+                    [|if|] (c != null)
+                        c.X([|c == null ? null : c.S|]);
+                }
+
+                void X(string s) { }
+            }
+            """,
+            """
+            using System;
+            
+            class C
+            {
+                string S;
+            
+                void M(C c)
+                {
+                    c?.X(c?.S);
+                }
+            
+                void X(string s) { }
+            }
+            """,
+            languageVersion: LanguageVersionExtensions.CSharpNext);
 }

src/Analyzers/Core/Analyzers/Analyzers.projitems

@@ -104,6 +104,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)UseExplicitTupleName\UseExplicitTupleNameDiagnosticAnalyzer.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)UseNullPropagation\AbstractUseNullPropagationDiagnosticAnalyzer_IfStatement.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)UseNullPropagation\AbstractUseNullPropagationDiagnosticAnalyzer.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)UseNullPropagation\AnalysisResults.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)UseObjectInitializer\AbstractUseObjectInitializerDiagnosticAnalyzer.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)UseObjectInitializer\UseNamedMemberInitializerAnalyzer.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)UseSystemHashCode\UseSystemHashCodeDiagnosticAnalyzer.cs" />

src/Analyzers/Core/Analyzers/UseNullPropagation/AbstractUseNullPropagationDiagnosticAnalyzer.cs

@@ -79,6 +79,16 @@ protected abstract bool TryAnalyzePatternCondition(
         ISyntaxFacts syntaxFacts, TExpressionSyntax conditionNode,
         [NotNullWhen(true)] out TExpressionSyntax? conditionPartToCheck, out bool isEquals);
 
+    public (INamedTypeSymbol? expressionType, IMethodSymbol? referenceEqualsMethod) GetAnalysisSymbols(Compilation compilation)
+    {
+        var expressionType = compilation.ExpressionOfTType();
+        var objectType = compilation.GetSpecialType(SpecialType.System_Object);
+        var referenceEqualsMethod = objectType?.GetMembers(nameof(ReferenceEquals))
+            .OfType<IMethodSymbol>()
+            .FirstOrDefault(m => m is { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 2 });
+        return (expressionType, referenceEqualsMethod);
+    }
+
     protected override void InitializeWorker(AnalysisContext context)
     {
         context.RegisterCompilationStartAction(context =>
@@ -88,22 +98,33 @@ protected override void InitializeWorker(AnalysisContext context)
 
             var expressionType = context.Compilation.ExpressionOfTType();
 
-            var objectType = context.Compilation.GetSpecialType(SpecialType.System_Object);
-            var referenceEqualsMethod = objectType?.GetMembers(nameof(ReferenceEquals))
-                .OfType<IMethodSymbol>()
-                .FirstOrDefault(m => m is { DeclaredAccessibility: Accessibility.Public, Parameters.Length: 2 });
+            var (objectType, referenceEqualsMethod) = GetAnalysisSymbols(context.Compilation);
 
             var syntaxKinds = this.SyntaxFacts.SyntaxKinds;
             context.RegisterSyntaxNodeAction(
-                context => AnalyzeTernaryConditionalExpression(context, expressionType, referenceEqualsMethod),
+                context => AnalyzeTernaryConditionalExpressionAndReportDiagnostic(context, expressionType, referenceEqualsMethod),
                 syntaxKinds.Convert<TSyntaxKind>(syntaxKinds.TernaryConditionalExpression));
             context.RegisterSyntaxNodeAction(
-                context => AnalyzeIfStatement(context, referenceEqualsMethod),
+                context => AnalyzeIfStatementAndReportDiagnostic(context, referenceEqualsMethod),
                 IfStatementSyntaxKind);
         });
     }
 
-    private void AnalyzeTernaryConditionalExpression(
+    public (TExpressionSyntax conditionalPart, SyntaxNode whenPart)? GetPartsOfConditionalExpression(
+        SemanticModel semanticModel,
+        TConditionalExpressionSyntax conditionalExpression,
+        CancellationToken cancellationToken)
+    {
+        var (objectType, referenceEqualsMethod) = GetAnalysisSymbols(semanticModel.Compilation);
+        var analysisResult = AnalyzeTernaryConditionalExpression(
+            semanticModel, objectType, referenceEqualsMethod, conditionalExpression, cancellationToken);
+        if (analysisResult is null)
+            return null;
+
+        return (analysisResult.Value.ConditionPartToCheck, analysisResult.Value.WhenPartToCheck);
+    }
+
+    private void AnalyzeTernaryConditionalExpressionAndReportDiagnostic(
         SyntaxNodeAnalysisContext context,
         INamedTypeSymbol? expressionType,
         IMethodSymbol? referenceEqualsMethod)
@@ -115,6 +136,27 @@ private void AnalyzeTernaryConditionalExpression(
         if (!option.Value || ShouldSkipAnalysis(context, option.Notification))
             return;
 
+        var analysisResult = AnalyzeTernaryConditionalExpression(
+            context.SemanticModel, expressionType, referenceEqualsMethod, conditionalExpression, cancellationToken);
+        if (analysisResult is null)
+            return;
+
+        context.ReportDiagnostic(DiagnosticHelper.Create(
+            Descriptor,
+            conditionalExpression.GetLocation(),
+            option.Notification,
+            context.Options,
+            additionalLocations: [conditionalExpression.GetLocation()],
+            analysisResult.Value.Properties));
+    }
+
+    public ConditionalExpressionAnalysisResult? AnalyzeTernaryConditionalExpression(
+        SemanticModel semanticModel,
+        INamedTypeSymbol? expressionType,
+        IMethodSymbol? referenceEqualsMethod,
+        TConditionalExpressionSyntax conditionalExpression,
+        CancellationToken cancellationToken)
+    {
         var syntaxFacts = this.SyntaxFacts;
         syntaxFacts.GetPartsOfConditionalExpression(
             conditionalExpression, out var condition, out var whenTrue, out var whenFalse);
@@ -125,32 +167,31 @@ private void AnalyzeTernaryConditionalExpression(
         var whenFalseNode = (TExpressionSyntax)syntaxFacts.WalkDownParentheses(whenFalse);
 
         if (!TryAnalyzeCondition(
-                context, syntaxFacts, referenceEqualsMethod, conditionNode,
-                out var conditionPartToCheck, out var isEquals))
+                semanticModel, referenceEqualsMethod, conditionNode,
+                out var conditionPartToCheck, out var isEquals, cancellationToken))
         {
-            return;
+            return null;
         }
 
         // Needs to be of the form:
         //      x == null ? null : ...    or
         //      x != null ? ...  : null;
         if (isEquals && !syntaxFacts.IsNullLiteralExpression(whenTrueNode))
-            return;
+            return null;
 
         if (!isEquals && !syntaxFacts.IsNullLiteralExpression(whenFalseNode))
-            return;
+            return null;
 
         var whenPartToCheck = isEquals ? whenFalseNode : whenTrueNode;
 
-        var semanticModel = context.SemanticModel;
         var whenPartMatch = GetWhenPartMatch(syntaxFacts, semanticModel, conditionPartToCheck, whenPartToCheck, cancellationToken);
         if (whenPartMatch == null)
-            return;
+            return null;
 
         // can't use ?. on a pointer
         var whenPartType = semanticModel.GetTypeInfo(whenPartMatch, cancellationToken).Type;
         if (whenPartType is IPointerTypeSymbol)
-            return;
+            return null;
 
         var type = semanticModel.GetTypeInfo(conditionalExpression, cancellationToken).Type;
         if (type?.IsValueType == true)
@@ -160,7 +201,7 @@ private void AnalyzeTernaryConditionalExpression(
                 // User has something like:  If(str is nothing, nothing, str.Length)
                 // In this case, converting to str?.Length changes the type of this from
                 // int to int?
-                return;
+                return null;
             }
             // But for a nullable type, such as  If(c is nothing, nothing, c.nullable)
             // converting to c?.nullable doesn't affect the type
@@ -172,7 +213,7 @@ private void AnalyzeTernaryConditionalExpression(
             // `x == null ? x : x.M` cannot be converted to `x?.M` when M is a method symbol.
             var memberSymbol = semanticModel.GetSymbolInfo(whenPartToCheck, cancellationToken).GetAnySymbol();
             if (memberSymbol is IMethodSymbol)
-                return;
+                return null;
 
             // `x == null ? x : x.Value` will be converted to just 'x'.
             if (UseNullPropagationHelpers.IsSystemNullableValueProperty(memberSymbol))
@@ -181,12 +222,7 @@ private void AnalyzeTernaryConditionalExpression(
 
         // ?. is not available in expression-trees.  Disallow the fix in that case.
         if (this.SemanticFacts.IsInExpressionTree(semanticModel, conditionNode, expressionType, cancellationToken))
-            return;
-
-        var locations = ImmutableArray.Create(
-            conditionalExpression.GetLocation(),
-            conditionPartToCheck.GetLocation(),
-            whenPartToCheck.GetLocation());
+            return null;
 
         var whenPartIsNullable = whenPartType?.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T;
         var properties = whenPartIsNullable
@@ -196,23 +232,21 @@ private void AnalyzeTernaryConditionalExpression(
         if (isTrivialNullableValueAccess)
             properties = properties.Add(UseNullPropagationHelpers.IsTrivialNullableValueAccess, UseNullPropagationHelpers.IsTrivialNullableValueAccess);
 
-        context.ReportDiagnostic(DiagnosticHelper.Create(
-            Descriptor,
-            conditionalExpression.GetLocation(),
-            option.Notification,
-            context.Options,
-            locations,
-            properties));
+        return new(
+            conditionPartToCheck,
+            whenPartToCheck,
+            properties);
     }
 
     private bool TryAnalyzeCondition(
-        SyntaxNodeAnalysisContext context,
-        ISyntaxFacts syntaxFacts,
+        SemanticModel semanticModel,
         IMethodSymbol? referenceEqualsMethod,
         TExpressionSyntax condition,
         [NotNullWhen(true)] out TExpressionSyntax? conditionPartToCheck,
-        out bool isEquals)
+        out bool isEquals,
+        CancellationToken cancellationToken)
     {
+        var syntaxFacts = this.SyntaxFacts;
         condition = (TExpressionSyntax)syntaxFacts.WalkDownParentheses(condition);
         var conditionIsNegated = false;
         if (syntaxFacts.IsLogicalNotExpression(condition))
@@ -228,8 +262,7 @@ private bool TryAnalyzeCondition(
                     syntaxFacts, binaryExpression, out conditionPartToCheck, out isEquals),
 
             TInvocationExpressionSyntax invocation => TryAnalyzeInvocationCondition(
-                    context, syntaxFacts, referenceEqualsMethod, invocation,
-                    out conditionPartToCheck, out isEquals),
+                semanticModel, syntaxFacts, referenceEqualsMethod, invocation, out conditionPartToCheck, out isEquals, cancellationToken),
 
             _ => TryAnalyzePatternCondition(syntaxFacts, condition, out conditionPartToCheck, out isEquals),
         };
@@ -261,12 +294,13 @@ private static bool TryAnalyzeBinaryExpressionCondition(
     }
 
     private static bool TryAnalyzeInvocationCondition(
-        SyntaxNodeAnalysisContext context,
+        SemanticModel semanticModel,
         ISyntaxFacts syntaxFacts,
         IMethodSymbol? referenceEqualsMethod,
         TInvocationExpressionSyntax invocation,
         [NotNullWhen(true)] out TExpressionSyntax? conditionPartToCheck,
-        out bool isEquals)
+        out bool isEquals,
+        CancellationToken cancellationToken)
     {
         conditionPartToCheck = null;
         isEquals = true;
@@ -311,8 +345,6 @@ private static bool TryAnalyzeInvocationCondition(
             return false;
         }
 
-        var semanticModel = context.SemanticModel;
-        var cancellationToken = context.CancellationToken;
         var symbol = semanticModel.GetSymbolInfo(invocation, cancellationToken).Symbol;
         return referenceEqualsMethod.Equals(symbol);
     }
@@ -337,7 +369,8 @@ private static bool TryAnalyzeInvocationCondition(
         return conditionRightIsNull ? conditionLeft : conditionRight;
     }
 
-    internal static TExpressionSyntax? GetWhenPartMatch(
+#pragma warning disable CA1822 // Mark members as static.  Helper method that doesn't want to call through generic form.
+    public TExpressionSyntax? GetWhenPartMatch(
         ISyntaxFacts syntaxFacts,
         SemanticModel semanticModel,
         TExpressionSyntax expressionToMatch,
@@ -361,6 +394,7 @@ private static bool TryAnalyzeInvocationCondition(
             current = unwrapped;
         }
     }
+#pragma warning restore CA1822 // Mark members as static
 
     private static TExpressionSyntax RemoveObjectCastIfAny(
         ISyntaxFacts syntaxFacts, SemanticModel semanticModel, TExpressionSyntax node, CancellationToken cancellationToken)