diff --git a/java-checks-test-sources/default/src/main/java/checks/FloatEquality.java b/java-checks-test-sources/default/src/main/java/checks/FloatEquality.java index 75cd035df59..479a6954a0b 100644 --- a/java-checks-test-sources/default/src/main/java/checks/FloatEquality.java +++ b/java-checks-test-sources/default/src/main/java/checks/FloatEquality.java @@ -1,5 +1,7 @@ package checks; +import static java.lang.Double.NaN; + class FloatEquality { void foo() { float f1 = 0.1f; @@ -51,4 +53,22 @@ void method(Double d1, Double d2, Float f1, Float f2) { if (f1.equals(f2)) { } // Noncompliant if (new Object().equals(f2)) { } //compliant } + + void nanComparisons() { + double d = 0.1d; + float f = 0.1f; + if (d == Double.NaN) {} // Compliant - covered by S9147 + if (d != Double.NaN) {} // Compliant - covered by S9147 + if (Double.NaN == d) {} // Compliant - covered by S9147 + if (f == Float.NaN) {} // Compliant - covered by S9147 + if (f != Float.NaN) {} // Compliant - covered by S9147 + if (Float.NaN == f) {} // Compliant - covered by S9147 + if (d == (Double.NaN)) {} // Compliant - covered by S9147 + if (f == (Float.NaN)) {} // Compliant - covered by S9147 + if (d == NaN) {} // Compliant - covered by S9147 (static import) + if (Double.NaN == Double.NaN) {} // Compliant - covered by S9147 (NaN on both sides) + if (f == f) {} // Compliant - NaN self-test pattern + if (d == Double.MAX_VALUE) {} // Noncompliant {{Equality tests should not be made with floating point values.}} + if (Double.NaN == Float.NaN) {} // Compliant - covered by S9147 + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java index 61560ef41b2..e437f336fc2 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java @@ -19,6 +19,7 @@ import java.util.Arrays; import java.util.List; import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; import org.sonar.java.model.SyntacticEquivalence; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.MethodMatchers; @@ -87,7 +88,9 @@ private static boolean isIndirectEquality(BinaryExpressionTree binaryExpressionT private static boolean isNanTest(BinaryExpressionTree binaryExpressionTree) { - return SyntacticEquivalence.areEquivalent(binaryExpressionTree.leftOperand(), binaryExpressionTree.rightOperand()); + return SyntacticEquivalence.areEquivalent(binaryExpressionTree.leftOperand(), binaryExpressionTree.rightOperand()) + || ExpressionUtils.getNanOwnerTypeName(binaryExpressionTree.leftOperand()) != null + || ExpressionUtils.getNanOwnerTypeName(binaryExpressionTree.rightOperand()) != null; } private static boolean hasFloatingType(ExpressionTree expressionTree) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/NanEqualityCheck.java b/java-checks/src/main/java/org/sonar/java/checks/NanEqualityCheck.java index 7fb4bfc7798..569ae494d35 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/NanEqualityCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/NanEqualityCheck.java @@ -17,16 +17,10 @@ package org.sonar.java.checks; import java.util.List; -import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ExpressionUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; -import org.sonar.plugins.java.api.semantic.Symbol; -import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; -import org.sonar.plugins.java.api.tree.ExpressionTree; -import org.sonar.plugins.java.api.tree.IdentifierTree; -import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.Tree; @Rule(key = "S9147") @@ -40,9 +34,9 @@ public List nodesToVisit() { @Override public void visitNode(Tree tree) { BinaryExpressionTree binaryExpression = (BinaryExpressionTree) tree; - String typeName = getNanTypeName(binaryExpression.leftOperand()); + String typeName = ExpressionUtils.getNanOwnerTypeName(binaryExpression.leftOperand()); if (typeName == null) { - typeName = getNanTypeName(binaryExpression.rightOperand()); + typeName = ExpressionUtils.getNanOwnerTypeName(binaryExpression.rightOperand()); } if (typeName != null) { reportIssue(binaryExpression.operatorToken(), @@ -50,37 +44,4 @@ public void visitNode(Tree tree) { } } - @Nullable - private static String getNanTypeName(ExpressionTree expression) { - ExpressionTree expr = ExpressionUtils.skipParentheses(expression); - if (expr.is(Tree.Kind.MEMBER_SELECT)) { - MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) expr; - if ("NaN".equals(memberSelect.identifier().name())) { - return resolveNanOwnerType(memberSelect.identifier().symbol()); - } - } else if (expr.is(Tree.Kind.IDENTIFIER)) { - IdentifierTree identifier = (IdentifierTree) expr; - if ("NaN".equals(identifier.name())) { - return resolveNanOwnerType(identifier.symbol()); - } - } - return null; - } - - @Nullable - private static String resolveNanOwnerType(Symbol symbol) { - if (symbol.isUnknown()) { - return null; - } - Symbol owner = symbol.owner(); - if (owner == null || owner.type() == null) { - return null; - } - Type ownerType = owner.type(); - if (ownerType.is("java.lang.Double") || ownerType.is("java.lang.Float")) { - return ownerType.name(); - } - return null; - } - } diff --git a/java-frontend/src/main/java/org/sonar/java/model/ExpressionUtils.java b/java-frontend/src/main/java/org/sonar/java/model/ExpressionUtils.java index 14517c13fdd..ad074e57213 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/ExpressionUtils.java +++ b/java-frontend/src/main/java/org/sonar/java/model/ExpressionUtils.java @@ -23,6 +23,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; @@ -403,4 +404,37 @@ public static String annotationAttributeName(ExpressionTree expression) { return "value"; } + @Nullable + public static String getNanOwnerTypeName(ExpressionTree expression) { + ExpressionTree expr = skipParentheses(expression); + if (expr.is(Tree.Kind.MEMBER_SELECT)) { + MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) expr; + if ("NaN".equals(memberSelect.identifier().name())) { + return resolveNanOwnerType(memberSelect.identifier().symbol()); + } + } else if (expr.is(Tree.Kind.IDENTIFIER)) { + IdentifierTree identifier = (IdentifierTree) expr; + if ("NaN".equals(identifier.name())) { + return resolveNanOwnerType(identifier.symbol()); + } + } + return null; + } + + @Nullable + private static String resolveNanOwnerType(Symbol symbol) { + if (symbol.isUnknown()) { + return null; + } + Symbol owner = symbol.owner(); + if (owner == null || owner.type() == null) { + return null; + } + Type ownerType = owner.type(); + if (ownerType.is("java.lang.Double") || ownerType.is("java.lang.Float")) { + return ownerType.name(); + } + return null; + } + } diff --git a/java-frontend/src/test/java/org/sonar/java/model/ExpressionUtilsTest.java b/java-frontend/src/test/java/org/sonar/java/model/ExpressionUtilsTest.java index e1f8c9aab6c..c880657fe69 100644 --- a/java-frontend/src/test/java/org/sonar/java/model/ExpressionUtilsTest.java +++ b/java-frontend/src/test/java/org/sonar/java/model/ExpressionUtilsTest.java @@ -41,11 +41,15 @@ import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.VariableTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; + import static java.lang.reflect.Modifier.isFinal; import static java.lang.reflect.Modifier.isPrivate; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import static org.sonar.java.model.ExpressionUtils.isInvocationOnVariable; import static org.sonar.java.model.ExpressionUtils.skipParenthesesUpwards; import static org.sonar.java.model.assertions.TreeAssert.assertThat; @@ -470,6 +474,130 @@ void areVariablesSame_unknown_symbol() { assertThat(ExpressionUtils.areVariablesSame(initializer.falseExpression(), condition.rightOperand(), false)).isFalse(); } + @Test + void getNanOwnerTypeName_member_select() { + // Double.NaN -> "Double" + assertNanOwnerTypeName("Double.NaN", "Double"); + // Float.NaN -> "Float" + assertNanOwnerTypeName("Float.NaN", "Float"); + // Non-NaN member select -> null + assertNanOwnerTypeName("Double.MAX_VALUE", null); + // Parenthesized expression (Double.NaN) -> "Double" + assertNanOwnerTypeName("(Double.NaN)", "Double"); + // Parenthesized expression (Float.NaN) -> "Float" + assertNanOwnerTypeName("(Float.NaN)", "Float"); + } + + @Test + void getNanOwnerTypeName_identifier_static_import() { + // Statically imported NaN from Double -> "Double" + CompilationUnitTree unit = JParserTestUtils.parse(""" + import static java.lang.Double.NaN; + class A { Object f = NaN; } + """); + ExpressionTree expr = ((VariableTree) ((ClassTree) unit.types().get(0)).members().get(0)).initializer(); + assertThat(ExpressionUtils.getNanOwnerTypeName(expr)).isEqualTo("Double"); + } + + @Test + void getNanOwnerTypeName_identifier_static_import_float() { + // Statically imported NaN from Float -> "Float" + CompilationUnitTree unit = JParserTestUtils.parse(""" + import static java.lang.Float.NaN; + class A { Object f = NaN; } + """); + ExpressionTree expr = ((VariableTree) ((ClassTree) unit.types().get(0)).members().get(0)).initializer(); + assertThat(ExpressionUtils.getNanOwnerTypeName(expr)).isEqualTo("Float"); + } + + @Test + void getNanOwnerTypeName_other_expression_types() { + // Literal -> null + assertNanOwnerTypeName("42.0", null); + // Method call -> null + assertNanOwnerTypeName("Double.valueOf(0.0)", null); + } + + @Test + void getNanOwnerTypeName_unknown_nan() { + // Unknown class NaN field -> null (symbol is unknown) + CompilationUnitTree unit = JParserTestUtils.parse(""" + class A { Object f = UnknownClass.NaN; } + """); + ExpressionTree expr = ((VariableTree) ((ClassTree) unit.types().get(0)).members().get(0)).initializer(); + assertThat(ExpressionUtils.getNanOwnerTypeName(expr)).isNull(); + } + + private void assertNanOwnerTypeName(String code, @Nullable String expected) { + CompilationUnitTree unit = JParserTestUtils.parse("class A { Object f = " + code + "; }"); + ExpressionTree expression = ((VariableTree) ((ClassTree) unit.types().get(0)).members().get(0)).initializer(); + String actual = ExpressionUtils.getNanOwnerTypeName(expression); + if (expected == null) { + assertThat(actual).isNull(); + } else { + assertThat(actual).isEqualTo(expected); + } + } + + @Test + void getNanOwnerTypeName_identifier_not_nan() { + // An identifier that is not named "NaN" should return null (line 417 false branch) + assertNanOwnerTypeName("someVariable", null); + } + + @Test + void getNanOwnerTypeName_custom_class_nan_field() { + // A NaN field on a custom class (not Double/Float) should return null (line 437) + CompilationUnitTree unit = JParserTestUtils.parse(""" + class MyClass { + static double NaN = 0.0; + } + class A { Object f = MyClass.NaN; } + """); + ExpressionTree expr = ((VariableTree) ((ClassTree) unit.types().get(1)).members().get(0)).initializer(); + assertThat(ExpressionUtils.getNanOwnerTypeName(expr)).isNull(); + } + + @Test + void getNanOwnerTypeName_member_select_not_nan() { + // A member select where the identifier is not "NaN" should return null (line 412 false branch) + assertNanOwnerTypeName("Double.MAX_VALUE", null); + } + + @Test + void getNanOwnerTypeName_null_owner_type_via_mock() { + // Test resolveNanOwnerType returns null when owner.type() is null (line 431) + Symbol nanSymbol = mock(Symbol.class); + when(nanSymbol.isUnknown()).thenReturn(false); + Symbol ownerSymbol = mock(Symbol.class); + when(ownerSymbol.type()).thenReturn(null); + when(nanSymbol.owner()).thenReturn(ownerSymbol); + + IdentifierTree identifier = mock(IdentifierTree.class); + when(identifier.name()).thenReturn("NaN"); + when(identifier.symbol()).thenReturn(nanSymbol); + when(identifier.is(Tree.Kind.MEMBER_SELECT)).thenReturn(false); + when(identifier.is(Tree.Kind.IDENTIFIER)).thenReturn(true); + + assertThat(ExpressionUtils.getNanOwnerTypeName(identifier)).isNull(); + } + + @Test + void getNanOwnerTypeName_null_owner_via_mock() { + // Test resolveNanOwnerType returns null when owner is null (line 431) + Symbol nanSymbol = mock(Symbol.class); + when(nanSymbol.isUnknown()).thenReturn(false); + when(nanSymbol.owner()).thenReturn(null); + + IdentifierTree identifier = mock(IdentifierTree.class); + when(identifier.name()).thenReturn("NaN"); + when(identifier.symbol()).thenReturn(nanSymbol); + when(identifier.is(Tree.Kind.MEMBER_SELECT)).thenReturn(false); + when(identifier.is(Tree.Kind.IDENTIFIER)).thenReturn(true); + + assertThat(ExpressionUtils.getNanOwnerTypeName(identifier)).isNull(); + } + @Test void testAnnotationAttributeName(){ var unit = JParserTestUtils.parse(""" diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9147.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9147.html index 3b090e2ff63..d6dcf7143a8 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9147.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9147.html @@ -56,6 +56,7 @@

Documentation

Related rules