Issue #5154: Incorrect indentation check for method preceded by annot… #5311

djydewang · 2017-12-03T16:05:18Z

…ation, with method parameter on separate line

rnveach · 2017-12-03T16:42:54Z

@djydewang CI must pass.
When CI is fixed, please also produce regression reports for your changes on all projects.

codecov-io · 2017-12-03T16:53:56Z

Codecov Report

Merging #5311 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #5311   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         296     296           
  Lines       16181   16182    +1     
  Branches     3698    3699    +1     
======================================
+ Hits        16181   16182    +1

Impacted Files	Coverage Δ
...kstyle/checks/indentation/LineWrappingHandler.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be89b05...8cacdd2. Read the comment docs.

djydewang · 2017-12-04T14:21:26Z

@rnveach
There is the regression report.
Please confirm the new violation is vaild or invaild.
Thanks!:)

rnveach · 2017-12-04T15:50:54Z

Thanks.
https://djydewang.github.io/diffReport_Issue5154/checkstyle/index.html#A1
https://djydewang.github.io/diffReport_Issue5154/checkstyle/xref/home/bbg/project/contribution/checkstyle-tester/repositories/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationIncorrect.java.html#L7

Old validation seems correct in this scenario. class shouldn't be indented, but previous line should be.
Please confirm and create a new UT and fix this validation.

djydewang · 2017-12-04T16:16:50Z

@rnveach
You mean the new violation in findbugs project is OK, right?

romani · 2017-12-05T12:19:24Z

You mean the new violation in findbugs project is OK, right?

all of them ok to my mind.

Old validation seems correct in this scenario. class shouldn't be indented, but previous line should be.

yes, previous behavior is more correct. Should be fixed.

djydewang · 2017-12-05T13:03:02Z

@romani

Old validation seems correct in this scenario.

Actually, old validation didn't check the line:
class InputAnnotationLocationIncorrect
Just like:

<?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">
  <module name="TreeWalker">
    <module name="Indentation">
      <property name="basicOffset" value="2"/>
      <property name="braceAdjustment" value="0"/>
      <property name="caseIndent" value="2"/>
      <property name="throwsIndent" value="4"/>
      <property name="lineWrappingIndentation" value="4"/>
      <property name="arrayInitIndent" value="2"/>
    </module>
  </module>
</module>

bbg@king-Lenovo:~/checkstyletest$ cat Test.java
package bugreport;

class Test {

  @interface Annotation1 {
    String field1();
    String field2();
  }

  @interface Annotation2 {};

  @Annotation1(field1 = "foo", field2 = "bar")
  public @Annotation2 String method( 
String param                       //violation
  ) {
    return null;
  }

}


bbg@king-Lenovo:~/checkstyletest$ java -jar checkstyle-8.5-all.jar -c identaionConfig.xml Test.java
Starting audit...
[ERROR] Test.java:13: 'public' has incorrect indentation level 2, expected level should be 6. [Indentation]
Audit done.
Checkstyle ends with 1 errors.

And I except this:

[ERROR] Test.java:14: 'String' has incorrect indentation level 0, expected level should be 6. [Indentation]
Audit done.
Checkstyle ends with 1 errors.

I try to fix it and find it's a huge work~:(
So, I plan to fix Issue #5154 first.

romani · 2017-12-05T13:25:54Z

I try to fix it and find it's a huge work~:(

yes, Indentation is most complicated Check and full of bugs and .... .
Probably it might be completely rewritten, but we try to keep regression on leash.

If regression report you have:
for code

   @MyAnnotation2 @com.puppycrawl.tools.checkstyle.checks.annotation.annotationlocation.MyAnnotation1 //warn
   (value = "")
   class InputAnnotationLocationIncorrect
   {

-# | '(' has incorrect indentation level 0, expected level should be 4. | 7 |  
+# | 'class' has incorrect indentation level 0, expected level should be 4. | 8

your PR fix issue #5154 but introduce new issue. I agree that case is weird and unlikely happen in real code. So users might not be affected and get more benefit from your fix.

@rnveach , please share your ideas.

djydewang · 2017-12-05T15:33:04Z

The regression report has updated.

rnveach · 2017-12-06T14:07:06Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/indentation/LineWrappingHandler.java

@@ -262,7 +262,8 @@ private void checkAnnotationIndentation(DetailAST atNode,
            if (isCurrentNodeCloseAnnotationAloneInLine
                    || node.getType() == TokenTypes.AT
                    && (parentNode.getParent().getType() == TokenTypes.MODIFIERS
-                        || parentNode.getParent().getType() == TokenTypes.ANNOTATIONS)) {
+                        || parentNode.getParent().getType() == TokenTypes.ANNOTATIONS)
+                    || parentNode.getType() == TokenTypes.MODIFIERS) {


@pietern all the other checks are verifying that we are validating the annotation, || parentNode.getType() == TokenTypes.MODIFIERS by itself is just saying we should work if it is the modifiers, regardless of annotations or not.
The whole method is annotation related, shouldn't this check be verifying a annotation somehow? Isn't this validating more than it should?

I honestly don't remember, it's been about 2 years since I last used checkstyle :)

sorry, I pinged the wrong person. Don't know how auto-complete picked you. Sorry.

@djydewang I was directing this at you.

djydewang · 2017-12-07T12:51:45Z

@rnveach

all the other checks are verifying that we are validating the annotation

Right, but program shouldn't go to else branch when the node is childnode of modifier.

The whole method is annotation related

No, you can see there is else branch.
It's just an ugly fixing because the old code logic is a little confused.
Actually, old version missed(attention) checking every line behind the end line of annotation.
Just another example:

bbg@king-Lenovo:~/checkstyletest$ java -jar checkstyle-8.5-all.jar -c identaionConfig.xml Test.java
Starting audit...
Audit done.
bbg@king-Lenovo:~/checkstyletest$ cat identaionConfig.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">
  <module name="TreeWalker">
    <module name="Indentation">
      <property name="basicOffset" value="2"/>
      <property name="braceAdjustment" value="0"/>
      <property name="caseIndent" value="2"/>
      <property name="throwsIndent" value="4"/>
      <property name="lineWrappingIndentation" value="4"/>
      <property name="arrayInitIndent" value="2"/>
    </module>
  </module>
</module>

bbg@king-Lenovo:~/checkstyletest$ cat Test.java
package bugreport;

class Test {

  @interface Annotation1 {
    String field1();
    String field2();
  }

  @interface Annotation2 {};

  @Annotation2
     public String method(       //violation
      String param                       
  ) {
    return null;
  }

}

bbg@king-Lenovo:~/checkstyletest$ java -jar checkstyle-8.5-all.jar -c identaionConfig.xml Test.java
Starting audit...
Audit done.

And I excepted:
[ERROR] Test.java:13: 'public' has incorrect indentation level 5, expected level should be 2. [Indentation]
This bug which I have reported before limited fixing.....

The advice of romani:

I agree that case is weird and unlikely happen in real code. So users might not be affected and get more benefit from your fix.

So, I chose to check modifier at where you mentioned along old code logic.

rnveach · 2017-12-08T14:51:39Z

Actually, old version missed(attention) checking every line behind the end line of annotation.

I am not saying your validation is wrong, I am saying that if your are checking non-annotations in an annotation only method then that is wrong. The method is only checking annotations and so bringing in other checks to that method will make the logic even more confusing.

I chose to check modifier at where you mentioned along old code logic.
|| parentNode.getType() == TokenTypes.MODIFIERS

If current node is a normal modifier like public, etc... then it shouldn't be validated here as it is not annotation related.
This sounds like the call to checkAnnotationIndentation is being given too many nodes from the variable annotationLines and that is what needs to be fixed.

djydewang · 2017-12-09T11:18:31Z

If current node is a normal modifier like public, etc... then it shouldn't be validated here as it is not annotation related.

Right, I mean if we are going to check the normal node elsewhere, then we have to fix the issue I reported.
Let me try now.

djydewang · 2017-12-12T15:23:23Z

This sounds like the call to checkAnnotationIndentation is being given too many nodes from the variable annotationLines and that is what needs to be fixed.

Right, I started out in this direction. I've found more and more Issues now, and if we're going to solve them we need to rewrite the LineWrappingHandler module completely.
I just fix logic bug in checkAnnotationIndentation function now.

rnveach

Please generate new difference report for new changes.

I've found more and more Issues now

Please create new issues you have found. There are probably quite a few issues with indentation.

rnveach · 2017-12-12T15:48:59Z

...awl/tools/checkstyle/checks/indentation/indentation/InputIndentationAnnotationIncorrect.java

@@ -0,0 +1,18 @@
+package com.puppycrawl.tools.checkstyle.checks.indentation.indentation; //indent:0 exp:0
+
+class InpuIndentationAnnotationIncorrect { //indent:0 exp:0


https://djydewang.github.io/diffReport_Issue5154/checkstyle/xref/home/bbg/project/contribution/checkstyle-tester/repositories/checkstyle/src/test/resources/com/puppycrawl/tools/checkstyle/checks/annotation/annotationlocation/InputAnnotationLocationIncorrect.java.html#L7
Test case we mentioned before had annotation on top class. Can you replicate it in this input file. You may leave the annotation on the nested class as an additional case.

…d by annotation, with method parameter on separate line

djydewang · 2017-12-13T09:52:57Z

It's regression report.
One thing is strange, I failed to generate report of spring-framework project.

Reseting git sources to commit 'v4.1.6.RELEASE'
HEAD 现在位于 c734ee1 Release version 4.1.6.RELEASE
spring-framework is synchronized
   [delete] Deleting directory /home/bbg/project/contribution/checkstyle-tester/src/main/java
     [copy] Copying 7095 files to /home/bbg/project/contribution/checkstyle-tester/src/main/java/spring-framework
     [copy] Copied 1453 empty directories to 3 empty directories under /home/bbg/project/contribution/checkstyle-tester/src/main/java/spring-framework
Running 'mvn clean' on src/main/java ...
[INFO] Scanning for projects...
[INFO]                                                                         
[INFO] ------------------------------------------------------------------------
[INFO] Building sample 0.0.1-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ sample ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.325s
[INFO] Finished at: Wed Dec 13 15:45:13 CST 2017
[INFO] Final Memory: 6M/106M
[INFO] ------------------------------------------------------------------------
Running Checkstyle on src/main/java ... with excludes 
[INFO] Error stacktraces are turned on.
[INFO] Scanning for projects...
[INFO]                                                                         
[INFO] ------------------------------------------------------------------------
[INFO] Building sample 0.0.1-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- maven-site-plugin:3.0:site (default-site) @ sample ---
[INFO] configuring report plugin org.apache.maven.plugins:maven-checkstyle-plugin:2.17
[INFO] configuring report plugin org.apache.maven.plugins:maven-jxr-plugin:2.5
[WARNING] Report plugin org.apache.maven.plugins:maven-project-info-reports-plugin has an empty version.
[WARNING] 
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING] 
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[INFO] configuring report plugin org.apache.maven.plugins:maven-project-info-reports-plugin:2.9
[WARNING] No project URL defined - decoration links will not be relativized!
[INFO] Rendering site with org.apache.maven.skins:maven-default-skin:jar:1.0 skin.
[INFO] Skipped "Source Xref" report, file "xref/index.html" already exists for the English version.
[INFO] Generating "Checkstyle" report    --- maven-checkstyle-plugin:2.17
[INFO] Generating "Source Xref" report    --- maven-jxr-plugin:2.5
[INFO] Generating "Dependency Convergence" report    --- maven-project-info-reports-plugin:2.9
[INFO] Generating "Dependency Information" report    --- maven-project-info-reports-plugin:2.9
[INFO] Generating "About" report    --- maven-project-info-reports-plugin:2.9
[INFO] Generating "Plugin Management" report    --- maven-project-info-reports-plugin:2.9
[INFO] Generating "Plugins" report    --- maven-project-info-reports-plugin:2.9
[INFO] Generating "Summary" report    --- maven-project-info-reports-plugin:2.9
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 54.224s
[INFO] Finished at: Wed Dec 13 15:46:08 CST 2017
[INFO] Final Memory: 31M/976M
[INFO] ------------------------------------------------------------------------
Running Checkstyle on src/main/java - finished
linking report to index.html
Removing non refernced xref files in report ...
Caught: java.lang.OutOfMemoryError: Java heap space
java.lang.OutOfMemoryError: Java heap space
	at java_nio_file_Files$readAllLines.call(Unknown Source)
	at launch.removeNonReferencedXrefFiles(launch.groovy:243)
	at launch$removeNonReferencedXrefFiles.callCurrent(Unknown Source)
	at launch.postProcessCheckstyleReport(launch.groovy:230)
	at launch$_generateCheckstyleReport_closure2.doCall(launch.groovy:86)
Caught: groovy.lang.GroovyRuntimeException: Error: !
groovy.lang.GroovyRuntimeException: Error: !
	at diff.executeCmd(diff.groovy:344)
	at diff.executeCmd(diff.groovy:338)
	at diff$executeCmd$0.callCurrent(Unknown Source)
	at diff.generateCheckstyleReport(diff.groovy:190)
	at diff$generateCheckstyleReport.callCurrent(Unknown Source)
	at diff.run(diff.groovy:25)

romani · 2017-12-15T21:04:22Z

regression is good
@rnveach , please finalize review.

rnveach · 2017-12-16T16:47:57Z

java.lang.OutOfMemoryError: Java heap spac

You must give atleast 3 gigs of memory to indentation, especially if configuration is not optimized to specific project.

djydewang force-pushed the bbg_Issue5154 branch from 0cb05b4 to 2452596 Compare December 3, 2017 16:53

checkstyle deleted a comment from djydewang Dec 5, 2017

djydewang force-pushed the bbg_Issue5154 branch from 2452596 to 0b05043 Compare December 5, 2017 13:56

romani approved these changes Dec 5, 2017

View reviewed changes

rnveach reviewed Dec 6, 2017

View reviewed changes

djydewang force-pushed the bbg_Issue5154 branch from 0b05043 to b8ff559 Compare December 12, 2017 03:21

rnveach requested changes Dec 12, 2017

View reviewed changes

djydewang force-pushed the bbg_Issue5154 branch from b8ff559 to 9982fe9 Compare December 12, 2017 16:26

Issue checkstyle#5154: Incorrect indentation check for method precede…

8cacdd2

…d by annotation, with method parameter on separate line

djydewang force-pushed the bbg_Issue5154 branch from 37e60ff to 8cacdd2 Compare December 12, 2017 16:37

rnveach approved these changes Dec 16, 2017

View reviewed changes

rnveach merged commit b02eed8 into checkstyle:master Dec 16, 2017

djydewang deleted the bbg_Issue5154 branch January 13, 2018 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #5154: Incorrect indentation check for method preceded by annot… #5311

Issue #5154: Incorrect indentation check for method preceded by annot… #5311

djydewang commented Dec 3, 2017 •

edited

rnveach commented Dec 3, 2017

codecov-io commented Dec 3, 2017 •

edited

djydewang commented Dec 4, 2017 •

edited

rnveach commented Dec 4, 2017

djydewang commented Dec 4, 2017

romani commented Dec 5, 2017

djydewang commented Dec 5, 2017 •

edited

romani commented Dec 5, 2017 •

edited

djydewang commented Dec 5, 2017

rnveach Dec 6, 2017 •

edited

pietern Dec 6, 2017

rnveach Dec 6, 2017

djydewang commented Dec 7, 2017 •

edited

rnveach commented Dec 8, 2017 •

edited

djydewang commented Dec 9, 2017 •

edited

djydewang commented Dec 12, 2017 •

edited by rnveach

rnveach left a comment

rnveach Dec 12, 2017

djydewang Dec 12, 2017

djydewang commented Dec 13, 2017 •

edited

romani commented Dec 15, 2017

rnveach commented Dec 16, 2017 •

edited

		@@ -0,0 +1,18 @@
		package com.puppycrawl.tools.checkstyle.checks.indentation.indentation; //indent:0 exp:0

		class InpuIndentationAnnotationIncorrect { //indent:0 exp:0

Issue #5154: Incorrect indentation check for method preceded by annot… #5311

Issue #5154: Incorrect indentation check for method preceded by annot… #5311

Conversation

djydewang commented Dec 3, 2017 • edited

rnveach commented Dec 3, 2017

codecov-io commented Dec 3, 2017 • edited

Codecov Report

djydewang commented Dec 4, 2017 • edited

rnveach commented Dec 4, 2017

djydewang commented Dec 4, 2017

romani commented Dec 5, 2017

djydewang commented Dec 5, 2017 • edited

romani commented Dec 5, 2017 • edited

djydewang commented Dec 5, 2017

rnveach Dec 6, 2017 • edited

Choose a reason for hiding this comment

pietern Dec 6, 2017

Choose a reason for hiding this comment

rnveach Dec 6, 2017

Choose a reason for hiding this comment

djydewang commented Dec 7, 2017 • edited

rnveach commented Dec 8, 2017 • edited

djydewang commented Dec 9, 2017 • edited

djydewang commented Dec 12, 2017 • edited by rnveach

rnveach left a comment

Choose a reason for hiding this comment

rnveach Dec 12, 2017

Choose a reason for hiding this comment

djydewang Dec 12, 2017

Choose a reason for hiding this comment

djydewang commented Dec 13, 2017 • edited

romani commented Dec 15, 2017

rnveach commented Dec 16, 2017 • edited

djydewang commented Dec 3, 2017 •

edited

codecov-io commented Dec 3, 2017 •

edited

djydewang commented Dec 4, 2017 •

edited

djydewang commented Dec 5, 2017 •

edited

romani commented Dec 5, 2017 •

edited

rnveach Dec 6, 2017 •

edited

djydewang commented Dec 7, 2017 •

edited

rnveach commented Dec 8, 2017 •

edited

djydewang commented Dec 9, 2017 •

edited

djydewang commented Dec 12, 2017 •

edited by rnveach

djydewang commented Dec 13, 2017 •

edited

rnveach commented Dec 16, 2017 •

edited