From b1d9dc29817122583d890f81f3430017a4bb78f6 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sun, 20 Aug 2023 19:50:03 +0300 Subject: [PATCH] Add minimal binary pattern support for null checks in DFA --- .../ValidateArgumentsOfPublicMethodsTests.cs | 70 +++++++++++++++++++ .../DataFlow/DataFlowOperationVisitor.cs | 55 +++++++++++++++ 2 files changed, 125 insertions(+) diff --git a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs index c67809df21..d77db2e121 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs @@ -6864,5 +6864,75 @@ End Class }, }.RunAsync(); } + + [Fact, WorkItem(6755, "https://github.com/dotnet/roslyn-analyzers/issues/6755")] + public async Task CSharp_BinaryPattern_HandlesNull() + { + var code = """ + public class C + { + public string M1(string[] obj) + { + if (obj is null or []) + { + return null; + } + + return obj.ToString(); + } + + public string M2(string[] obj) + { + if (obj is null or []) + { + return [|obj|].ToString(); + } + + return null; + } + } + """; + await new VerifyCS.Test + { + TestCode = code, + FixedCode = code, + LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp11, + }.RunAsync(); + } + + [Fact, WorkItem(6755, "https://github.com/dotnet/roslyn-analyzers/issues/6755")] + public async Task CSharp_BinaryPattern_HandlesNotNull() + { + var code = """ + public class C + { + public string M1(string[] obj) + { + if (obj is not null and []) + { + return obj.ToString(); + } + + return null; + } + + public string M2(string[] obj) + { + if (obj is not null and []) + { + return null; + } + + return [|obj|].ToString(); + } + } + """; + await new VerifyCS.Test + { + TestCode = code, + FixedCode = code, + LanguageVersion = CodeAnalysis.CSharp.LanguageVersion.CSharp11, + }.RunAsync(); + } } } diff --git a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs index a904bdeaa4..faae816149 100644 --- a/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs +++ b/src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs @@ -1612,6 +1612,25 @@ private void PerformPredicateAnalysisCore(IOperation operation, TAnalysisData ta case OperationKind.BinaryPattern: // These high level patterns should not be present in the lowered CFG: https://github.com/dotnet/roslyn/issues/47068 predicateValueKind = PredicateValueKind.Unknown; + + // We special case common null check to reduce false positives. But this implementation for BinaryPattern is very incomplete. + if (FlowBranchConditionKind == ControlFlowConditionKind.WhenFalse) + { + var binaryPattern = (IBinaryPatternOperation)isPatternOperation.Pattern; + if (IsNotNullWhenFalse(binaryPattern)) + { + predicateValueKind = SetValueForIsNullComparisonOperator(isPatternOperation.Value, equals: false, targetAnalysisData: targetAnalysisData); + } + } + else if (FlowBranchConditionKind == ControlFlowConditionKind.WhenTrue) + { + var binaryPattern = (IBinaryPatternOperation)isPatternOperation.Pattern; + if (IsNotNullWhenTrue(binaryPattern)) + { + predicateValueKind = SetValueForIsNullComparisonOperator(isPatternOperation.Value, equals: false, targetAnalysisData: targetAnalysisData); + } + } + break; default: @@ -1761,6 +1780,42 @@ bool IsOverrideOrImplementationOfEquatableEquals(IMethodSymbol methodSymbol) return false; } + + bool IsNotNullWhenFalse(IOperation operation) + { + if (operation is IConstantPatternOperation constant && constant.Value.ConstantValue.HasValue && constant.Value.ConstantValue.Value is null) + { + // This is a null check. So we are not null on the false branch. + return true; + } + + if (operation is IBinaryPatternOperation { OperatorKind: BinaryOperatorKind.Or } binaryOrOperation) + { + // Example: if (c is null or "") + // The whole operation is not null when false, because one of the OR branches is not null when false. + return IsNotNullWhenFalse(binaryOrOperation.LeftPattern) || IsNotNullWhenFalse(binaryOrOperation.RightPattern); + } + + return false; + } + + bool IsNotNullWhenTrue(IOperation operation) + { + if (operation is INegatedPatternOperation negated && negated.Pattern is IConstantPatternOperation constant && constant.Value.ConstantValue.HasValue && constant.Value.ConstantValue.Value is null) + { + // This is a not null check. So we are not null on the true branch. + return true; + } + + if (operation is IBinaryPatternOperation { OperatorKind: BinaryOperatorKind.And } binaryOrOperation) + { + // Example: if (c is not null and "") + // The whole operation is not null when true, because one of the AND branches is not null when true. + return IsNotNullWhenTrue(binaryOrOperation.LeftPattern) || IsNotNullWhenTrue(binaryOrOperation.RightPattern); + } + + return false; + } } protected virtual void SetPredicateValueKind(IOperation operation, TAnalysisData analysisData, PredicateValueKind predicateValueKind)