Skip to content

Commit

Permalink
Issue #3426: remove warning on PACKAGE_DEF preceded by javadoc not se…
Browse files Browse the repository at this point in the history 
…parated by line
  • Loading branch information
@kazachka
kazachka committed Dec 9, 2016
1 parent 548ecd7 commit b0eaddd
Show file tree
Hide file tree
Showing 8 changed files with 222 additions and 30 deletions.
109 changes: 97 additions & 12 deletions src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java
View file
Expand Up @@ -27,6 +27,7 @@
import com.puppycrawl.tools.checkstyle.api.FileContents;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.utils.CommonUtils;
import com.puppycrawl.tools.checkstyle.utils.JavadocUtils;

/**
* Checks for empty line separators after header, package, all import declarations,
Expand Down Expand Up @@ -250,6 +251,11 @@ public void setAllowMultipleEmptyLinesInsideClassMembers(boolean allow) {
allowMultipleEmptyLinesInsideClassMembers = allow;
}

@Override
public boolean isCommentNodesRequired() {
return true;
}

@Override
public int[] getDefaultTokens() {
return getAcceptableTokens();
Expand Down Expand Up @@ -285,7 +291,10 @@ public void visitToken(DetailAST ast) {
processMultipleLinesInside(ast);
}

final DetailAST nextToken = ast.getNextSibling();
DetailAST nextToken = ast.getNextSibling();
while (nextToken != null && isTrailingComment(nextToken)) {
nextToken = nextToken.getNextSibling();
}
if (nextToken != null) {
final int astType = ast.getType();
switch (astType) {
Expand All @@ -299,15 +308,7 @@ public void visitToken(DetailAST ast) {
processPackage(ast, nextToken);
break;
default:
if (nextToken.getType() == TokenTypes.RCURLY) {
if (hasNotAllowedTwoEmptyLinesBefore(nextToken)) {
log(ast.getLineNo(), MSG_MULTIPLE_LINES_AFTER, ast.getText());
}
}
else if (!hasEmptyLineAfter(ast)) {
log(nextToken.getLineNo(), MSG_SHOULD_BE_SEPARATED,
nextToken.getText());
}
processToken(ast, nextToken);
}
}
}
Expand Down Expand Up @@ -405,7 +406,14 @@ && hasNotAllowedTwoEmptyLinesBefore(ast)) {
*/
private void processPackage(DetailAST ast, DetailAST nextToken) {
if (ast.getLineNo() > 1 && !hasEmptyLineBefore(ast)) {
log(ast.getLineNo(), MSG_SHOULD_BE_SEPARATED, ast.getText());
if (getFileContents().getFileName().endsWith("package-info.java")) {
if (ast.getFirstChild().getChildCount() == 0 && !isPrecededByJavadoc(ast)) {
log(ast.getLineNo(), MSG_SHOULD_BE_SEPARATED, ast.getText());
}
}
else {
log(ast.getLineNo(), MSG_SHOULD_BE_SEPARATED, ast.getText());
}
}
if (!hasEmptyLineAfter(ast)) {
log(nextToken.getLineNo(), MSG_SHOULD_BE_SEPARATED, nextToken.getText());
Expand All @@ -431,12 +439,30 @@ private void processImport(DetailAST ast, DetailAST nextToken, int astType) {
*/
private void processVariableDef(DetailAST ast, DetailAST nextToken) {
if (isTypeField(ast) && !hasEmptyLineAfter(ast)
&& !isFollowedBySingleLineComment(ast)
&& isViolatingEmptyLineBetweenFieldsPolicy(nextToken)) {
log(nextToken.getLineNo(), MSG_SHOULD_BE_SEPARATED,
nextToken.getText());
}
}

/**
* Process tokens, that aren't package, import or variable.
* @param ast token to process
* @param nextToken next Token
*/
private void processToken(DetailAST ast, DetailAST nextToken) {
if (nextToken.getType() == TokenTypes.RCURLY) {
if (hasNotAllowedTwoEmptyLinesBefore(nextToken)) {
log(ast.getLineNo(), MSG_MULTIPLE_LINES_AFTER, ast.getText());
}
}
else if (!hasEmptyLineAfter(ast) && !isFollowedBySingleLineComment(ast)) {
log(nextToken.getLineNo(), MSG_SHOULD_BE_SEPARATED,
nextToken.getText());
}
}

/**
* Checks whether token placement violates policy of empty line between fields.
* @param detailAST token to be analyzed
Expand Down Expand Up @@ -487,8 +513,12 @@ private boolean hasEmptyLineAfter(DetailAST token) {
if (lastToken == null) {
lastToken = token.getLastChild();
}
DetailAST nextToken = token.getNextSibling();
if (isTrailingComment(nextToken)) {
nextToken = nextToken.getNextSibling();
}
// Start of the next token
final int nextBegin = token.getNextSibling().getLineNo();
final int nextBegin = nextToken.getLineNo();
// End of current token.
final int currentEnd = lastToken.getLineNo();
return hasEmptyLine(currentEnd + 1, nextBegin - 1);
Expand Down Expand Up @@ -533,6 +563,61 @@ private boolean hasEmptyLineBefore(DetailAST token) {
return lineBefore.trim().isEmpty();
}

/**
* Check if token is preceded by javadoc comment.
* @param token token for check.
* @return true, if token is preceded by javadoc comment.
*/
private static boolean isPrecededByJavadoc(DetailAST token) {
boolean result = false;
final DetailAST previous = token.getPreviousSibling();
if (previous.getType() == TokenTypes.BLOCK_COMMENT_BEGIN
&& JavadocUtils.isJavadocComment(previous)) {
result = true;
}
return result;
}

/**
* Check if token is a trailing comment.
* @param comment ast node that represents comment.
* @return true, if given ast is trailing comment.
*/
private static boolean isTrailingComment(DetailAST comment) {
final int commentType = comment.getType();
DetailAST previousToken = comment.getPreviousSibling();
while (previousToken.getLastChild() != null) {
previousToken = previousToken.getLastChild();
}
boolean result = false;
if (commentType == TokenTypes.SINGLE_LINE_COMMENT) {
result = comment.getLineNo() == previousToken.getLineNo();
}
else if (commentType == TokenTypes.BLOCK_COMMENT_BEGIN) {
//check if comment is not multiline and on same line that previous sibling
result = comment.getLineNo() == comment.getLastChild().getLineNo()
&& comment.getLineNo() == previousToken.getLineNo();
}
return result;
}

/**
* Check if token is followed by single line comment.
* @param ast token for check.
* @return true, if given ast node is followed by single line comment.
*/
private static boolean isFollowedBySingleLineComment(DetailAST ast) {
DetailAST nextToken = ast.getNextSibling();
while (isTrailingComment(nextToken)) {
nextToken = nextToken.getNextSibling();
}
boolean result = false;
if (ast.getNextSibling().getType() == TokenTypes.SINGLE_LINE_COMMENT) {
result = true;
}
return result;
}

/**
* If variable definition is a type field.
* @param variableDef variable definition.
Expand Down
89 changes: 74 additions & 15 deletions ...t/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheckTest.java
View file
Expand Up @@ -55,14 +55,18 @@ public void testDefault() throws Exception {

final String[] expected = {
"21: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "import"),
"35: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CLASS_DEF"),
"38: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "VARIABLE_DEF"),
"39: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "STATIC_INIT"),
"43: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INSTANCE_INIT"),
"57: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CTOR_DEF"),
"62: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"79: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"110: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INTERFACE_DEF"),
"28: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "//"),
"30: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
"34: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
"38: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CLASS_DEF"),
"41: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "VARIABLE_DEF"),
"42: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "STATIC_INIT"),
"46: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INSTANCE_INIT"),
"60: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CTOR_DEF"),
"65: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"82: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"113: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INTERFACE_DEF"),
"157: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
};
verify(checkConfig, getPath("InputEmptyLineSeparator.java"), expected);
}
Expand All @@ -75,13 +79,17 @@ public void testAllowNoEmptyLineBetweenFields() throws Exception {

final String[] expected = {
"21: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "import"),
"35: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CLASS_DEF"),
"39: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "STATIC_INIT"),
"43: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INSTANCE_INIT"),
"57: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CTOR_DEF"),
"62: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"79: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"110: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INTERFACE_DEF"),
"28: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "//"),
"30: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
"34: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
"38: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CLASS_DEF"),
"42: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "STATIC_INIT"),
"46: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INSTANCE_INIT"),
"60: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "CTOR_DEF"),
"65: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"82: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "METHOD_DEF"),
"113: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "INTERFACE_DEF"),
"157: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "/*"),
};
verify(checkConfig, getPath("InputEmptyLineSeparator.java"), expected);
}
Expand Down Expand Up @@ -191,4 +199,55 @@ public void testAllowMultipleEmptyLinesInsideClassMembers() throws Exception {
getPath("InputEmptyLineSeparatorMultipleEmptyLinesInside.java"),
expected);
}

@Test
public void testDisAllowMultilineCommentBeforePackageDefinition() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(EmptyLineSeparatorCheck.class);
final String[] expected = {
"5: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "package"),
};
verify(checkConfig,
getPath("InputEmptyLineSeparatorMultilineCommentHeader.java"),
expected);
}

@Test
public void testAllowPackageAnnotation() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(EmptyLineSeparatorCheck.class);
final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
verify(checkConfig,
getPath("package-info/test1/package-info.java"),
expected);
}

@Test
public void testAllowJavadocBeforePackage() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(EmptyLineSeparatorCheck.class);
final String[] expected = CommonUtils.EMPTY_STRING_ARRAY;
verify(checkConfig,
getPath("package-info/test2/package-info.java"),
expected);
}

@Test
public void testDisAllowBlockCommentBeforePackage() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(EmptyLineSeparatorCheck.class);
final String[] expected = {
"3: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "package"),
};
verify(checkConfig,
getPath("package-info/test3/package-info.java"),
expected);
}

@Test
public void testAllowSingleLineCommentPackage() throws Exception {
final DefaultConfiguration checkConfig = createCheckConfig(EmptyLineSeparatorCheck.class);
final String[] expected = {
"3: " + getCheckMessage(MSG_SHOULD_BE_SEPARATED, "package"),
};
verify(checkConfig,
getPath("package-info/test4/package-info.java"),
expected);
}
}
20 changes: 17 additions & 3 deletions .../resources/com/puppycrawl/tools/checkstyle/checks/whitespace/InputEmptyLineSeparator.java
View file
Expand Up @@ -17,18 +17,21 @@
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.puppycrawl.tools.checkstyle.checks.whitespace;
package com.puppycrawl.tools.checkstyle.checks.whitespace; //no violation: trailing comment
import java.io.Serializable;
import java.util.ArrayList;
import java.util.ArrayList; /*no violation: trailing comment*/
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.Collections;
//violation: not separated by empty line and not trailing comment

import com.oracle.net.Sdp;
import com.oracle.net.Sdp; /*violation: not separated by empty line
and not trailing comment*/

import javax.swing.AbstractAction;
/*violation: not separated by empty line and not trailing comment*/

import org.apache.commons.beanutils.locale.converters.ByteLocaleConverter;
import org.apache.commons.beanutils.BasicDynaBean;
Expand Down Expand Up @@ -124,7 +127,17 @@ private InnerClass()
{
//empty
}
//no violation: single line comment after method
}


class SecondInnerClass {

private int intVariable; //to check isFollowedBySingleLineComment
} //violation: field definition should be separated from

private int intVariable;
//no violation: single line comment after method
}

class Class2{
Expand All @@ -141,4 +154,5 @@ public int compareTo(InputEmptyLineSeparator aObject) //ok
return 0;
}
};
/*violation: block comment after method before RCURLY*/
}
9 changes: 9 additions & 0 deletions ...awl/tools/checkstyle/checks/whitespace/InputEmptyLineSeparatorMultilineCommentHeader.java
View file
@@ -0,0 +1,9 @@
/*violation: to check warning
that causes by miltiline comment
before package definition.
*/
package com.puppycrawl.tools.checkstyle.checks.whitespace;

public class InputEmptyLineSeparatorMultilineCommentHeader
{
}
5 changes: 5 additions & 0 deletions ...es/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test1/package-info.java
View file
@@ -0,0 +1,5 @@
/*OK: for test allowing to place annotation before PACKAGE_DEF.*/
@Deprecated
package com.puppycrawl.tools.checkstyle.checks.whitespace.packageinfo.test1;

import java.lang.Deprecated;
6 changes: 6 additions & 0 deletions ...es/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test2/package-info.java
View file
@@ -0,0 +1,6 @@
/**OK: for test allowing to place javadoc before PACKAGE_DEF.*/
package com.puppycrawl.tools.checkstyle.checks.whitespace.packageinfo.test2;

//that import is for doing check of PACKAGE_DEF,
//because EmptyLineSeparatorCheck doesn't check last token
import java.lang.System;
7 changes: 7 additions & 0 deletions ...es/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test3/package-info.java
View file
@@ -0,0 +1,7 @@
/*violation: for test that there's warning when block comment isn't
separated from PACKAGE_DEF by line.*/
package com.puppycrawl.tools.checkstyle.checks.whitespace.packageinfo.test3;

//that import is for doing check of PACKAGE_DEF,
//because EmptyLineSeparatorCheck doesn't check last token
import java.lang.System;
7 changes: 7 additions & 0 deletions ...es/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test4/package-info.java
View file
@@ -0,0 +1,7 @@
//for test there's warning when single line comment isn't separated from PACKAGE_DEF by line
//violation is expected after this line
package com.puppycrawl.tools.checkstyle.checks.whitespace.packageinfo.test4;

//that import is for doing check of PACKAGE_DEF,
//because EmptyLineSeparatorCheck doesn't check last token
import java.lang.System;

0 comments on commit b0eaddd

Please sign in to comment.