Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #461: Field single declaration check implemented #811

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ EmptyPublicCtorInClassCheck.desc = <p>This Check looks for useless empty public
EmptyPublicCtorInClassCheck.classAnnotationNames = Regex which matches names of class annotations which require class to have public no-argument ctor. Default value is "javax\\.persistence\\.Entity".
EmptyPublicCtorInClassCheck.ctorAnnotationNames = Regex which matches names of ctor annotations which make empty public ctor essential. Default value is "com\\.google\\.inject\\.Inject".

FieldSingleDeclarationCheck.name = Field Single Declaration Check
FieldSingleDeclarationCheck.desc = <p>This checks ensures that classes have at most 1 field of the given className. This can be useful for example to ensure that only one Logger is used.</p>
FieldSingleDeclarationCheck.fullyQualifiedClassName = Fully qualified name of class of which there should only be max. 1 field per class. Example: "org.slf4j.Logger".

FinalizeImplementationCheck.name = Finalize Implementation
FinalizeImplementationCheck.desc = <p>This Check detects 3 most common cases of incorrect finalize() method implementation:</p><ul><li>negates effect of superclass finalize<br/><code>protected void finalize() { } <br/> protected void finalize() { doSomething(); }</code></li><li>useless (or worse) finalize<br/><code>protected void finalize() { super.finalize(); }</code></li><li>public finalize<br/><code>public void finalize() { try { doSomething(); } finally { super.finalize() } }</code></li></ul>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,17 @@
<message-key key="empty.public.ctor"/>
</rule-metadata>

<rule-metadata name="%FieldSingleDeclarationCheck.name" internal-name="FieldSingleDeclarationCheck" parent="TreeWalker">
<alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck"/>
<description>%FieldSingleDeclarationCheck.desc</description>

<property-metadata name="fullyQualifiedClassName" datatype="String" default-value="java.util.logging.Logger">
<description>%FieldSingleDeclarationCheck.fullyQualifiedClassName</description>
</property-metadata>

<message-key key="field.count"/>
</rule-metadata>

<rule-metadata name="%ForbidCertainImportsCheck.name" internal-name="ForbidCertainImportsCheck" parent="TreeWalker">
<alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.ForbidCertainImportsCheck"/>
<description>%ForbidCertainImportsCheck.desc</description>
Expand Down
3 changes: 3 additions & 0 deletions sevntu-checks/sevntu-checks.xml
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@
<property name="ignoreFieldNamePattern" value="serialVersionUID"/>
</module>
<module name="com.github.sevntu.checkstyle.checks.coding.EmptyPublicCtorInClassCheck"/>
<module name="com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck">
<property name="fullyQualifiedClassName" value="java.util.logging.Logger"/>
</module>
<module name="com.github.sevntu.checkstyle.checks.coding.DiamondOperatorForVariableDefinitionCheck"/>
<module name="com.github.sevntu.checkstyle.checks.coding.NameConventionForJunit4TestClassesCheck"/>
<module name="com.github.sevntu.checkstyle.checks.coding.UselessSuperCtorCallCheck"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@

import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.FullIdent;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.CheckUtil;
import com.puppycrawl.tools.checkstyle.utils.TokenUtil;

/**
Expand Down Expand Up @@ -76,4 +79,40 @@ public static DetailAST getNextSubTreeNode(DetailAST node, DetailAST subTreeRoot
return toVisitAst;
}

/**
* Obtains full type name of first token in node.
* @param ast an AST node
* @return fully qualified name (FQN) of type, or null if none was found
*/
public static String getTypeNameOfFirstToken(DetailAST ast) {
final DetailAST findFirstToken = ast.findFirstToken(TokenTypes.TYPE);
final FullIdent ident = CheckUtil.createFullType(findFirstToken);
Comment on lines +88 to +89
Copy link
Member Author

@yaziza yaziza Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should test findFirstToken and ident against NPE. A variable definition should always have a TYPE and IDENT right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression would should if an exception is possible.


return ident.getText();
}

/**
* Checks node for matching class, taken both FQN and short name into account.
*
* @param ast
* an AST node
* @param fqClassName
* fully qualified class name
* @return true if type name of first token in node is fqnClassName, or its short name; false
* otherwise
*/
public static boolean matchesFullyQualifiedName(DetailAST ast, String fqClassName) {
final String typeName = getTypeNameOfFirstToken(ast);
final int lastDotPosition = fqClassName.lastIndexOf('.');
boolean isMatched = false;
if (lastDotPosition == -1) {
isMatched = typeName.equals(fqClassName);
}
else {
final String shortClassName = fqClassName.substring(lastDotPosition + 1);
isMatched = typeName.equals(fqClassName) || typeName.equals(shortClassName);
}
return isMatched;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2020 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library 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 GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.github.sevntu.checkstyle.checks.coding;

import com.github.sevntu.checkstyle.SevntuUtil;
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

/**
* <p>This checks ensures that classes have at most 1 field of the given className.</p>
*
* <p>This can be useful for example to ensure that only one Logger is used:</p>
*
* <pre>
* &lt;module name="TreeWalker"&gt;
* &lt;module name="com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck"&gt;
* &lt;property name="fullyQualifiedClassName" value="org.slf4j.Logger" /&gt;
* &lt;/module&gt;
* &lt;/module&gt;
*
* class Example {
* private static final org.slf4j.Logger logger = LoggerFactory.getLogger(Example.class); // OK
* private static org.slf4j.Logger logger2; // NOK!
* }
* </pre>
*
* @author Milos Fabian, Pantheon Technologies - original author (in OpenDaylight.org)
* @author Michael Vorburger.ch - refactored and made more generalized for contribution to Sevntu
* @author <a href="mailto:[email protected]">Yasser Aziza</a> - completed sevntu integration
*/
public class FieldSingleDeclarationCheck extends AbstractCheck {

/**
* Violation message key.
*/
public static final String MSG_KEY = "field.count";

/**
* Configuration property with class name check for.
*/
private String fullyQualifiedClassName = "java.util.logging.Logger";

/**
* Field to remember if class was previously seen in current File.
*/
private boolean hasPreviouslySeenClass;

/**
* Set Class of which there should only be max. 1 field per class.
* @param className the fully qualified name (FQN) of the class
*/
public void setFullyQualifiedClassName(String className) {
this.fullyQualifiedClassName = className;
}

@Override
public int[] getDefaultTokens() {
return new int[] {TokenTypes.VARIABLE_DEF };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this being enough. You are not checking imports so your implementation is very susceptible to name conflicts.

}

@Override
public int[] getRequiredTokens() {
return getDefaultTokens();
}

@Override
public int[] getAcceptableTokens() {
return getDefaultTokens();
}

@Override
public void beginTree(DetailAST rootAST) {
this.hasPreviouslySeenClass = false;
}

@Override
public void visitToken(DetailAST ast) {
if (SevntuUtil.matchesFullyQualifiedName(ast, fullyQualifiedClassName)) {
if (hasPreviouslySeenClass) {
log(ast.getLineNo(), MSG_KEY, fullyQualifiedClassName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give it the full AST.

}
this.hasPreviouslySeenClass = true;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ custom.declaration.order.method=Method definition in wrong order. Expected ''{0}
diamond.operator.for.variable.definition=Diamond operator expected.
either.log.or.throw=Either log or throw exception.
empty.public.ctor=This empty public constructor is useless.
field.count={0} should be declared only once.
finalize.implementation.missed.super.finalize=You have to call super.finalize() right after finally opening brace.
finalize.implementation.missed.try.finally=finalize() method should contain try-finally block with super.finalize() call inside finally block.
finalize.implementation.public=finalize() method should have a "protected" visibility.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2020 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library 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 GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.github.sevntu.checkstyle.checks.coding;

import org.junit.Test;

import com.puppycrawl.tools.checkstyle.AbstractModuleTestSupport;
import com.puppycrawl.tools.checkstyle.DefaultConfiguration;

/**
* LoggerDeclarationsCountCheck test.
*
* @author <a href="mailto:[email protected]">Michael Vorburger</a> - completed sevntu integration
*/
public class FieldSingleDeclarationCheckTest extends AbstractModuleTestSupport {

@Override
protected String getPackageLocation() {
return "com/github/sevntu/checkstyle/checks/coding";
}

@Test
public void testFieldSingleDeclarationOk() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

checkConfig.addAttribute("fullyQualifiedClassName", "org.slf4j.Logger");

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
}

@Test
public void testFieldSingleDeclarationNok() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

checkConfig.addAttribute("fullyQualifiedClassName", "org.slf4j.Logger");

final String[] expected = {
"10: " + getCheckMessage(FieldSingleDeclarationCheck.MSG_KEY, "org.slf4j.Logger"),
};

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckNok.java"), expected);
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
}

@Test
public void testFieldSingleDeclarationFqnNok() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

checkConfig.addAttribute("fullyQualifiedClassName", "org.slf4j.Logger");

final String[] expected = {
"7: " + getCheckMessage(FieldSingleDeclarationCheck.MSG_KEY, "org.slf4j.Logger"),
};

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckFqnNok.java"), expected);
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
}

@Test
public void testNoClassNameConfigured() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckNok.java"), new String[] {});
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckFqnNok.java"),
new String[] {});
}

@Test
public void testCustomTokens() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

checkConfig.addAttribute("fullyQualifiedClassName", "org.slf4j.Logger");
// This is required just so that getAcceptableTokens() gets coverage
checkConfig.addAttribute("tokens", "VARIABLE_DEF");

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
}

@Test
public void testUnknownClassName() throws Exception {
final DefaultConfiguration checkConfig =
createModuleConfig(FieldSingleDeclarationCheck.class);

checkConfig.addAttribute("fullyQualifiedClassName", "SomeClass");
// This is required just so that getAcceptableTokens() gets coverage
checkConfig.addAttribute("tokens", "VARIABLE_DEF");

verify(checkConfig, getPath("InputFieldSingleDeclarationCheckOk.java"), new String[] {});
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckNok.java"), new String[] {});
verify(checkConfig, getPath("InputFieldSingleDeclarationCheckFqnNok.java"),
new String[] {});
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputFieldSingleDeclarationCheckFqnNok {

// even twice a FQN type names work:
org.slf4j.Logger logger1;
org.slf4j.Logger logger2; // <= Checkstyle violation raised here!

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.github.sevntu.checkstyle.checks.coding;

import java.io.File;
import java.util.logging.Logger;

public class InputFieldSingleDeclarationCheckNok {

// both FQN and import'ed type names work:
Logger logger1;
org.slf4j.Logger logger2; // <= Checkstyle violation raised here!

// some field of another type (req. for test coverage)
File file;

// a method, so that there are not only fields here (req. for test coverage)
void foo() { }

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputFieldSingleDeclarationCheckOk {

org.slf4j.Logger logger;

}
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,19 @@
</param>
</rule>

<rule>
<key>com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck</key>
<name>Field Single Declaration Check</name>
<tag>coding</tag>
<description>This checks ensures that classes have at most 1 field of the given className. This can be useful for example to ensure that only one Logger is used.</description>
<configKey>Checker/TreeWalker/com.github.sevntu.checkstyle.checks.coding.FieldSingleDeclarationCheck</configKey>

<param key="fullyQualifiedClassName" type="STRING">
<defaultValue>java.util.logging.Logger</defaultValue>
<description>Fully qualified name of class of which there should only be max. 1 field per class. Example: "java.util.logging.Logger".</description>
</param>
</rule>

<rule>
<key>com.github.sevntu.checkstyle.checks.coding.FinalizeImplementationCheck</key>
<name>Finalize Implementation Check</name>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void testAllRulesValid() {
Assert.assertEquals("Incorrect repository language", "java", repository.language());

final List<RulesDefinition.Rule> rules = repository.rules();
Assert.assertEquals("Incorrect number of loaded rules", 59, rules.size());
Assert.assertEquals("Incorrect number of loaded rules", 60, rules.size());

for (RulesDefinition.Rule rule : rules) {
Assert.assertNotNull("Rule key is not set", rule.key());
Expand Down