提交 2ac5d2c2 编写于 作者: C caikang.ck

1. add rule LockShouldWithTryFinallyRule

2. fix some issue
上级 545c0089
......@@ -69,6 +69,7 @@ configuration/**
# sass gitignore#
.sass-cache
.idea
.vscode
# tcc_coverage
coverage.ec
......@@ -88,3 +89,4 @@ hsf.configuration/
#hsf test
*.instance
/target/
......@@ -9,7 +9,7 @@
</parent>
<groupId>com.alibaba.p3c</groupId>
<artifactId>p3c-pmd</artifactId>
<version>2.0.0</version>
<version>2.0.1</version>
<packaging>jar</packaging>
<name>p3c-pmd</name>
<properties>
......@@ -17,6 +17,7 @@
<pmd.version>6.15.0</pmd.version>
<maven.compiler.target>1.8</maven.compiler.target>
<annotation.version>1.3.2</annotation.version>
<kotlin.version>1.3.50</kotlin.version>
</properties>
<description>Alibaba Java Coding Guidelines PMD implementations</description>
<url>https://github.com/alibaba/p3c</url>
......@@ -99,6 +100,11 @@
<artifactId>javax.annotation-api</artifactId>
<version>${annotation.version}</version>
</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib-jdk8</artifactId>
<version>${kotlin.version}</version>
</dependency>
</dependencies>
<build>
<plugins>
......@@ -142,6 +148,37 @@
</dependency>
</dependencies>
</plugin>
<plugin>
<artifactId>kotlin-maven-plugin</artifactId>
<groupId>org.jetbrains.kotlin</groupId>
<version>${kotlin.version}</version>
<executions>
<execution>
<id>compile</id>
<goals>
<goal>compile</goal>
</goals>
<configuration>
<sourceDirs>
<sourceDir>${project.basedir}/src/main/kotlin</sourceDir>
<sourceDir>${project.basedir}/src/main/java</sourceDir>
</sourceDirs>
</configuration>
</execution>
<execution>
<id>test-compile</id>
<goals>
<goal>test-compile</goal>
</goals>
<configuration>
<sourceDirs>
<sourceDir>${project.basedir}/src/test/kotlin</sourceDir>
<sourceDir>${project.basedir}/src/test/java</sourceDir>
</sourceDirs>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
......@@ -151,6 +188,32 @@
<target>${maven.compiler.target}</target>
<encoding>UTF-8</encoding>
</configuration>
<executions>
<!-- Replacing default-compile as it is treated specially by maven -->
<execution>
<id>default-compile</id>
<phase>none</phase>
</execution>
<!-- Replacing default-testCompile as it is treated specially by maven -->
<execution>
<id>default-testCompile</id>
<phase>none</phase>
</execution>
<execution>
<id>java-compile</id>
<phase>compile</phase>
<goals>
<goal>compile</goal>
</goals>
</execution>
<execution>
<id>java-test-compile</id>
<phase>test-compile</phase>
<goals>
<goal>testCompile</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<artifactId>maven-assembly-plugin</artifactId>
......@@ -190,20 +253,38 @@
</tags>
</configuration>
</plugin>
<!-- <plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-gpg-plugin</artifactId>
<version>1.6</version>
<executions>
<execution>
<id>sign-artifacts</id>
<phase>verify</phase>
<goals>
<goal>sign</goal>
</goals>
</execution>
</executions>
</plugin>-->
<plugin>
<artifactId>maven-assembly-plugin</artifactId>
<version>3.0.0</version>
<configuration>
<descriptorRefs>
<descriptorRef>jar-with-dependencies</descriptorRef>
</descriptorRefs>
</configuration>
<executions>
<execution>
<id>make-assembly</id>
<phase>package</phase>
<goals>
<goal>single</goal>
</goals>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-gpg-plugin</artifactId>
<version>1.6</version>
<executions>
<execution>
<id>sign-artifacts</id>
<phase>verify</phase>
<goals>
<goal>sign</goal>
</goals>
</execution>
</executions>
</plugin>
</plugins>
</build>
</project>
......@@ -19,7 +19,6 @@ import java.text.SimpleDateFormat;
import java.util.HashSet;
import java.util.Set;
import java.util.Stack;
import java.util.concurrent.locks.Lock;
import com.alibaba.p3c.pmd.lang.java.rule.AbstractAliRule;
......@@ -37,6 +36,10 @@ import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import net.sourceforge.pmd.lang.java.ast.AbstractJavaNode;
import net.sourceforge.pmd.lang.java.ast.Token;
import static com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.isLockNode;
import static com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.isLockStatementExpression;
import static com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.isUnLockStatementExpression;
/**
* [Mandatory] SimpleDataFormat is unsafe, do not define it as a static variable.
* If have to, lock or DateUtils class must be used.
......@@ -46,8 +49,6 @@ import net.sourceforge.pmd.lang.java.ast.Token;
*/
public class AvoidCallStaticSimpleDateFormatRule extends AbstractAliRule {
private static final String FORMAT_METHOD_NAME = "format";
private static final String LOCK_NAME = "lock";
private static final String UN_LOCK_NAME = "unlock";
@Override
public Object visit(ASTMethodDeclaration node, Object data) {
......@@ -101,7 +102,9 @@ public class AvoidCallStaticSimpleDateFormatRule extends AbstractAliRule {
// add lock node
stack.push(flowNode.getNode());
return;
} else if (isUnLockStatementExpression(statementExpression)) {
}
if (isUnLockStatementExpression(statementExpression)) {
// remove element in lock block
while (!stack.isEmpty()) {
Node node = stack.pop();
......@@ -132,16 +135,10 @@ public class AvoidCallStaticSimpleDateFormatRule extends AbstractAliRule {
return name.getImage();
}
private boolean isLockNode(Node node) {
if (!(node instanceof ASTStatementExpression)) {
return false;
}
ASTStatementExpression statementExpression = (ASTStatementExpression)node;
return isLockStatementExpression(statementExpression);
}
private boolean isStaticSimpleDateFormatCall(ASTPrimaryExpression primaryExpression,
Set<String> localSimpleDateFormatNames) {
private boolean isStaticSimpleDateFormatCall(
ASTPrimaryExpression primaryExpression,
Set<String> localSimpleDateFormatNames
) {
if (primaryExpression.jjtGetNumChildren() == 0) {
return false;
}
......@@ -149,6 +146,9 @@ public class AvoidCallStaticSimpleDateFormatRule extends AbstractAliRule {
if (name == null || name.getType() != SimpleDateFormat.class) {
return false;
}
if (name.getNameDeclaration() == null || name.getNameDeclaration().getName() == null) {
return false;
}
if (localSimpleDateFormatNames.contains(name.getNameDeclaration().getName())) {
return false;
}
......@@ -161,20 +161,4 @@ public class AvoidCallStaticSimpleDateFormatRule extends AbstractAliRule {
return FORMAT_METHOD_NAME.equals(token.image);
}
private boolean isLockStatementExpression(ASTStatementExpression statementExpression) {
return isLockTypeAndMethod(statementExpression, LOCK_NAME);
}
private boolean isUnLockStatementExpression(ASTStatementExpression statementExpression) {
return isLockTypeAndMethod(statementExpression, UN_LOCK_NAME);
}
private boolean isLockTypeAndMethod(ASTStatementExpression statementExpression, String methodName) {
ASTName name = statementExpression.getFirstDescendantOfType(ASTName.class);
if (name == null || name.getType() == null || !Lock.class.isAssignableFrom(name.getType())) {
return false;
}
Token token = (Token)name.jjtGetLastToken();
return methodName.equals(token.image);
}
}
......@@ -19,8 +19,8 @@ import java.util.List;
import java.util.concurrent.ThreadFactory;
import com.alibaba.p3c.pmd.lang.java.rule.AbstractAliRule;
import com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils;
import net.sourceforge.pmd.lang.java.ast.ASTAllocationExpression;
import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement;
import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceDeclaration;
......
......@@ -15,6 +15,8 @@
*/
package com.alibaba.p3c.pmd.lang.java.rule.naming;
import java.util.regex.Pattern;
import com.alibaba.p3c.pmd.I18nResources;
import com.alibaba.p3c.pmd.lang.java.rule.AbstractAliRule;
import com.alibaba.p3c.pmd.lang.java.util.StringAndCharConstants;
......@@ -27,8 +29,6 @@ import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclarator;
import net.sourceforge.pmd.lang.java.ast.ASTTypeDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId;
import java.util.regex.Pattern;
/**
* [Mandatory] Method names, parameter names, member variable names, and local variable names should be written in
* lowerCamelCase.
......@@ -39,7 +39,6 @@ import java.util.regex.Pattern;
public class LowerCamelCaseVariableNamingRule extends AbstractAliRule {
private static final String MESSAGE_KEY_PREFIX = "java.naming.LowerCamelCaseVariableNamingRule.violation.msg";
private Pattern pattern = Pattern.compile("^[a-z][a-z0-9]*([A-Z][a-z0-9]+)*(DO|DTO|VO|DAO|BO|DOList|DTOList|VOList|DAOList|BOList|X|Y|Z|UDF|UDAF|[A-Z])?$");
@Override
......
......@@ -65,4 +65,5 @@ public class WrapperTypeEqualityRule extends AbstractAliRule {
return "length".equals(expression.jjtGetLastToken().getImage())
&& ".".equals(expression.jjtGetFirstToken().getNext().getImage());
}
}
......@@ -71,7 +71,7 @@ public class ClassCastExceptionWithToArrayRule extends AbstractAliRule {
if (childName.endsWith(".toArray") && suffix.getArgumentCount() == 0
&& primarySuffixs.size() == 1) {
addViolationWithMessage(data, item,
"java.set.ClassCastExceptionWithSubListToArrayListRule.violation.msg",
"java.set.ClassCastExceptionWithToArrayRule.violation.msg",
new Object[] {childName});
}
}
......
......@@ -15,10 +15,15 @@
*/
package com.alibaba.p3c.pmd.lang.java.rule.util;
import java.util.concurrent.locks.Lock;
import net.sourceforge.pmd.lang.ast.Node;
import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration;
import net.sourceforge.pmd.lang.java.ast.ASTName;
import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression;
import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression;
import net.sourceforge.pmd.lang.java.ast.AbstractJavaAccessTypeNode;
import net.sourceforge.pmd.lang.java.ast.Token;
import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper;
/**
......@@ -26,6 +31,10 @@ import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper;
* @date 2016/11/16
*/
public class NodeUtils {
public static final String LOCK_NAME = "lock";
public static final String LOCK_INTERRUPTIBLY_NAME = "lockInterruptibly";
public static final String UN_LOCK_NAME = "unlock";
public static boolean isParentOrSelf(Node descendant, Node ancestor) {
if (descendant == ancestor) {
return true;
......@@ -64,4 +73,29 @@ public class NodeUtils {
public static Class<?> getNodeType(AbstractJavaAccessTypeNode node) {
return node == null ? null : node.getType();
}
public static boolean isLockStatementExpression(ASTStatementExpression statementExpression) {
return isLockTypeAndMethod(statementExpression, LOCK_NAME);
}
public static boolean isUnLockStatementExpression(ASTStatementExpression statementExpression) {
return isLockTypeAndMethod(statementExpression, UN_LOCK_NAME);
}
private static boolean isLockTypeAndMethod(ASTStatementExpression statementExpression, String methodName) {
ASTName name = statementExpression.getFirstDescendantOfType(ASTName.class);
if (name == null || name.getType() == null || !Lock.class.isAssignableFrom(name.getType())) {
return false;
}
Token token = (Token)name.jjtGetLastToken();
return methodName.equals(token.image);
}
public static boolean isLockNode(Node node) {
if (!(node instanceof ASTStatementExpression)) {
return false;
}
ASTStatementExpression statementExpression = (ASTStatementExpression)node;
return isLockStatementExpression(statementExpression);
}
}
package com.alibaba.p3c.pmd.lang.java.rule.concurrent
import com.alibaba.p3c.pmd.lang.java.rule.AbstractAliRule
import com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.LOCK_INTERRUPTIBLY_NAME
import com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.LOCK_NAME
import com.alibaba.p3c.pmd.lang.java.rule.util.NodeUtils.UN_LOCK_NAME
import net.sourceforge.pmd.lang.java.ast.ASTBlock
import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement
import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement
import net.sourceforge.pmd.lang.java.ast.ASTName
import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression
import net.sourceforge.pmd.lang.java.ast.ASTTryStatement
import net.sourceforge.pmd.lang.java.ast.AbstractJavaNode
import java.util.concurrent.locks.Lock
/**
* @author caikang
* @date 2019/09/29
*/
open class LockShouldWithTryFinallyRule : AbstractAliRule() {
override fun visit(node: ASTBlock, data: Any): Any? {
checkBlock(node, data)
return super.visit(node, data)
}
private fun checkBlock(block: ASTBlock, data: Any) {
val statements = block.findChildrenOfType(ASTBlockStatement::class.java)
if (statements.isNullOrEmpty()) {
return
}
var lockExpression: ASTStatementExpression? = null
for (statement in statements) {
if (lockExpression != null) {
// check try finally
val tryStatement = findNodeByXpath(
statement, XPATH_TRY_STATEMENT,
ASTTryStatement::class.java
)
if (!checkTryStatement(tryStatement)) {
addLockViolation(data, lockExpression)
}
lockExpression = null
continue
}
// find lock expression
val expression = findNodeByXpath(
statement, XPATH_LOCK_STATEMENT,
ASTStatementExpression::class.java
) ?: continue
if (!expression.isLock) {
continue
}
val astName = expression.getFirstDescendantOfType(ASTName::class.java)
val lockMethod = astName?.image?.let {
it.endsWith(".$LOCK_NAME") || it.endsWith(".$LOCK_INTERRUPTIBLY_NAME")
} ?: false
if (!lockMethod) {
continue
}
lockExpression = expression
}
lockExpression?.let {
addLockViolation(data, it)
}
}
private fun addLockViolation(data: Any, lockExpression: ASTStatementExpression) {
addViolationWithMessage(
data, lockExpression,
"java.concurrent.LockShouldWithTryFinallyRule.violation.msg",
arrayOf<Any>(getExpressName(lockExpression))
)
}
private fun checkTryStatement(tryStatement: ASTTryStatement?): Boolean {
if (tryStatement == null) {
return false
}
val finallyStatement = tryStatement.getFirstChildOfType(ASTFinallyStatement::class.java) ?: return false
val statementExpression = findNodeByXpath(
finallyStatement,
XPATH_UNLOCK_STATEMENT, ASTStatementExpression::class.java
) ?: return false
if (!statementExpression.isLock) {
return false
}
val astName = statementExpression.getFirstDescendantOfType(ASTName::class.java) ?: return false
return astName.image?.endsWith(".$UN_LOCK_NAME") ?: false
}
private fun <T> findNodeByXpath(statement: AbstractJavaNode, xpath: String, clazz: Class<T>): T? {
val nodes = statement.findChildNodesWithXPath(xpath)
if (nodes == null || nodes.isEmpty()) {
return null
}
val node = nodes[0]
return if (!clazz.isAssignableFrom(node.javaClass)) {
null
} else clazz.cast(node)
}
private fun getExpressName(statementExpression: ASTStatementExpression): String {
val name = statementExpression.getFirstDescendantOfType(ASTName::class.java)
return name.image
}
companion object {
private const val XPATH_LOCK_STATEMENT = "Statement/StatementExpression"
private const val XPATH_UNLOCK_STATEMENT = "Block/BlockStatement/Statement/StatementExpression"
private const val XPATH_TRY_STATEMENT = "Statement/TryStatement"
}
private val ASTStatementExpression?.isLock: Boolean
get() = this?.type?.let {
Lock::class.java.isAssignableFrom(it)
} ?: false
}
......@@ -178,6 +178,15 @@
<entry key="java.concurrent.ThreadShouldSetNameRule.rule.msg">
<![CDATA[创建线程或线程池时请指定有意义的线程名称,方便出错时回溯。创建线程池的时候请使用带ThreadFactory的构造函数,并且提供自定义ThreadFactory实现或者使用第三方实现。]]>
</entry>
<entry key="java.concurrent.LockShouldWithTryFinallyRule.violation.msg">
<![CDATA[锁【%s】必须紧跟try代码块,且unlock要放到finally第一行。]]>
</entry>
<entry key="java.concurrent.LockShouldWithTryFinallyRule.rule.msg">
<![CDATA[在使用阻塞等待获取锁的方式中,必须在try代码块之外,并且在加锁方法与try代码块之间没有任何可能抛出异常的方法调用,避免加锁成功后,在finally中无法解锁。
说明一:如果在lock方法与try代码块之间的方法调用抛出异常,那么无法解锁,造成其它线程无法成功获取锁。
说明二:如果lock方法在try代码块之内,可能由于其它方法抛出异常,导致在finally代码块中,unlock对未加锁的对象解锁,它会调用AQS的tryRelease方法(取决于具体实现类),抛出IllegalMonitorStateException异常。
说明三:在Lock对象的lock方法实现中可能抛出unchecked异常,产生的后果与说明二相同。]]>
</entry>
<!-- flowcontrol -->
<entry key="java.flowcontrol.SwitchStatementRule.violation.nodefault">
......
......@@ -180,7 +180,15 @@ Note: Below are the problems created by usage of Executors for thread pool creat
<entry key="java.concurrent.ThreadShouldSetNameRule.rule.msg">
<![CDATA[A meaningful thread name is helpful to trace the error information,so assign a name when creating threads or thread pools.]]>
</entry>
<entry key="java.concurrent.LockShouldWithTryFinallyRule.violation.msg">
<![CDATA[Lock operation [%s] must immediately follow by try block, and unlock operation must be placed in the first line of finally block.]]>
</entry>
<entry key="java.concurrent.LockShouldWithTryFinallyRule.rule.msg">
<![CDATA[When getting the lock by blocking methods, such as waiting in the blocking queue, lock() must be put outside the try block. Besides, make sure there is no method that throws Exception between the lock() and try block, in case the lock won't be released in the finally block.
Explain 1: If there was any Exception thrown between the lock() and try block, it won't be able to release the lock, causing that the other threads cannot get the lock.
Explain 2: If there was lock() in the try block and a method that throw Exception between the try block and lock(), it is possible that unlock() won't work. Then AQS(AbstractQueuedSynchronizer) method will be called(depends on the implementation of the class) and IllegalMonitorStateException will be thrown.
Explain 3: It is possible that when implementing the lock() method in Lock object, it would throw unchecked Exception, resulting in the same outcome of the Explain 2.]]>
</entry>
<!-- flowcontrol -->
<entry key="java.flowcontrol.SwitchStatementRule.violation.nodefault">
<![CDATA[missing default statement in switch block]]>
......
......@@ -286,4 +286,44 @@ Positive example 2:
]]>
</example>
</rule>
<rule name="LockShouldWithTryFinallyRule"
language="java"
since="1.6"
message="java.concurrent.LockShouldWithTryFinallyRule.rule.msg"
dfa="true"
class="com.alibaba.p3c.pmd.lang.java.rule.concurrent.LockShouldWithTryFinallyRule">
<description>java.concurrent.LockShouldWithTryFinallyRule.rule.desc</description>
<priority>1</priority>
<example>
<![CDATA[
Positive example:
Lock lock = new XxxLock();
// ...
lock.lock();
try {
doSomething();
doOthers();
} finally {
lock.unlock();
}
]]>
</example>
<example>
<![CDATA[
Negative example:
Lock lock = new XxxLock();
// ...
try {
// If an exception is thrown here, the finally block is executed directly
doSomething();
// The finally block executes regardless of whether the lock is successful or not
lock.lock();
doOthers();
} finally {
lock.unlock();
}
]]>
</example>
</rule>
</ruleset>
......@@ -37,5 +37,6 @@ public class ConcurrentRuleTest extends SimpleAggregatorTst {
addRule(RULE_NAME, "ThreadLocalShouldRemoveRule");
addRule(RULE_NAME, "AvoidConcurrentCompetitionRandomRule");
addRule(RULE_NAME, "CountDownShouldInFinallyRule");
addRule(RULE_NAME, "LockShouldWithTryFinallyRule");
}
}
<?xml version="1.0" encoding="UTF-8"?>
<test-data xmlns="http://pmd.sourceforge.net/rule-tests"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://pmd.sourceforge.net/rule-tests https://pmd.sourceforge.io/rule-tests_1_0_0.xsd">
<test-code>
<description>LockShouldWithTryFinallyRule</description>
<expected-problems>2</expected-problems>
<expected-linenumbers>15,20</expected-linenumbers>
<code><![CDATA[
package com.alibaba.test.p3c;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
/**
* @author caikang
* @date 2019/09/29
*/
public class LockTest {
private Lock lock = new ReentrantLock();
public void testLock() {
try {
lock.lockInterruptibly();
} catch (InterruptedException e) {
e.printStackTrace();
}
try {
lock.lock();
try {
} finally {
}
} catch (Exception e) {
} finally {
lock.unlock();
}
}
}
]]></code>
</test-code>
</test-data>
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册