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

TranslationCheck prints violations from previous AbstractFileSetCheck run #5263

Closed
rnveach opened this Issue Nov 14, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Nov 14, 2017

I cannot reliably reproduce this in a small case. Most likely it has to do with the random nature that checks are run and this bug requires specific scenario.

When I run checkstyle on itself on one PC, I get weird output on a clean run with no cache file.

Starting audit...
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:22: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:23: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:39: Missing a Javadoc comment. [JavadocType]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:39: Type Javadoc comment is missing an null tag. [WriteTag]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:41:5: Missing a Javadoc comment. [JavadocVariable]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:43:5: Missing a Javadoc comment. [JavadocVariable]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:45:5: Missing a Javadoc comment. [JavadocVariable]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:52:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:60:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:68:13: The String "/CLASS_DEF[@text='InputXpathQueryGenerator']" appears 4 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:71:22: The String "Generated queries do not match expected ones" appears 21 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:74:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:91:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:99:13: The String "/CLASS_DEF[@text='InputXpathQueryGenerator']/OBJBLOCK" appears 16 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:100:23: The String "/METHOD_DEF[@text='callSomeMethod']/SLIST/LITERAL_FOR" appears 4 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:114:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:122:13: The String "/CLASS_DEF[@text='InputXpathQueryGenerator']/OBJBLOCK/METHOD_DEF[@text='Label']" appears 3 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:127:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:140:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:155:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:168:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:178:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:190:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:202:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:213:23: The String "/METHOD_DEF[@text='saveUser']/PARAMETERS/PARAMETER_DEF[@text='name']" appears 3 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:223:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:236:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:251:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:260:23: The String "/METHOD_DEF[@text='callSomeMethod']/SLIST/VARIABLE_DEF[@text='another']" appears 2 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:268:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:281:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:296:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:308:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:320:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:322:48: The String "InputXpathQueryGeneratorTabWidth.java" appears 4 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:333:17: The String "/CLASS_DEF[@text='InputXpathQueryGeneratorTabWidth']/OBJBLOCK" appears 7 times in the file. [MultipleStringLiterals]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:342:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:362:5: Missing a Javadoc comment. [JavadocMethod]
[ERROR] C:\checkstyleWorkspaceEclipse\checkstyle\src\main\resources\com\puppycrawl\tools\checkstyle\checks\messages_fr.properties:380:5: Missing a Javadoc comment. [JavadocMethod]
Audit done.
Checkstyle ends with 39 errors.

The problem occurs under these conditions:

  1. 2 checks must be defined in the configuration. One check must be TranslationCheck, doesn't matter what the other is as long as it produces a violation.
  2. The files to be run must be in the order of any type of file that produces a violation by the first check, and a property file.
  3. In Checker, TranslationCheck must be executed first followed by the check that produces a violation.

When this occurs, TranslationCheck leaks the violations when the finishProcessing method is run from the previous Check and assigns those violations under the File name TranslationCheck is using.

Code to prove my case:
When a check is run and finished, we grab the violations and return them to the caller.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java#L77-L84
The problem is we don't erase the field MESSAGE_COLLECTOR after this method is run. We expect a 2nd method call to erase the list, but if it is never run, the list is kept populated.
When finishProcessing is called for TranslationCheck, it always calls fireErrors regardless if errors were generated or not. But we never produced any new violations or cleared out the list from the last run, so we leak violations from the previous process call and reprint them under the TranslationCheck file it is examining.
This doesn't leak more violations because we clear the violation list after the first call to fireErrors.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java#L174-L177

@rnveach rnveach added the bug label Nov 14, 2017

@romani romani added the approved label Nov 14, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 15, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 15, 2017

rnveach added a commit to rnveach/checkstyle that referenced this issue Nov 15, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 16, 2017

Member

It sounds like easy to reproduce by CLI

Config with two checks and execution on two files.


I do not see how Translation check can avoid execution of process(File file, FileText fileText) and do not clean violations. Please point me to the code or provide more detailed scenarios.

Member

romani commented Nov 16, 2017

It sounds like easy to reproduce by CLI

Config with two checks and execution on two files.


I do not see how Translation check can avoid execution of process(File file, FileText fileText) and do not clean violations. Please point me to the code or provide more detailed scenarios.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 16, 2017

Member

It sounds like easy to reproduce by CLI

I will try again, but I couldn't reproduce on linux.

Translation check can avoid execution of process(File file, FileText fileText) and do not clean violations.

It does not avoid executing process.
Checker calls process and process only cleans violations when method is started, not after it is finished. So after the call, it leaves violations in the list. These violations are all violations, even ones that are suppressed.
TranslationCheck fires errors in finishProcessing which comes after Checker finishes all the process calls. Thats when it picks up the left over violations from the previous process call.

This is all because TranslationCheck is a multi-file check.

Member

rnveach commented Nov 16, 2017

It sounds like easy to reproduce by CLI

I will try again, but I couldn't reproduce on linux.

Translation check can avoid execution of process(File file, FileText fileText) and do not clean violations.

It does not avoid executing process.
Checker calls process and process only cleans violations when method is started, not after it is finished. So after the call, it leaves violations in the list. These violations are all violations, even ones that are suppressed.
TranslationCheck fires errors in finishProcessing which comes after Checker finishes all the process calls. Thats when it picks up the left over violations from the previous process call.

This is all because TranslationCheck is a multi-file check.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Nov 16, 2017

Member

@romani Here is something that I can reproduce. I didn't realize 2 message property files are required, not just 1.

$ cat TestClass.java
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class TestClass {
    void method() {
    }
}

$ cat messages.properties
test=test

$ cat messages_de.properties
test=test

$ cat Suppressions.xml
<?xml version="1.0"?>

<!DOCTYPE suppressions PUBLIC
    "-//Puppy Crawl//DTD Suppressions 1.1//EN"
    "http://checkstyle.sourceforge.net/dtds/suppressions_1_1.dtd">

<suppressions>
    <suppress checks="AvoidStaticImport" files="TestClass.java"/>
</suppressions>

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">

<module name="Checker">
  <property name="severity" value="error"/>

  <property name="fileExtensions" value="java, properties, xml, vm, g, g4, dtd"/>

  <!-- Filters -->
  <module name="SuppressionFilter">
    <property name="file" value="Suppressions.xml"/>
  </module>

  <module name="Translation">
  </module>

  <module name="TreeWalker">
    <property name="tabWidth" value="4"/>

    <module name="AvoidStaticImport"/>
  </module>
</module>


$ java -jar checkstyle-8.4-all.jar -c TestConfig.xml messages.properties messages_de.properties TestClass.java
Starting audit...
[ERROR] messages.properties:1: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
[ERROR] messages.properties:2: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Audit done.
Checkstyle ends with 2 errors.
Member

rnveach commented Nov 16, 2017

@romani Here is something that I can reproduce. I didn't realize 2 message property files are required, not just 1.

$ cat TestClass.java
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;

public class TestClass {
    void method() {
    }
}

$ cat messages.properties
test=test

$ cat messages_de.properties
test=test

$ cat Suppressions.xml
<?xml version="1.0"?>

<!DOCTYPE suppressions PUBLIC
    "-//Puppy Crawl//DTD Suppressions 1.1//EN"
    "http://checkstyle.sourceforge.net/dtds/suppressions_1_1.dtd">

<suppressions>
    <suppress checks="AvoidStaticImport" files="TestClass.java"/>
</suppressions>

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://checkstyle.sourceforge.net/dtds/configuration_1_3.dtd">

<module name="Checker">
  <property name="severity" value="error"/>

  <property name="fileExtensions" value="java, properties, xml, vm, g, g4, dtd"/>

  <!-- Filters -->
  <module name="SuppressionFilter">
    <property name="file" value="Suppressions.xml"/>
  </module>

  <module name="Translation">
  </module>

  <module name="TreeWalker">
    <property name="tabWidth" value="4"/>

    <module name="AvoidStaticImport"/>
  </module>
</module>


$ java -jar checkstyle-8.4-all.jar -c TestConfig.xml messages.properties messages_de.properties TestClass.java
Starting audit...
[ERROR] messages.properties:1: Using a static member import should be avoided - org.junit.Assert.assertEquals. [AvoidStaticImport]
[ERROR] messages.properties:2: Using a static member import should be avoided - org.junit.Assert.assertTrue. [AvoidStaticImport]
Audit done.
Checkstyle ends with 2 errors.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 18, 2017

Member

TranslationCheck fires errors in finishProcessing which comes after Checker finishes all the process calls. Thats when it picks up the left over violations from the previous process call.
This is all because TranslationCheck is a multi-file check.

Hmm, multi-file Checks are not properly supported so are not considered in design changes for others.
Issue is valid.

Member

romani commented Nov 18, 2017

TranslationCheck fires errors in finishProcessing which comes after Checker finishes all the process calls. Thats when it picks up the left over violations from the previous process call.
This is all because TranslationCheck is a multi-file check.

Hmm, multi-file Checks are not properly supported so are not considered in design changes for others.
Issue is valid.

romani added a commit that referenced this issue Nov 18, 2017

@romani romani added this to the 8.5 milestone Nov 18, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Nov 18, 2017

Member

fix is merged.

Member

romani commented Nov 18, 2017

fix is merged.

@romani romani closed this Nov 18, 2017

timurt added a commit to timurt/checkstyle that referenced this issue Dec 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment