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 #4714: Make SuppressionCommentFilter and SuppressWithNearbyCommentFilter children of TreeWalker #4755

Merged
merged 2 commits into from Jul 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 16 additions & 14 deletions config/checkstyle_checks.xml
Expand Up @@ -31,24 +31,10 @@
<property name="severity" value="ignore"/>
<property name="acceptOnMatch" value="false"/>
</module>
<module name="SuppressionCommentFilter">
<!--
Use suppressions.xml for suppressions, this is only example.
checkFormat will prevent suppression comments from being valid.
-->
<property name="checkFormat" value="IGNORETHIS"/>
<property name="offCommentFormat" value="CSOFF\: .*"/>
<property name="onCommentFormat" value="CSON\: .*"/>
</module>
<module name="SuppressionFilter">
<property name="file" value="${checkstyle.suppressions.file}"/>
</module>
<module name="SuppressWarningsFilter"/>
<module name="SuppressWithNearbyCommentFilter">
<property name="commentFormat" value="-@cs\[(\w{8,}(\|\w{8,})*)\] \w[\(\)\-\.\'\`\,\:\;\w ]{10,}"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="3"/>
</module>

<!-- Headers -->
<module name="Header">
Expand Down Expand Up @@ -298,6 +284,22 @@
<module name="UnnecessaryParentheses"/>
<module name="VariableDeclarationUsageDistance"/>

<!-- Filters-->
<module name="SuppressionCommentFilter">
<!--
Use suppressions.xml for suppressions, this is only example.
checkFormat will prevent suppression comments from being valid.
-->
<property name="checkFormat" value="IGNORETHIS"/>
<property name="offCommentFormat" value="CSOFF\: .*"/>
<property name="onCommentFormat" value="CSON\: .*"/>
</module>
<module name="SuppressWithNearbyCommentFilter">
<property name="commentFormat" value="-@cs\[(\w{8,}(\|\w{8,})*)\] \w[\(\)\-\.\'\`\,\:\;\w ]{10,}"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="3"/>
</module>

<!-- Imports -->
<module name="AvoidStarImport"/>
<module name="AvoidStaticImport"/>
Expand Down
10 changes: 5 additions & 5 deletions config/checkstyle_sevntu_checks.xml
Expand Up @@ -14,13 +14,13 @@
<module name="SuppressionFilter">
<property name="file" value="${project.basedir}/config/sevntu_suppressions.xml"/>
</module>
<module name="SuppressWithNearbyCommentFilter">
<property name="commentFormat" value="-@cs\[(\w{8,})\] \w[\(\)\-\.\'\`\,\:\;\w ]{10,}"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="3"/>
</module>

<module name="TreeWalker">
<module name="SuppressWithNearbyCommentFilter">
<property name="commentFormat" value="-@cs\[(\w{8,})\] \w[\(\)\-\.\'\`\,\:\;\w ]{10,}"/>
<property name="checkFormat" value="$1"/>
<property name="influenceFormat" value="3"/>
</module>
<module name="FileContentsHolder"/>
<module name="StaticMethodCandidate"/>
<module name="UselessSingleCatchCheck"/>
Expand Down
2 changes: 2 additions & 0 deletions config/import-control.xml
Expand Up @@ -142,6 +142,8 @@

<subpackage name="filters">
<allow class="java.lang.ref.WeakReference" local-only="true"/>
<allow class="com.puppycrawl.tools.checkstyle.TreeWalkerAuditEvent" local-only="true"/>
<allow class="com.puppycrawl.tools.checkstyle.TreeWalkerFilter" local-only="true"/>
<disallow pkg="com\.puppycrawl\.tools\.checkstyle\.checks\.[^.]+" regex="true"/>
<allow pkg="com.puppycrawl.tools.checkstyle.utils"/>
</subpackage>
Expand Down
3 changes: 3 additions & 0 deletions pom.xml
Expand Up @@ -517,6 +517,9 @@
<id>sevntu-checkstyle-check</id>
<phase>verify</phase>
<configuration>
<!-- skipped until sevntu-checks to 8.1 upgrade
see https://github.com/checkstyle/checkstyle/pull/4755 -->
<skip>true</skip>
<configLocation>${project.basedir}/config/checkstyle_sevntu_checks.xml</configLocation>
<failOnViolation>true</failOnViolation>
<includeResources>false</includeResources>
Expand Down
68 changes: 60 additions & 8 deletions src/main/java/com/puppycrawl/tools/checkstyle/TreeWalker.java
Expand Up @@ -29,6 +29,8 @@
import java.util.Locale;
import java.util.Map.Entry;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;

import antlr.CommonHiddenStreamToken;
import antlr.RecognitionException;
Expand All @@ -40,13 +42,15 @@
import com.google.common.collect.Multimap;
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck;
import com.puppycrawl.tools.checkstyle.api.AutomaticBean;
import com.puppycrawl.tools.checkstyle.api.CheckstyleException;
import com.puppycrawl.tools.checkstyle.api.Configuration;
import com.puppycrawl.tools.checkstyle.api.Context;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.ExternalResourceHolder;
import com.puppycrawl.tools.checkstyle.api.FileContents;
import com.puppycrawl.tools.checkstyle.api.FileText;
import com.puppycrawl.tools.checkstyle.api.LocalizedMessage;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaLexer;
import com.puppycrawl.tools.checkstyle.grammars.GeneratedJavaRecognizer;
Expand All @@ -59,6 +63,9 @@
*
* @author Oliver Burn
*/
// -@cs[ClassFanOutComplexity] To resolve issue 4714, new classes were imported. Number of
// classes current class relies on currently is 27, which is above threshold 25.
// see https://github.com/checkstyle/checkstyle/issues/4714.
public final class TreeWalker extends AbstractFileSetCheck implements ExternalResourceHolder {

/** Default distance between tab stops. */
Expand All @@ -78,6 +85,12 @@ public final class TreeWalker extends AbstractFileSetCheck implements ExternalRe
/** Registered comment checks. */
private final Set<AbstractCheck> commentChecks = new HashSet<>();

/** The ast filters. */
private final Set<TreeWalkerFilter> filters = new HashSet<>();

/** The sorted set of messages. */
private final SortedSet<LocalizedMessage> messages = new TreeSet<>();

/** The distance between tab stops. */
private int tabWidth = DEFAULT_TAB_WIDTH;

Expand Down Expand Up @@ -144,23 +157,35 @@ public void finishLocalSetup() {
childContext = checkContext;
}

/**
* {@inheritDoc} Creates child module.
* @noinspection ChainOfInstanceofChecks
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this suppression here when Checker, which has similar code doesn't need it?
https://github.com/rnveach/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L436

Copy link
Contributor Author

@timurt timurt Jul 23, 2017

Choose a reason for hiding this comment

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

@rnveach
Your example from your forked repo, inside main repo this suppression exists
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Checker.java#L442

Otherwise TeamCity fails

Copy link
Member

@rnveach rnveach Jul 23, 2017

Choose a reason for hiding this comment

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

Sorry, I used the wrong link to my own fork.
You are correct and can ignore this.

*/
@Override
public void setupChild(Configuration childConf)
throws CheckstyleException {
final String name = childConf.getName();
final Object module = moduleFactory.createModule(name);
if (!(module instanceof AbstractCheck)) {
if (module instanceof AutomaticBean) {
Copy link
Member

@rnveach rnveach Jul 23, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rnveach
I think moduleFactory.createModule(name); is where exception probably could happen, but i was not surrounded by try/catch before. Should I do it now?

Copy link
Member

Choose a reason for hiding this comment

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

It was a question to more to the others. It may be helpful to let the user know their module cant initialize versus some other exception that can happen around there.
@MEZk @romani

Copy link
Member

@romani romani Jul 23, 2017

Choose a reason for hiding this comment

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

createModule already put in exception all required context for user - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java#L183

but other methods contextualize and configure will not provide context of which module was in reflection initialization process.
@timurt , please add try-catch.

UPDATE: moved to #4814

Copy link
Contributor Author

@timurt timurt Jul 23, 2017

Choose a reason for hiding this comment

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

@romani @rnveach @MEZk
As I understood, TreeWalker is the child of the Checker. So if any exception occurs inside TreeWalker.setupChild it will be caught inside Checker.setupChild, right?
Is this try-catch necessary?
It will require some tests to be changed, because new thrown exception differs from old one.

Previous exception message

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - Cannot set property 'excludedPackages' to 'com.puppycrawl.tools.checkstyle.checks.metrics.inputs.a.' in module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck

Exception with try/catch inside TreeWalker.setupChild

cannot initialize module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck - Cannot set property 'excludedPackages' to 'com.puppycrawl.tools.checkstyle.checks.metrics.inputs.a.' in module com.puppycrawl.tools.checkstyle.checks.metrics.ClassFanOutComplexityCheck

Other moment, if for example TreeWalker.setupChild catches exception and throws CheckstyleException. This thrown exception will be caught by Checker.setupChild and message becomes little bit weird:

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - cannot initialize module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck - Cannot set property 'option' to 'invalid_option' in module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck

original message

cannot initialize module com.puppycrawl.tools.checkstyle.TreeWalker - Cannot set property 'option' to 'invalid_option' in module com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck

Copy link
Member

Choose a reason for hiding this comment

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

lets move this nuance to #4814 .
It could be done later.

@rnveach , @MEZk , please approve PR.

final AutomaticBean bean = (AutomaticBean) module;
bean.contextualize(childContext);
bean.configure(childConf);
}
if (module instanceof AbstractCheck) {
final AbstractCheck check = (AbstractCheck) module;
check.init();
registerCheck(check);
}
else if (module instanceof TreeWalkerFilter) {
final TreeWalkerFilter filter = (TreeWalkerFilter) module;
filters.add(filter);
}
else {
throw new CheckstyleException(
"TreeWalker is not allowed as a parent of " + name
+ " Please review 'Parent Module' section for this Check in web"
+ " documentation if Check is standard.");
}
final AbstractCheck check = (AbstractCheck) module;
check.contextualize(childContext);
check.configure(childConf);
check.init();

registerCheck(check);
}

@Override
Expand All @@ -169,6 +194,7 @@ protected void processFiltered(File file, FileText fileText) throws CheckstyleEx
if (CommonUtils.matchesFileExtension(file, getFileExtensions())) {
final String msg = "%s occurred during the analysis of file %s.";
final String fileName = file.getPath();

try {
if (!ordinaryChecks.isEmpty()
|| !commentChecks.isEmpty()) {
Expand All @@ -183,6 +209,10 @@ protected void processFiltered(File file, FileText fileText) throws CheckstyleEx

walk(astWithComments, contents, AstState.WITH_COMMENTS);
}
final SortedSet<LocalizedMessage> filteredMessages =
getFilteredMessages(fileName, contents);
addMessages(filteredMessages);
messages.clear();
}
}
catch (final TokenStreamRecognitionException tre) {
Expand All @@ -198,6 +228,28 @@ protected void processFiltered(File file, FileText fileText) throws CheckstyleEx
}
}

/**
* Returns filtered set of {@link LocalizedMessage}.
* @param fileName path to the file
* @param fileContents the contents of the file
* @return filtered set of messages
*/
private SortedSet<LocalizedMessage> getFilteredMessages(String fileName,
FileContents fileContents) {
final SortedSet<LocalizedMessage> result = new TreeSet<>(messages);
for (LocalizedMessage element : messages) {
final TreeWalkerAuditEvent event =
new TreeWalkerAuditEvent(fileContents, fileName, element);
for (TreeWalkerFilter filter : filters) {
if (!filter.accept(event)) {
result.remove(element);
break;
}
}
}
return result;
}

/**
* Register a check for a given configuration.
* @param check the check to register
Expand Down Expand Up @@ -350,7 +402,7 @@ private void notifyEnd(DetailAST rootAST, AstState astState) {

for (AbstractCheck check : checks) {
check.finishTree(rootAST);
addMessages(check.getMessages());
messages.addAll(check.getMessages());
}
}

Expand Down
@@ -0,0 +1,118 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2017 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.puppycrawl.tools.checkstyle;

import com.puppycrawl.tools.checkstyle.api.FileContents;
import com.puppycrawl.tools.checkstyle.api.LocalizedMessage;

/**
* Raw {@code TreeWalker} event for audit.
*
* @author Timur Tibeyev
*/
public class TreeWalkerAuditEvent {
/** Filename event associated with. **/
private final String fileName;
/** The file contents. */
private final FileContents fileContents;
/** Message associated with the event. **/
private final LocalizedMessage localizedMessage;

/**
* Creates a new {@code TreeWalkerAuditEvent} instance.
*
* @param fileContents contents of the file associated with the event
* @param fileName file associated with the event
* @param localizedMessage the actual message
*/
public TreeWalkerAuditEvent(FileContents fileContents, String fileName,
LocalizedMessage localizedMessage) {
this.fileContents = fileContents;
this.fileName = fileName;
this.localizedMessage = localizedMessage;
}

/**
* Returns name of file being audited.
* @return the file name currently being audited or null if there is
* no relation to a file.
*/
public String getFileName() {
return fileName;
}

/**
* Returns contents of the file.
* @return contents of the file.
*/
public FileContents getFileContents() {
return fileContents;
}

/**
* Gets the localized message.
* @return the localized message
*/
public LocalizedMessage getLocalizedMessage() {
return localizedMessage;
}

/**
* Return the line number on the source file where the event occurred.
* This may be 0 if there is no relation to a file content.
* @return an integer representing the line number in the file source code.
*/
public int getLine() {
return localizedMessage.getLineNo();
}

/**
* Return the message associated to the event.
* @return the event message
*/
public String getMessage() {
return localizedMessage.getMessage();
}

/**
* Gets the column associated with the message.
* @return the column associated with the message
*/
public int getColumn() {
return localizedMessage.getColumnNo();
}

/**
* Returns id of module.
* @return the identifier of the module that generated the event. Can return
* null.
*/
public String getModuleId() {
return localizedMessage.getModuleId();
}

/**
* Gets the name of the source for the message.
* @return the name of the source for the message
*/
public String getSourceName() {
return localizedMessage.getSourceName();
}
}
@@ -0,0 +1,35 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2017 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.puppycrawl.tools.checkstyle;

/**
* An interface for filtering {@code TreeWalkerAuditEvent}.
*
* @author Timur Tibeyev.
*/
@FunctionalInterface
Copy link
Contributor

Choose a reason for hiding this comment

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

You declare functional interface. but do not use lambdas. Please, remove. It is not necessary. Future contributor will not understand your intention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MEZk
Done
Added noinspection javadoc because TeamCity was failing with violation:

27: TreeWalkerFilter Interface TreeWalkerFilter may be annotated with @FunctionalInterface

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnveach
Please explain why do we need the annotation ?

Copy link
Member

Choose a reason for hiding this comment

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

annotation is allowed here: #3435 (comment)

public interface TreeWalkerFilter {
/**
* Determines whether or not a filtered {@code TreeWalkerAuditEvent} is accepted.
* @param treeWalkerAuditEvent the TreeWalkerAuditEvent to filter.
* @return true if the event is accepted.
*/
boolean accept(TreeWalkerAuditEvent treeWalkerAuditEvent);
}