Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimal binary pattern support for null checks in DFA #6877

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Add minimal binary pattern support for null checks in DFA #6877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions ...icrosoft.CodeQuality.Analyzers/QualityGuidelines/ValidateArgumentsOfPublicMethodsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
55 changes: 55 additions & 0 deletions src/Utilities/FlowAnalysis/FlowAnalysis/Framework/DataFlow/DataFlowOperationVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice optimization! Generic flow analysis handling for binary pattern operations is going to be non-trivial as there are sort of conditional branches with the and and or operators. I think this is a good starting point to handle the most important cases.

{
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:
Expand Down Expand Up @@ -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)
Expand Down