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 S2583 FP: Should not raise on double condition #8475

Merged
merged 3 commits into from
Dec 20, 2023
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
19 changes: 15 additions & 4 deletions analyzers/src/SonarAnalyzer.Common/Helpers/TypeHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using Microsoft.CodeAnalysis;
using static Google.Protobuf.WellKnownTypes.Field;

namespace SonarAnalyzer.Helpers;

internal static class TypeHelper
Expand Down Expand Up @@ -106,6 +109,12 @@ public static bool IsAny(this ITypeSymbol typeSymbol, ImmutableArray<KnownType>
return false;
}

public static bool IsNullableOfAny(this ITypeSymbol type, ImmutableArray<KnownType> argumentTypes) =>
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
NullableTypeArgument(type).IsAny(argumentTypes);

public static bool IsNullableOf(this ITypeSymbol type, KnownType typeArgument) =>
NullableTypeArgument(type).Is(typeArgument);

public static bool IsType(this IParameterSymbol parameter, KnownType type) =>
parameter != null && parameter.Type.Is(type);

Expand All @@ -121,10 +130,7 @@ public static bool IsInType(this ISymbol symbol, ImmutableArray<KnownType> types
#endregion TypeName

public static bool IsNullableBoolean(this ITypeSymbol type) =>
type is INamedTypeSymbol namedType
&& namedType.OriginalDefinition.Is(KnownType.System_Nullable_T)
&& namedType.TypeArguments.Length == 1
&& namedType.TypeArguments[0].Is(KnownType.System_Boolean);
type.IsNullableOf(KnownType.System_Boolean);

public static bool Implements(this ITypeSymbol typeSymbol, KnownType type) =>
typeSymbol is { }
Expand Down Expand Up @@ -205,4 +211,9 @@ public static ITypeSymbol GetSymbolType(this ISymbol symbol) =>
ITypeSymbol x => x,
_ => null,
};

private static ITypeSymbol NullableTypeArgument(ITypeSymbol type) =>
type is INamedTypeSymbol namedType && namedType.OriginalDefinition.Is(KnownType.System_Nullable_T)
? namedType.TypeArguments[0]
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
: null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,24 @@ private static ProgramState LearnBranchingNumberConstraint(ProgramState state, I
var kind = falseBranch ? Opposite(binary.OperatorKind) : binary.OperatorKind;
var leftNumber = state[binary.LeftOperand]?.Constraint<NumberConstraint>();
var rightNumber = state[binary.RightOperand]?.Constraint<NumberConstraint>();
if (rightNumber is not null && !binary.LeftOperand.ConstantValue.HasValue && binary.LeftOperand.TrackedSymbol(state) is { } leftSymbol)
if (rightNumber is not null && OperandSymbol(binary.LeftOperand) is { } leftSymbol)
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
state = LearnBranching(leftSymbol, leftNumber, kind, rightNumber);
}
if (leftNumber is not null && !binary.RightOperand.ConstantValue.HasValue && binary.RightOperand.TrackedSymbol(state) is { } rightSymbol)
if (leftNumber is not null && OperandSymbol(binary.RightOperand) is { } rightSymbol)
{
state = LearnBranching(rightSymbol, rightNumber, Flip(kind), leftNumber);
}
return state;

ISymbol OperandSymbol(IOperation operand) =>
!operand.ConstantValue.HasValue
&& operand.TrackedSymbol(state) is { } symbol
&& symbol.GetSymbolType() is INamedTypeSymbol type
&& (type.IsAny(KnownType.IntegralNumbersIncludingNative) || type.IsNullableOfAny(KnownType.IntegralNumbersIncludingNative))
cristian-ambrosini-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
? symbol
: null;

ProgramState LearnBranching(ISymbol symbol, NumberConstraint existingNumber, BinaryOperatorKind kind, NumberConstraint comparedNumber) =>
!(falseBranch && symbol.GetSymbolType().IsNullableValueType()) // Don't learn opposite for "nullable > 0", because it could also be <null>.
&& RelationalNumberConstraint(existingNumber, kind, comparedNumber, isLoopCondition, visitCount) is { } newConstraint
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -777,4 +777,48 @@ static CollectionConstraint Constraint(bool? empty) =>
_ => null
};
}

[DataTestMethod]
[DataRow("3.14")] // Double
[DataRow("3.14M")] // Decimal
[DataRow("3.14F")] // Float
public void Binary_Relation_FloatingPoint_DoesNotLearnNumberConstraint(string value)
{
var code = $$"""
var value = {{value}};
if (value > 0)
{
Tag("If", value);
}
else
{
Tag("Else", value);
}
""";
var validator = SETestContext.CreateCS(code).Validator;
validator.TagValue("If").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
validator.TagValue("Else").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
}

[DataTestMethod]
[DataRow("3.14")] // Double
[DataRow("3.14M")] // Decimal
[DataRow("3.14F")] // Float
public void Binary_Equals_FloatingPoint_DoesNotLearnNumberConstraint(string value)
{
var code = $$"""
var value = {{value}};
if (value == 0)
{
Tag("If", value);
}
else
{
Tag("Else", value);
}
""";
var validator = SETestContext.CreateCS(code).Validator;
validator.TagValue("If").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
validator.TagValue("Else").Should().HaveOnlyConstraint(ObjectConstraint.NotNull);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3602,3 +3602,63 @@ public void TestMethod()

private void Callback() { }
}

// https://github.com/SonarSource/sonar-dotnet/issues/8470
public class Repro_8470
{
public string WithDouble()
{
double t = 0.5;
if (t <= 0)
{
return "a";
}
if (t >= 1) // Compliant, we don't track floating point numbers
{
return "b";
}
return "c";
}

public string WithDoubleSwappedOperands()
{
double t = 0.5;
if (0 >= t)
{
return "a";
}
if (1 <= t) // Compliant, we don't track floating point numbers
{
return "b";
}
return "c";
}

public string WithDecimal()
{
decimal t = 0.5M;
if (t <= 0)
{
return "a";
}
if (t >= 1) // Compliant, we don't track floating point numbers
{
return "b";
}
return "c";
}

public string WithFloat()
{
float t = 0.5F;
if (t <= 0)
{
return "a";
}
if (t >= 1) // Compliant, we don't track floating point numbers
{
return "b";
}
return "c";
}
}