diff --git a/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java deleted file mode 100644 index ecb8eb4112f..00000000000 --- a/java-checks-test-sources/default/src/main/java/checks/SynchronizedLockCheckSample.java +++ /dev/null @@ -1,42 +0,0 @@ -package checks; - -import java.util.concurrent.TimeUnit; -import java.util.concurrent.locks.Condition; -import java.util.concurrent.locks.Lock; - -class SynchronizedLockCheckSample { - void foo() { - Lock lock = new MyLockImpl(); - synchronized (lock) { // Noncompliant {{Synchronize on this "Lock" object using "acquire/release".}} -// ^^^^ - } - synchronized (new MyLockImpl()) { // Noncompliant {{Synchronize on this "Lock" object using "acquire/release".}} - } - synchronized (new UselessIncrementCheck()) { // Compliant - } - } -} - -class MyLockImpl implements Lock { - @Override - public void lock() { - } - @Override - public void lockInterruptibly() throws InterruptedException { - } - @Override - public boolean tryLock() { - return false; - } - @Override - public boolean tryLock(long time, TimeUnit unit) throws InterruptedException { - return false; - } - @Override - public void unlock() { - } - @Override - public Condition newCondition() { - return null; - } -} diff --git a/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java new file mode 100644 index 00000000000..05233a7a1e3 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/SynchronizedOnConcurrentObjectCheckSample.java @@ -0,0 +1,134 @@ +package checks; + +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.Future; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.Semaphore; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.locks.Condition; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; + +class SynchronizedOnConcurrentObjectCheckSample { + + private final ReentrantLock reentrantLock = new ReentrantLock(); + private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); + private final Lock lock = new ReentrantLock(); + private final Semaphore semaphore = new Semaphore(1); + private final CountDownLatch latch = new CountDownLatch(1); + private final CyclicBarrier barrier = new CyclicBarrier(2); + private final BlockingQueue blockingQueue = new ArrayBlockingQueue<>(10); + private final ArrayBlockingQueue arrayBlockingQueue = new ArrayBlockingQueue<>(10); + private final LinkedBlockingQueue linkedBlockingQueue = new LinkedBlockingQueue<>(); + private final AtomicBoolean atomicBoolean = new AtomicBoolean(); + private final AtomicInteger atomicInteger = new AtomicInteger(); + private final CustomLock customLock = new CustomLock(); + private final CustomLockImpl customLockImpl = new CustomLockImpl(); + + private final Object objectLock = new Object(); + private final ConcurrentHashMap concurrentMap = new ConcurrentHashMap<>(); + private Future future; + + void noncompliant() { + synchronized (reentrantLock) { // Noncompliant {{Use the "ReentrantLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (lock) { // Noncompliant {{Use the "Lock" API for synchronization instead of a "synchronized" block.}} + // ^^^^ + } + + synchronized (rwLock) { // Noncompliant {{Use the "ReentrantReadWriteLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^ + } + + synchronized (semaphore) { // Noncompliant {{Use the "Semaphore" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^ + } + + synchronized (latch) { // Noncompliant {{Use the "CountDownLatch" API for synchronization instead of a "synchronized" block.}} + // ^^^^^ + } + + synchronized (barrier) { // Noncompliant {{Use the "CyclicBarrier" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^ + } + + synchronized (blockingQueue) { // Noncompliant {{Use the "BlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (arrayBlockingQueue) { // Noncompliant {{Use the "ArrayBlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^^^^^ + } + + synchronized (linkedBlockingQueue) { // Noncompliant {{Use the "LinkedBlockingQueue" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^^^^^^ + } + + synchronized (atomicBoolean) { // Noncompliant {{Use the "AtomicBoolean" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (atomicInteger) { // Noncompliant {{Use the "AtomicInteger" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^ + } + + synchronized (customLock) { // Noncompliant {{Use the "CustomLock" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^ + } + + synchronized (customLockImpl) { // Noncompliant {{Use the "CustomLockImpl" API for synchronization instead of a "synchronized" block.}} + // ^^^^^^^^^^^^^^ + } + } + + void compliant() { + synchronized (objectLock) { + } + + synchronized (concurrentMap) { + } + + synchronized (future) { + } + + reentrantLock.lock(); + try { + } finally { + reentrantLock.unlock(); + } + + rwLock.writeLock().lock(); + try { + } finally { + rwLock.writeLock().unlock(); + } + } + + void example() { + var lock2 = new ReentrantLock(); + synchronized (lock2) { // Noncompliant + } + } + + static class CustomLock extends ReentrantLock { + } + + // Custom Lock implementation outside java.util.concurrent.locks — caught via isSubtypeOf(Lock) + static class CustomLockImpl implements Lock { + @Override public void lock() {} + @Override public void lockInterruptibly() throws InterruptedException {} + @Override public boolean tryLock() { return false; } + @Override public boolean tryLock(long time, TimeUnit unit) throws InterruptedException { return false; } + @Override public void unlock() {} + @Override public Condition newCondition() { return null; } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java deleted file mode 100644 index 23e523142b3..00000000000 --- a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedLockCheck.java +++ /dev/null @@ -1,45 +0,0 @@ -/* - * SonarQube Java - * Copyright (C) SonarSource Sàrl - * mailto:info AT sonarsource DOT com - * - * You can redistribute and/or modify this program under the terms of - * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. - * See the Sonar Source-Available License for more details. - * - * You should have received a copy of the Sonar Source-Available License - * along with this program; if not, see https://sonarsource.com/license/ssal/ - */ -package org.sonar.java.checks; - -import org.sonar.check.Rule; -import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; -import org.sonar.plugins.java.api.tree.ExpressionTree; -import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; -import org.sonar.plugins.java.api.tree.Tree; -import org.sonar.plugins.java.api.tree.Tree.Kind; - -import java.util.Collections; -import java.util.List; - -@Rule(key = "S2442") -public class SynchronizedLockCheck extends IssuableSubscriptionVisitor { - - @Override - public List nodesToVisit() { - return Collections.singletonList(Kind.SYNCHRONIZED_STATEMENT); - } - - @Override - public void visitNode(Tree tree) { - ExpressionTree expression = ((SynchronizedStatementTree) tree).expression(); - if (expression.symbolType().isSubtypeOf("java.util.concurrent.locks.Lock")) { - reportIssue(expression, "Synchronize on this \"Lock\" object using \"acquire/release\"."); - } - } - -} diff --git a/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java new file mode 100644 index 00000000000..3f9237e7f82 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheck.java @@ -0,0 +1,73 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.Collections; +import java.util.List; +import java.util.Set; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Type; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.SynchronizedStatementTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.Tree.Kind; + +@Rule(key = "S2442") +public class SynchronizedOnConcurrentObjectCheck extends IssuableSubscriptionVisitor { + + private static final String CONCURRENT_LOCKS_PREFIX = "java.util.concurrent.locks."; + private static final String CONCURRENT_ATOMIC_PREFIX = "java.util.concurrent.atomic."; + private static final Set CONCURRENT_SYNC_TYPES = Set.of( + "java.util.concurrent.Semaphore", + "java.util.concurrent.CountDownLatch", + "java.util.concurrent.CyclicBarrier", + "java.util.concurrent.Exchanger", + "java.util.concurrent.Phaser", + "java.util.concurrent.BlockingQueue", + "java.util.concurrent.BlockingDeque", + "java.util.concurrent.TransferQueue"); + + @Override + public List nodesToVisit() { + return Collections.singletonList(Kind.SYNCHRONIZED_STATEMENT); + } + + @Override + public void visitNode(Tree tree) { + ExpressionTree expression = ((SynchronizedStatementTree) tree).expression(); + Type type = expression.symbolType(); + if (isSynchronizationPrimitive(type)) { + reportIssue(expression, String.format( + "Use the \"%s\" API for synchronization instead of a \"synchronized\" block.", type.name())); + } + } + + private static boolean isSynchronizationPrimitive(Type type) { + return type.isSubtypeOf("java.util.concurrent.locks.Lock") + || isKnownSyncPrimitive(type) + || type.symbol().superTypes().stream().anyMatch(SynchronizedOnConcurrentObjectCheck::isKnownSyncPrimitive); + } + + private static boolean isKnownSyncPrimitive(Type type) { + String fqn = type.fullyQualifiedName(); + return fqn.startsWith(CONCURRENT_LOCKS_PREFIX) + || fqn.startsWith(CONCURRENT_ATOMIC_PREFIX) + || CONCURRENT_SYNC_TYPES.contains(fqn); + } + +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java similarity index 66% rename from java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java rename to java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java index cb03810805f..72ac10b1a2b 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/SynchronizedLockCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/SynchronizedOnConcurrentObjectCheckTest.java @@ -21,13 +21,23 @@ import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; -class SynchronizedLockCheckTest { +class SynchronizedOnConcurrentObjectCheckTest { @Test void test() { CheckVerifier.newVerifier() - .onFile(mainCodeSourcesPath("checks/SynchronizedLockCheckSample.java")) - .withCheck(new SynchronizedLockCheck()) + .onFile(mainCodeSourcesPath("checks/SynchronizedOnConcurrentObjectCheckSample.java")) + .withCheck(new SynchronizedOnConcurrentObjectCheck()) .verifyIssues(); } + + @Test + void test_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/SynchronizedOnConcurrentObjectCheckSample.java")) + .withCheck(new SynchronizedOnConcurrentObjectCheck()) + .withoutSemantic() + .verifyIssues(); + } + }