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.

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

Fix S1125 FP: Type check for objects and dynamic types #8575

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ protected override void Initialize(SonarAnalysisContext context)
context.RegisterNodeAction(CheckNotEquals, SyntaxKind.NotEqualsExpression);
context.RegisterNodeAction(CheckConditional, SyntaxKind.ConditionalExpression);
context.RegisterNodeAction(CheckForLoopCondition, SyntaxKind.ForStatement);
base.Initialize(context);
}

private void CheckForLoopCondition(SonarSyntaxNodeReportingContext context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public abstract class BooleanLiteralUnnecessaryBase<TSyntaxKind> : SonarDiagnost
{
internal const string DiagnosticId = "S1125";

private ImmutableArray<INamedTypeSymbol> systemBooleanInterfaces;

protected enum ErrorLocation
{
// The BooleanLiteral node is highlighted
Expand All @@ -46,6 +48,12 @@ protected enum ErrorLocation
protected abstract bool IsTrue(SyntaxNode syntaxNode);
protected abstract bool IsFalse(SyntaxNode syntaxNode);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(x =>
{
systemBooleanInterfaces = x.Compilation.GetTypeByMetadataName(KnownType.System_Boolean).Interfaces;
});

// For C# 7 syntax
protected virtual bool IsInsideTernaryWithThrowExpression(SyntaxNode syntaxNode) => false;

Expand Down Expand Up @@ -145,7 +153,9 @@ protected bool CheckForNullabilityAndBooleanConstantsReport(SonarSyntaxNodeRepor
var left = Language.Syntax.RemoveParentheses(GetLeftNode(node));
var right = Language.Syntax.RemoveParentheses(GetRightNode(node));

if (CheckForNullability(left, right, context.SemanticModel))
if (right is null // Avoids DeclarationPattern or RecursivePattern
|| TypeShouldBeIgnored(left, context.SemanticModel)
|| TypeShouldBeIgnored(right, context.SemanticModel))
{
return true;
}
Expand Down Expand Up @@ -205,9 +215,13 @@ private Location CalculateExtendedLocation(SyntaxNode parent, bool isLeftSide)
return isLeftSide ? parent.CreateLocation(token) : token.CreateLocation(parent);
}

private static bool CheckForNullability(SyntaxNode left, SyntaxNode right, SemanticModel model) =>
right is null // Avoids DeclarationPattern or RecursivePattern
|| model.GetTypeInfo(left).Type.IsNullableBoolean()
|| model.GetTypeInfo(right).Type.IsNullableBoolean();
private bool TypeShouldBeIgnored(SyntaxNode node, SemanticModel model)
{
var type = model.GetTypeInfo(node).Type;
return type.IsNullableBoolean()
|| type is ITypeParameterSymbol
|| type.Is(KnownType.System_Object)
|| systemBooleanInterfaces.Any(x => x.Equals(type));

Choose a reason for hiding this comment

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

FYI: Regarding symbol "Equals" there is some fun coming up, when we upgrade Roslyn:
dotnet/roslyn-analyzers#3427
Symbol equality comparison should be explicit about nullable annotation handling. Our version of Roslyn does not contain SymbolEqualityComparer.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ protected override void Initialize(SonarAnalysisContext context)
context.RegisterNodeAction(CheckEquals, SyntaxKind.EqualsExpression);
context.RegisterNodeAction(CheckNotEquals, SyntaxKind.NotEqualsExpression);
context.RegisterNodeAction(CheckConditional, SyntaxKind.TernaryConditionalExpression);
base.Initialize(context);
}

private void CheckLogicalNot(SonarSyntaxNodeReportingContext context)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,17 @@ public void Foo(string key)
var x = (key is null) ? throw new ArgumentNullException(nameof(key)) : false;
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/7792
class ConvertibleGenericTypes
{
void ConvertibleToBool<T1, T2, T3>(T1 unconstrained, T2 constrainedToStruct, T3 constrainedToBoolInterface)
where T2 : struct
where T3 : IComparable<bool>
{
if (unconstrained is true) { }
if (constrainedToStruct is true) { }
if (constrainedToBoolInterface is true) { }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,17 @@ public void Foo(string key)
var x = (key is null) ? throw new ArgumentNullException(nameof(key)) : false;
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/7792
class ConvertibleGenericTypes
{
void ConvertibleToBool<T1, T2, T3>(T1 unconstrained, T2 constrainedToStruct, T3 constrainedToBoolInterface)
where T2 : struct
where T3 : IComparable<bool>
{
if (unconstrained is true) { }
if (constrainedToStruct is true) { }
if (constrainedToBoolInterface is true) { }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -214,4 +214,22 @@ void Method(bool cond)
if (cond) { } // Fixed
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/7792
class ObjectIsBool
{
void Object(object obj, Exception exc)
{
if (obj is true) { }
if (exc.Data["SomeKey"] is true) { }
}

void ConvertibleToBool(IComparable comparable, IComparable<bool> comparableBool, IEquatable<bool> equatable, IConvertible convertible)
{
if (comparable is true) { }
if (comparableBool is true) { }
if (equatable is true) { }
if (convertible is true) { }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,22 @@ void Method(bool cond)
if (cond == false) { } // Noncompliant, TP but code fix is wrong - it should be fixed to "if (!cond)"
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/7792
class ObjectIsBool
{
void Object(object obj, Exception exc)
{
if (obj is true) { }
if (exc.Data["SomeKey"] is true) { }
}

void ConvertibleToBool(IComparable comparable, IComparable<bool> comparableBool, IEquatable<bool> equatable, IConvertible convertible)
{
if (comparable is true) { }
if (comparableBool is true) { }
if (equatable is true) { }
if (convertible is true) { }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,8 @@ Public Class BooleanLiteralUnnecessary
End Sub

Function BooleanMethod() As Boolean
Return True
End Function
Return True
End Function

End Class

Expand Down Expand Up @@ -220,3 +220,13 @@ Public Class Repro
Return True
End Function
End Class

' https://github.com/SonarSource/sonar-dotnet/issues/7792
Class ObjectIsBool
Sub Method(obj As Object, exc As Exception)
If obj = True Then
End If
If exc.Data("SomeKey") = True Then
End If
End Sub
End Class
Loading