diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 08285c4368..b341ed6703 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -22,6 +22,11 @@ This is a minor release. #### New Rules +* The new Java rule [`LinguisticNaming`](pmd_rules_java_codestyle.html#linguisticnaming) (`java-codestyle`) + detects cases, when a method name indicates it returns a boolean (such as `isSmall()`) but it doesn't. + Besides method names, the rule also checks field and variable names. It also checks, that getters return + something but setters won't. The rule has several properties with which it can be customized. + * The new PL/SQL rule [`ForLoopNaming`](pmd_rules_plsql_codestyle.html#forloopnaming) (`plsql-codestyle`) enforces a naming convention for "for loops". Both "cursor for loops" and "index for loops" are covered. The rule can be customized via patterns. By default, short variable names are reported. @@ -48,6 +53,7 @@ This is a minor release. ### External Contributions +* [#109](https://github.com/pmd/pmd/pull/109): \[java] Add two linguistics rules under naming - [Arda Aslan](https://github.com/ardaasln) * [#1254](https://github.com/pmd/pmd/pull/1254): \[ci] \[GSoC] Integrating the danger and pmdtester to travis CI - [BBG](https://github.com/djydewang) * [#1258](https://github.com/pmd/pmd/pull/1258): \[java] Use typeof in MissingSerialVersionUID - [krichter722](https://github.com/krichter722) * [#1264](https://github.com/pmd/pmd/pull/1264): \[cpp] Fix NullPointerException in CPPTokenizer:99 - [Rafael Cortês](https://github.com/mrfyda) diff --git a/pmd-core/src/main/resources/rulesets/releases/670.xml b/pmd-core/src/main/resources/rulesets/releases/670.xml index 650eb5c783..6d478e135e 100644 --- a/pmd-core/src/main/resources/rulesets/releases/670.xml +++ b/pmd-core/src/main/resources/rulesets/releases/670.xml @@ -9,5 +9,7 @@ This ruleset contains links to rules that are new in PMD v6.7.0 + + diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java index 60fdd00c24..21d9b3d8ea 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/ast/ASTVariableDeclarator.java @@ -21,4 +21,9 @@ public class ASTVariableDeclarator extends AbstractJavaTypeNode { public Object jjtAccept(JavaParserVisitor visitor, Object data) { return visitor.visit(this, data); } + + public String getName() { + // first child will be VariableDeclaratorId + return jjtGetChild(0).getImage(); + } } diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java new file mode 100644 index 0000000000..7ab22598c0 --- /dev/null +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/codestyle/LinguisticNamingRule.java @@ -0,0 +1,186 @@ +/** + * BSD-style license; for more info see http://pmd.sourceforge.net/license.html + */ + +package net.sourceforge.pmd.lang.java.rule.codestyle; + +import java.util.List; + +import net.sourceforge.pmd.lang.java.ast.ASTFieldDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; +import net.sourceforge.pmd.lang.java.ast.ASTResultType; +import net.sourceforge.pmd.lang.java.ast.ASTType; +import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; +import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; +import net.sourceforge.pmd.properties.BooleanProperty; +import net.sourceforge.pmd.properties.StringMultiProperty; + +public class LinguisticNamingRule extends AbstractJavaRule { + private static final BooleanProperty CHECK_BOOLEAN_METHODS = BooleanProperty.named("checkBooleanMethod") + .defaultValue(true).desc("Check method names and types for inconsistent naming").uiOrder(1.0f).build(); + private static final BooleanProperty CHECK_GETTERS = BooleanProperty.named("checkGetters").defaultValue(true) + .desc("Check return type of getters").uiOrder(2.0f).build(); + private static final BooleanProperty CHECK_SETTERS = BooleanProperty.named("checkSetters").defaultValue(true) + .desc("Check return type of setters").uiOrder(3.0f).build(); + private static final BooleanProperty CHECK_PREFIXED_TRANSFORM_METHODS = BooleanProperty + .named("checkPrefixedTransformMethods") + .defaultValue(true).desc("Check return type of methods whose names start with 'to'").uiOrder(4.0f).build(); + private static final BooleanProperty CHECK_TRANSFORM_METHODS = BooleanProperty.named("checkTransformMethods") + .defaultValue(false).desc("Check return type of methods which contain 'To' in their name").uiOrder(4.0f).build(); + private static final StringMultiProperty BOOLEAN_METHOD_PREFIXES_PROPERTY = StringMultiProperty + .named("booleanMethodPrefixes").defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of methods that return boolean").uiOrder(5.0f).build(); + + private static final BooleanProperty CHECK_FIELDS = BooleanProperty.named("checkFields").defaultValue(true) + .desc("Check field names and types for inconsistent naming").uiOrder(6.0f).build(); + private static final BooleanProperty CHECK_VARIABLES = BooleanProperty.named("checkVariables").defaultValue(true) + .desc("Check local variable names and types for inconsistent naming").uiOrder(7.0f).build(); + private static final StringMultiProperty BOOLEAN_FIELD_PREFIXES_PROPERTY = StringMultiProperty + .named("booleanFieldPrefixes").defaultValues("is", "has", "can", "have", "will", "should") + .desc("the prefixes of fields and variables that indicate boolean").uiOrder(8.0f).build(); + + public LinguisticNamingRule() { + definePropertyDescriptor(CHECK_BOOLEAN_METHODS); + definePropertyDescriptor(CHECK_GETTERS); + definePropertyDescriptor(CHECK_SETTERS); + definePropertyDescriptor(CHECK_PREFIXED_TRANSFORM_METHODS); + definePropertyDescriptor(CHECK_TRANSFORM_METHODS); + definePropertyDescriptor(BOOLEAN_METHOD_PREFIXES_PROPERTY); + definePropertyDescriptor(CHECK_FIELDS); + definePropertyDescriptor(CHECK_VARIABLES); + definePropertyDescriptor(BOOLEAN_FIELD_PREFIXES_PROPERTY); + addRuleChainVisit(ASTMethodDeclaration.class); + addRuleChainVisit(ASTFieldDeclaration.class); + addRuleChainVisit(ASTLocalVariableDeclaration.class); + } + + @Override + public Object visit(ASTMethodDeclaration node, Object data) { + String nameOfMethod = node.getMethodName(); + + if (getProperty(CHECK_BOOLEAN_METHODS)) { + checkBooleanMethods(node, data, nameOfMethod); + } + + if (getProperty(CHECK_SETTERS)) { + checkSetters(node, data, nameOfMethod); + } + + if (getProperty(CHECK_GETTERS)) { + checkGetters(node, data, nameOfMethod); + } + + if (getProperty(CHECK_PREFIXED_TRANSFORM_METHODS)) { + checkPrefixedTransformMethods(node, data, nameOfMethod); + } + + if (getProperty(CHECK_TRANSFORM_METHODS)) { + checkTransformMethods(node, data, nameOfMethod); + } + + return data; + } + + private void checkPrefixedTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (resultType.isVoid() && hasPrefix(nameOfMethod, "to")) { + // To as prefix + addViolationWithMessage(data, node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); + } + } + + private void checkTransformMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (resultType.isVoid() && containsWord(nameOfMethod, "To")) { + // To in the middle somewhere + addViolationWithMessage(data, node, "Linguistics Antipattern - The transform method ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); + } + } + + private void checkGetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "get") && resultType.isVoid()) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The getter ''{0}'' should not return void linguistically", + new Object[] { nameOfMethod }); + } + } + + private void checkSetters(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + if (hasPrefix(nameOfMethod, "set") && !resultType.isVoid()) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The setter ''{0}'' should not return any type except void linguistically", + new Object[] { nameOfMethod }); + } + } + + private void checkBooleanMethods(ASTMethodDeclaration node, Object data, String nameOfMethod) { + ASTResultType resultType = node.getResultType(); + ASTType t = node.getResultType().getFirstChildOfType(ASTType.class); + if (!resultType.isVoid() && t != null) { + for (String prefix : getProperty(BOOLEAN_METHOD_PREFIXES_PROPERTY)) { + if (hasPrefix(nameOfMethod, prefix) && !"boolean".equalsIgnoreCase(t.getTypeImage())) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The method ''{0}'' indicates linguistically it returns a boolean, but it returns ''{1}''", + new Object[] { nameOfMethod, t.getTypeImage() }); + } + } + } + } + + private void checkField(String typeImage, ASTVariableDeclarator node, Object data) { + for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equalsIgnoreCase(typeImage)) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The field ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", + new Object[] { node.getName(), typeImage }); + } + } + } + + private void checkVariable(String typeImage, ASTVariableDeclarator node, Object data) { + for (String prefix : getProperty(BOOLEAN_FIELD_PREFIXES_PROPERTY)) { + if (hasPrefix(node.getName(), prefix) && !"boolean".equalsIgnoreCase(typeImage)) { + addViolationWithMessage(data, node, "Linguistics Antipattern - The variable ''{0}'' indicates linguistically it is a boolean, but it is ''{1}''", + new Object[] { node.getName(), typeImage }); + } + } + } + + @Override + public Object visit(ASTFieldDeclaration node, Object data) { + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null && getProperty(CHECK_FIELDS)) { + List fields = node.findChildrenOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator field : fields) { + checkField(type.getTypeImage(), field, data); + } + } + return data; + } + + @Override + public Object visit(ASTLocalVariableDeclaration node, Object data) { + ASTType type = node.getFirstChildOfType(ASTType.class); + if (type != null && getProperty(CHECK_VARIABLES)) { + List variables = node.findChildrenOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator variable : variables) { + checkVariable(type.getTypeImage(), variable, data); + } + } + return data; + } + + private static boolean hasPrefix(String name, String prefix) { + return name.startsWith(prefix) && name.length() > prefix.length() + && Character.isUpperCase(name.charAt(prefix.length())); + } + + private static boolean containsWord(String name, String word) { + int index = name.indexOf(word); + if (index >= 0 && name.length() > index + word.length()) { + return Character.isUpperCase(name.charAt(index + word.length())); + } + return false; + } +} diff --git a/pmd-java/src/main/resources/category/java/codestyle.xml b/pmd-java/src/main/resources/category/java/codestyle.xml index 27f03db591..a6e9b4c955 100644 --- a/pmd-java/src/main/resources/category/java/codestyle.xml +++ b/pmd-java/src/main/resources/category/java/codestyle.xml @@ -938,6 +938,69 @@ if (foo) { // preferred approach + + + This rule finds Linguistic Naming Antipatterns. It checks for fields, that are named, as if they should + be boolean but have a different type. It also checks for methods, that according to their name, should + return a boolean, but don't. Further, it checks, that getters return something and setters won't. + Finally, it checks that methods, that start with "to" - so called transform methods - actually return + something, since according to their name, they should convert or transform one object into another. + There is additionally an option, to check for methods that contain "To" in their name - which are + also transform methods. However, this is disabled by default, since this detection is prone to + false positives. + + For more information, see [Linguistic Antipatterns - What They Are and How +Developers Perceive Them](https://doi.org/10.1007/s10664-014-9350-8). + + 3 + + + + + + + + + + Method Prefix is + 1 + 6 + + Linguistics Antipattern - The method 'isValid' indicates linguistically it returns a boolean, but it returns 'int' + + + + + + Method Prefix Has + 1 + 6 + + Linguistics Antipattern - The method 'hasChild' indicates linguistically it returns a boolean, but it returns 'int' + + + + + + Method Prefix Have + 1 + 6 + + Linguistics Antipattern - The method 'haveChild' indicates linguistically it returns a boolean, but it returns 'int' + + + + + + Method Prefix can + 1 + 6 + + Linguistics Antipattern - The method 'canFly' indicates linguistically it returns a boolean, but it returns 'int' + + + + + + Method Prefix will + 1 + 6 + + Linguistics Antipattern - The method 'willFly' indicates linguistically it returns a boolean, but it returns 'int' + + + + + + Method Prefix should + 1 + 6 + + Linguistics Antipattern - The method 'shouldFly' indicates linguistically it returns a boolean, but it returns 'long' + + + + + + Method Setters + 1 + 6 + + Linguistics Antipattern - The setter 'setName' should not return any type except void linguistically + + + + + + Method Getters + 1 + 6 + + Linguistics Antipattern - The getter 'getName' should not return void linguistically + + + + + + Method Prefix to: Transform Method + 1 + 6 + + Linguistics Antipattern - The transform method 'toDataType' should not return void linguistically + + + + + + Method Contains To: Transformation methods + true + 1 + 2 + + Linguistics Antipattern - The transform method 'grapeToWine' should not return void linguistically + + + + + + Field/Variable Prefix is + 2 + 3,8 + + Linguistics Antipattern - The field 'isValid' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'isValidLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Field/Variable Prefix has + 2 + 3,8 + + Linguistics Antipattern - The field 'hasMoney' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'hasMoneyLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Field/Variable Prefix can + 2 + 3,8 + + Linguistics Antipattern - The field 'canFly' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'canFlyLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Field/Variable Prefix will + 2 + 3,8 + + Linguistics Antipattern - The field 'willMove' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'willMoveLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Field/Variable Prefix have + 2 + 3,8 + + Linguistics Antipattern - The field 'haveLegs' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'haveLegsLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Field/Variable Prefix should + 2 + 3,8 + + Linguistics Antipattern - The field 'shouldClimb' indicates linguistically it is a boolean, but it is 'int' + Linguistics Antipattern - The variable 'shouldClimbLocal' indicates linguistically it is a boolean, but it is 'int' + + + + + + Multiple fields/local variables + 4 + 2,2,4,4 + + + + + Boolean fields/methods false positive + 0 + + +