diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/DoubleCheckedLockingCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/DoubleCheckedLockingCheckSample.java index 091f4e63059..0412e4de107 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/DoubleCheckedLockingCheckSample.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/DoubleCheckedLockingCheckSample.java @@ -17,5 +17,18 @@ public UnknownType unknownType() { } } + public UnknownType unknownTypeNegated() { + if (!(unknownType != null)) { +// ^^^^^^^^^^^^^^^^^^^^^^> + synchronized (Helper.class) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(unknownType != null)) { +// ^^^^^^^^^^^^^^^^^^^^^^< + this.unknownType = new UnknownType(); + } + } + } + } + class Helper { } } diff --git a/java-checks-test-sources/default/src/main/java/checks/DoubleCheckedLockingCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/DoubleCheckedLockingCheckSample.java index b1e85c62d20..bd221c5aa6f 100644 --- a/java-checks-test-sources/default/src/main/java/checks/DoubleCheckedLockingCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/DoubleCheckedLockingCheckSample.java @@ -14,6 +14,42 @@ public Helper classicCase() { return helper; } + public Helper classicCaseNegated() { + if (!(helper != null)) +// ^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (helper == null) +// ^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseInnerNegated() { + if (helper == null) +// ^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(helper != null)) +// ^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseRepeatedNegations() { + if (((!!(helper == null)))) +// ^^^^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(!(!(helper != null)))) +// ^^^^^^^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + public Helper fieldIsNotActuallyInitializedHere() { if (helper == null) synchronized (this) { // Compliant - field is not initialized in this method actually @@ -250,6 +286,24 @@ public synchronized Helper synchronizedClassicCase() { return helper; } + public Helper negatedNullEqualityOuter() { + if (!(helper == null)) + synchronized (this) { // Compliant + if (helper == null) + helper = new Helper(); + } + return helper; + } + + public Helper doubleNegatedNotEqualInner() { + if (helper == null) + synchronized (this) { + if (!!(helper != null)) + helper = new Helper(); + } + return helper; + } + static class Helper { } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java b/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java index c68b7f11d3d..1aeb0481743 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/synchronization/DoubleCheckedLockingCheck.java @@ -16,7 +16,14 @@ */ package org.sonar.java.checks.synchronization; +import java.util.Arrays; +import java.util.Deque; +import java.util.LinkedList; +import java.util.List; +import java.util.Optional; +import java.util.stream.Stream; import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; import org.sonar.java.model.ModifiersUtils; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; @@ -34,19 +41,15 @@ import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; import org.sonar.plugins.java.api.tree.SyntaxToken; import org.sonar.plugins.java.api.tree.Tree; - -import java.util.Arrays; -import java.util.Deque; -import java.util.LinkedList; -import java.util.List; -import java.util.Optional; -import java.util.stream.Stream; +import org.sonar.plugins.java.api.tree.UnaryExpressionTree; import static org.sonar.plugins.java.api.tree.Tree.Kind.EQUAL_TO; import static org.sonar.plugins.java.api.tree.Tree.Kind.IDENTIFIER; import static org.sonar.plugins.java.api.tree.Tree.Kind.IF_STATEMENT; +import static org.sonar.plugins.java.api.tree.Tree.Kind.LOGICAL_COMPLEMENT; import static org.sonar.plugins.java.api.tree.Tree.Kind.MEMBER_SELECT; import static org.sonar.plugins.java.api.tree.Tree.Kind.METHOD; +import static org.sonar.plugins.java.api.tree.Tree.Kind.NOT_EQUAL_TO; import static org.sonar.plugins.java.api.tree.Tree.Kind.NULL_LITERAL; import static org.sonar.plugins.java.api.tree.Tree.Kind.SYNCHRONIZED_STATEMENT; @@ -90,10 +93,18 @@ private static Optional isIfFieldEqNull(Tree tree) { return Optional.empty(); } IfStatementTree ifTree = (IfStatementTree) tree; - if (!ifTree.condition().is(EQUAL_TO)) { + + ExpressionTree condition = ExpressionUtils.skipParentheses(ifTree.condition()); + boolean negated = false; + while (condition.is(LOGICAL_COMPLEMENT)) { + negated = !negated; + condition = ExpressionUtils.skipParentheses(((UnaryExpressionTree) condition).expression()); + } + if (!condition.is(negated ? NOT_EQUAL_TO : EQUAL_TO)) { return Optional.empty(); } - BinaryExpressionTree eqRelation = (BinaryExpressionTree) ifTree.condition(); + + BinaryExpressionTree eqRelation = (BinaryExpressionTree) condition; if (eqRelation.rightOperand().is(NULL_LITERAL)) { return isField(eqRelation.leftOperand()).map(f -> new IfFieldEqNull(ifTree, f)); }