From 54709c7903e4bda6bd7b43b474a5920277e29242 Mon Sep 17 00:00:00 2001 From: David Schroeter Date: Mon, 2 Nov 2026 13:47:05 +0100 Subject: [PATCH 1/3] Add regression tests for negated double checked locking --- .../DoubleCheckedLockingCheckSample.java | 13 + .../DoubleCheckedLockingCheckSample.java | 245 ++++++++++++++++++ 2 files changed, 258 insertions(+) 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..d71464a334d 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,150 @@ 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 classicCaseBothNegated() { + if (!(helper != null)) +// ^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(helper != null)) +// ^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseOuterDoubleNegated() { + if (!!(helper == null)) +// ^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (helper == null) +// ^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseInnerDoubleNegated() { + if (helper == null) +// ^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!!(helper == null)) +// ^^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseOuterTripleNegated() { + if (!!!(helper != null)) +// ^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (helper == null) +// ^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseInnerTripleNegated() { + if (helper == null) +// ^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!!!(helper != null)) +// ^^^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper classicCaseNestedParentheses() { + if (((!(!((helper == null)))))) +// ^^^^^^^^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!((!((!(helper != null)))))) +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + return helper; + } + + public Helper memberSelectConditionNegated() { + if (!(this.helper != null)) +// ^^^^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (helper == null) +// ^^^^^^^^^^^^^^< + this.helper = new Helper(); + } + return helper; + } + + public Helper memberSelectCondition2Negated() { + if (helper == null) +// ^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(this.helper != null)) +// ^^^^^^^^^^^^^^^^^^^^^^< + this.helper = new Helper(); + } + return helper; + } + + public Helper memberSelectCondition3Negated() { + if (!(this.helper != null)) +// ^^^^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(this.helper != null)) +// ^^^^^^^^^^^^^^^^^^^^^^< + this.helper = new Helper(); + } + return helper; + } + + public Helper invertedConditionsNegated() { + if (!(null != this.helper)) +// ^^^^^^^^^^^^^^^^^^^^^^> + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (!(null != helper)) +// ^^^^^^^^^^^^^^^^^< + this.helper = new Helper(); + } + return helper; + } + public Helper fieldIsNotActuallyInitializedHere() { if (helper == null) synchronized (this) { // Compliant - field is not initialized in this method actually @@ -24,6 +168,16 @@ public Helper fieldIsNotActuallyInitializedHere() { return helper; } + public Helper fieldIsNotActuallyInitializedHereNegated() { + if (!(helper != null)) + synchronized (this) { // Compliant - field is not initialized in this method actually + if (!(helper != null)) { + System.out.println("haha!"); + } + } + return helper; + } + public Helper memberSelectCondition() { if (this.helper == null) // ^^^^^^^^^^^^^^^^^^^> @@ -129,6 +283,15 @@ public Helper classicCase() { return helper; } + public Helper classicCaseNegated() { + if (!(helper != null)) + synchronized (this) { // Compliant because field is volatile + if (!(helper != null)) + helper = new Helper(); + } + return helper; + } + static class Helper { } } @@ -155,6 +318,25 @@ public Helper unrelatedNestedIfs() { return helper; } + public Helper unrelatedNestedIfsNegated() { + if (!(null != helper)) { +// ^^^^^^^^^^^^^^^^^> + if (sunIsUp) { + doSomething(); + } + synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} +// ^^^^^^^^^^^^ + if (sunIsDown) { + doSomethingElse(); + if (!(null != helper)) +// ^^^^^^^^^^^^^^^^^< + helper = new Helper(); + } + } + } + return helper; + } + static class Helper { int field; } @@ -250,6 +432,69 @@ public synchronized Helper synchronizedClassicCase() { return helper; } + public synchronized Helper synchronizedClassicCaseNegated() { + if (!(helper != null)) + synchronized (this) { // Compliant + if (!(helper != null)) + helper = new Helper(); + } + return helper; + } + + public Helper negatedNullEqualityOuter() { + if (!(helper == null)) + synchronized (this) { // Compliant + if (helper == null) + helper = new Helper(); + } + return helper; + } + + public Helper negatedNullEqualityInner() { + if (helper == null) + synchronized (this) { // Compliant + if (!(helper == null)) + helper = new Helper(); + } + return helper; + } + + public Helper doubleNegatedNotEqualOuter() { + if (!!(helper != null)) + synchronized (this) { + if (helper == null) + helper = new Helper(); + } + return helper; + } + + public Helper doubleNegatedNotEqualInner() { + if (helper == null) + synchronized (this) { + if (!!(helper != null)) + helper = new Helper(); + } + return helper; + } + + public Helper tripleNegatedEqualOuter() { + if (!!!(helper == null)) + synchronized (this) { + if (helper == null) + helper = new Helper(); + } + return helper; + } + + public Helper tripleNegatedEqualInner() { + if (helper == null) + synchronized (this) { + if (!!!(helper == null)) + helper = new Helper(); + } + return helper; + } + static class Helper { } } From fa7afa36e9f5571407b69510b6ce2a98a7b654cf Mon Sep 17 00:00:00 2001 From: David Schroeter Date: Mon, 2 Nov 2026 13:54:44 +0100 Subject: [PATCH 2/3] Add implementation for negated double checked locking --- .../DoubleCheckedLockingCheck.java | 29 +++++++++++++------ 1 file changed, 20 insertions(+), 9 deletions(-) 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)); } From 2b534dc14765e266a1158cf17b58f2b6070b3944 Mon Sep 17 00:00:00 2001 From: David Schroeter Date: Mon, 2 Nov 2026 16:47:04 +0100 Subject: [PATCH 3/3] Remove redundent tests --- .../DoubleCheckedLockingCheckSample.java | 201 +----------------- 1 file changed, 5 insertions(+), 196 deletions(-) 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 d71464a334d..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 @@ -38,122 +38,14 @@ public Helper classicCaseInnerNegated() { return helper; } - public Helper classicCaseBothNegated() { - if (!(helper != null)) -// ^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!(helper != null)) -// ^^^^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper classicCaseOuterDoubleNegated() { - if (!!(helper == null)) -// ^^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (helper == null) -// ^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper classicCaseInnerDoubleNegated() { - if (helper == null) -// ^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!!(helper == null)) -// ^^^^^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper classicCaseOuterTripleNegated() { - if (!!!(helper != null)) -// ^^^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (helper == null) -// ^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper classicCaseInnerTripleNegated() { - if (helper == null) -// ^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!!!(helper != null)) -// ^^^^^^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper classicCaseNestedParentheses() { - if (((!(!((helper == null)))))) -// ^^^^^^^^^^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!((!((!(helper != null)))))) -// ^^^^^^^^^^^^^^^^^^^^^^^^^^^< - helper = new Helper(); - } - return helper; - } - - public Helper memberSelectConditionNegated() { - if (!(this.helper != null)) -// ^^^^^^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (helper == null) -// ^^^^^^^^^^^^^^< - this.helper = new Helper(); - } - return helper; - } - - public Helper memberSelectCondition2Negated() { - if (helper == null) -// ^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!(this.helper != null)) -// ^^^^^^^^^^^^^^^^^^^^^^< - this.helper = new Helper(); - } - return helper; - } - - public Helper memberSelectCondition3Negated() { - if (!(this.helper != null)) -// ^^^^^^^^^^^^^^^^^^^^^^> - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (!(this.helper != null)) -// ^^^^^^^^^^^^^^^^^^^^^^< - this.helper = new Helper(); - } - return helper; - } - - public Helper invertedConditionsNegated() { - if (!(null != this.helper)) + public Helper classicCaseRepeatedNegations() { + if (((!!(helper == null)))) // ^^^^^^^^^^^^^^^^^^^^^^> synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} // ^^^^^^^^^^^^ - if (!(null != helper)) -// ^^^^^^^^^^^^^^^^^< - this.helper = new Helper(); + if (!(!(!(helper != null)))) +// ^^^^^^^^^^^^^^^^^^^^^^^< + helper = new Helper(); } return helper; } @@ -168,16 +60,6 @@ public Helper fieldIsNotActuallyInitializedHere() { return helper; } - public Helper fieldIsNotActuallyInitializedHereNegated() { - if (!(helper != null)) - synchronized (this) { // Compliant - field is not initialized in this method actually - if (!(helper != null)) { - System.out.println("haha!"); - } - } - return helper; - } - public Helper memberSelectCondition() { if (this.helper == null) // ^^^^^^^^^^^^^^^^^^^> @@ -283,15 +165,6 @@ public Helper classicCase() { return helper; } - public Helper classicCaseNegated() { - if (!(helper != null)) - synchronized (this) { // Compliant because field is volatile - if (!(helper != null)) - helper = new Helper(); - } - return helper; - } - static class Helper { } } @@ -318,25 +191,6 @@ public Helper unrelatedNestedIfs() { return helper; } - public Helper unrelatedNestedIfsNegated() { - if (!(null != helper)) { -// ^^^^^^^^^^^^^^^^^> - if (sunIsUp) { - doSomething(); - } - synchronized (this) { // Noncompliant {{Remove this dangerous instance of double-checked locking.}} -// ^^^^^^^^^^^^ - if (sunIsDown) { - doSomethingElse(); - if (!(null != helper)) -// ^^^^^^^^^^^^^^^^^< - helper = new Helper(); - } - } - } - return helper; - } - static class Helper { int field; } @@ -432,15 +286,6 @@ public synchronized Helper synchronizedClassicCase() { return helper; } - public synchronized Helper synchronizedClassicCaseNegated() { - if (!(helper != null)) - synchronized (this) { // Compliant - if (!(helper != null)) - helper = new Helper(); - } - return helper; - } - public Helper negatedNullEqualityOuter() { if (!(helper == null)) synchronized (this) { // Compliant @@ -450,24 +295,6 @@ public Helper negatedNullEqualityOuter() { return helper; } - public Helper negatedNullEqualityInner() { - if (helper == null) - synchronized (this) { // Compliant - if (!(helper == null)) - helper = new Helper(); - } - return helper; - } - - public Helper doubleNegatedNotEqualOuter() { - if (!!(helper != null)) - synchronized (this) { - if (helper == null) - helper = new Helper(); - } - return helper; - } - public Helper doubleNegatedNotEqualInner() { if (helper == null) synchronized (this) { @@ -477,24 +304,6 @@ public Helper doubleNegatedNotEqualInner() { return helper; } - public Helper tripleNegatedEqualOuter() { - if (!!!(helper == null)) - synchronized (this) { - if (helper == null) - helper = new Helper(); - } - return helper; - } - - public Helper tripleNegatedEqualInner() { - if (helper == null) - synchronized (this) { - if (!!!(helper == null)) - helper = new Helper(); - } - return helper; - } - static class Helper { } }