Issue #3426: remove warning on PACKAGE_DEF predicted by javadoc not separated by line #3549

kazachka · 2016-11-12T18:38:53Z

rnveach

I have some requested changes and some questions to think about as you fix up code to pass CI.
CI is not happy about some style and code coverage.

rnveach · 2016-11-12T23:39:28Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

+     * @param token token.
+     * @return true, if token is predicted by javadoc comment.
+     */
+    private boolean isPredictedByJavadoc(DetailAST token) {


How does predicted fit into what this method does?
I think the correct word here is preceded since we are checking if the javadoc comes before (precedes) the token.
Please update name and javadoc.

rnveach · 2016-11-12T23:42:40Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

@@ -534,6 +534,37 @@ private boolean hasEmptyLineBefore(DetailAST token) {
    }

    /**
+     * Check if token is predicted by javadoc comment
+     * @param token token.


Can we get a slightly better description than just duplicating the parameter name?

rnveach · 2016-11-13T00:05:32Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

+            String lineBefore;
+            int lineIndex = 2;
+            do {
+                lineBefore = getLines()[lineNo - lineIndex].trim();


why not just make lineIndex the value of lineNo - 2 and decrement each loop. This will save us from having to redo this calculation each time through, twice.

Also please stash the result of getLines as we do a clone on the values each call.

rnveach · 2016-11-13T00:15:12Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

+                else if (lineBefore.length() == 0) {
+                    break;
+                }
+                else if (lineBefore.charAt(0) != '*') {


What if the user has a space/tab before the *?

For this case we use trim:
lineBefore = lines[lineIndex].trim();

rnveach · 2016-11-13T00:15:53Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

+            do {
+                lineBefore = getLines()[lineNo - lineIndex].trim();
+                if (lineBefore.startsWith("/**")) {
+                    result = true;


If we know the result is true, then why don't we break out now?

romani · 2016-11-13T13:22:17Z

@kazachka , please make sure that all CIs are green , I skimmed them and all of them are real problems.

codecov-io · 2016-11-15T21:18:53Z

Current coverage is 100% (diff: 100%)

Merging #3549 into master will not change coverage

@@           master   #3549   diff @@
=====================================
  Files         275     275          
  Lines       13335   13352    +17   
  Methods         0       0          
  Messages        0       0          
  Branches     3039    3047     +8   
=====================================
+ Hits        13335   13352    +17   
  Misses          0       0          
  Partials        0       0

Powered by Codecov. Last update c2bbc97...2a32f8c

kazachka · 2016-11-15T22:10:19Z

Done

rnveach

Minor changes.
Once these are made, I am fine with the code.

rnveach · 2016-11-16T00:31:24Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

+        final String[] lines = getLines();
+        String lineBefore = lines[lineIndex].trim();
+        while (!lineBefore.startsWith("/**")) {
+            if (lineIndex == 0 || !lineBefore.isEmpty() && lineBefore.charAt(0) != '*') {


Taken from: http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html#packagecomment

Note that while the comment separators /** and /* must be present, the leading asterisks on the intermediate lines can be omitted.

Taken from: http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html#leadingasterisks

Leading asterisks - When javadoc parses a doc comment, leading asterisk (*) characters on each line are discarded; blanks and tabs preceding the initial asterisk (*) characters are also discarded. Starting with 1.4, if you omit the leading asterisk on a line, the leading white space is no longer removed. This enables you to paste code examples directly into a doc comment inside a <PRE> tag, and its indentation will be honored. Spaces are generally interpreted by browsers more uniformly than tabs. Indentation is relative to the left margin (rather than the separator /** or <PRE> tag).

We should accept Javadocs with no leading asterisks. If they omit the leading asterisk, your current code will fail thinking it is not part of the Javadoc as your lineBefore.charAt(0) != '*' is looking for the leading asterisk.
Since we are looking above the package it can't be another node as there are no valid nodes that go above the package. We can only run into Javadocs or Comments.

I think we should change the leading asterisk check to a leading comment check (// or /*).
We should also have a test for this.

@romani Do you agree?

yes, we have to support both approaches to write javadoc.

current implementations is fragile by design, as this Check is not comments aware - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

to receive all comments well parsed Check need to override method

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

Example:
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/TodoCommentCheck.java#L68

in this case Check will have all comments in good form.
Useful method - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java#L246

I will write a bit more about isCommentNodesRequired in scope of #3561

rnveach · 2016-11-16T00:47:53Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

@@ -534,6 +534,30 @@ private boolean hasEmptyLineBefore(DetailAST token) {
    }

    /**
+     * Check if token is preceded by javadoc comment
+     * @param token token for check.
+     * @return true, if token is predicted by javadoc comment.


You missed renaming to preceded.

rnveach · 2016-11-22T01:19:07Z

@kazachka CI is failing.
I also don't believe we are expecting changes in JavadocDetailNodeParser.java.

romani · 2016-11-30T00:27:08Z

@kazachka ,

CI is failed at - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info.java

package-info.java are not ordinary java files.

Here is example of similar problem - module-info.java
http://hg.openjdk.java.net/jdk9/dev/jdk/file/b2a69d66dc65/src/jdk.naming.rmi/share/classes/module-info.java , such files will be in java9.
module-info.java we are skipping as we do not have parser for them for now.

package-info.java is smth similar but match by grammar to java files so that is why we process them.
Smth that is possible in module-info.java in not possible in java files that contains class.

Resolution: we have special files that we need to process separately in this Check. There is an ability to get name of the file in the Check, so pelase check it and make logic to allow no space for javadoc and annotations in package-info.java files.
We need to mention this nuance in javadoc/xdoc.

kazachka · 2016-11-30T21:05:42Z

Update to allow to placing javadoc and annotations before PACKAGE_DEF only in package-info.java

romani · 2016-12-01T03:22:21Z

@kazachka ,

http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html

package-info.java - Can contain a package declaration, package annotations, package comments and Javadoc tags. This file is generally preferred over package.html.

So please remove "import" from package-info.java inputs.

https://docs.oracle.com/javase/8/docs/technotes/tools/unix/javac.html

-Xpkginfo:[always,legacy,nonempty]
    Control whether javac generates package-info.class files from package-info.java files. Possible mode arguments for this option include the following.
    always
        Always generate a package-info.class file for every package-info.java file. This option may be useful if you use a build system such as Ant, which checks that each .java file has a corresponding .class file.
    legacy
        Generate a package-info.class file only if package-info.java contains annotations. Don't generate a package-info.class file if package-info.java only contains comments.
        Note: A package-info.class file might be generated but be empty if all the annotations in the package-info.java file have RetentionPolicy.SOURCE.
    nonempty
        Generate a package-info.class file only if package-info.java contains annotations with RetentionPolicy.CLASS or RetentionPolicy.RUNTIME.

you can see fact that it is java file is actually cause nuances(compatibility modes for other tools like ANT :) ) for javac too.

Please in comments over packages in package-info.java state that there is not violations are expected.

I reviewed changes for trailing comments removal in inputs .... I do not like this ...
Trailing comments should not be considered as some separation. Can we update Check to skip training comments?

kazachka · 2016-12-02T08:27:09Z

Google Guava has import declarations for annotations in package-info.java, for example there:
https://github.com/google/guava/blob/master/guava/src/com/google/common/base/package-info.java

If I remove import declarations check will not validate PACKAGE_DEF because of this:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/whitespace/EmptyLineSeparatorCheck.java

Line 289 in 25a37e5

if (nextToken != null) {

kazachka · 2016-12-02T12:26:01Z

and 3. done

romani

more items to fix

romani · 2016-12-03T16:19:34Z

...urces/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test1/package-info.java

@@ -0,0 +1,5 @@
+/*OK: for test allowing to place annotation before PACKAGE_DEF.*/
+@Deprecated
+package com.puppycrawl.tools.checkstyle.checks.whitespace.test1.packageinfo;


com.puppycrawl.tools.checkstyle.checks.whitespace.test1.packageinfo

vs

com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test1

path should be the same as package - "com/puppycrawl/tools/checkstyle/checks/whitespace/packageinfo/test1", or package the same path.
Please fix other cases too.

romani · 2016-12-03T16:21:08Z

...urces/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test1/package-info.java

+@Deprecated
+package com.puppycrawl.tools.checkstyle.checks.whitespace.test1.packageinfo;
+
+import java.lang.Deprecated;


please do import of the same annotations(2 annotations) as in Guava file that I/you mentioned above.

Done, but tests aren't compile, it needs to import javax.annotation package in pom.xml

yes, ... adding new dependency only for Inputs is too much.
I am ok to keep usage of Deprecated only. Looks like it is only annotation that could be applied to packages in jdk.

romani · 2016-12-03T16:22:03Z

...urces/com/puppycrawl/tools/checkstyle/checks/whitespace/package-info/test2/package-info.java

+/**OK: for test allowing to place javadoc before PACKAGE_DEF.*/
+package com.puppycrawl.tools.checkstyle.checks.whitespace.test2.packageinfo;
+
+import java.lang.System;


what for we have import here ? (the same question to all other Input files)

EmptyLineSeparatorCheck doesn't check last token. I place import after PACKAGE_DEF to make package declaration no last.

please put this nuance as a comment in Input file.

romani · 2016-12-04T16:09:34Z

please revert changes for usage of javax annotations, my bad.

kazachka · 2016-12-04T17:15:36Z

updated

kazachka · 2016-12-04T17:18:44Z

Is it good that check allow tokens not separated by line before PACKAGE_DEF with annotations?

romani · 2016-12-04T18:51:33Z

Kind of OK.
According to grammar, onlt annotations and comments are posible to put. We cover them now.

romani · 2016-12-04T19:03:40Z

Please privide diff report for several projects to make sure that we dud not damage anithing else

kazachka · 2016-12-05T18:40:38Z

isTrailingComment() updated.
Diff-report:
https://kazachka.github.io/empty-line-separator-diff-report/

romani · 2016-12-05T20:14:58Z

home/zenigata/workspace/contribution/checkstyle-tester/src/main/java/apache-ant/src/main/org/apache/tools/ant/taskdefs/optional/ejb/EjbJar.java Severity Rule Message Line Co warning EmptyLineSeparator '//' should be separated from previous statement. 126

This and similar are not expected to appear.

kazachka · 2016-12-05T20:22:46Z

Which tokens shouldn't be separated from comments by line?

romani · 2016-12-05T20:54:39Z

No separation in this case.
There was no such violation before update and there should be no such violation after your update.

kazachka · 2016-12-09T11:53:07Z

added isTrailingCommentMethod to allow comments after all of tokens except of package and imports definitions.

kazachka · 2016-12-09T12:06:54Z

Diff-report:
https://kazachka.github.io/empty-line-separator-diff-report/

romani · 2016-12-09T18:52:38Z

@kazachka , both new violations(green) are not expected.
Please move them to Inputs and UTs.

kazachka · 2016-12-11T12:43:16Z

Updated.
Diff-report:
https://kazachka.github.io/empty-line-separator-diff-report/

romani · 2016-12-11T13:35:22Z

report is ok, but I do not see in UTs and Inputs all cases where you had regression with false-positives.
Please add them to Inputs, if run into regression ones it could happen second time too.

…doc not separated by line

kazachka · 2016-12-12T11:31:55Z

I add more test cases for comments.

kazachka force-pushed the fix-in-empty-line-separator branch 4 times, most recently from 92224dc to fd94c60 Compare November 12, 2016 21:08

rnveach requested changes Nov 13, 2016

View reviewed changes

kazachka force-pushed the fix-in-empty-line-separator branch 2 times, most recently from ccdc33b to 17f3cfc Compare November 13, 2016 12:24

kazachka force-pushed the fix-in-empty-line-separator branch 2 times, most recently from e9e67f5 to d9e4e56 Compare November 15, 2016 21:00

kazachka force-pushed the fix-in-empty-line-separator branch from d9e4e56 to 4202c76 Compare November 15, 2016 21:55

rnveach requested changes Nov 16, 2016

View reviewed changes

kazachka force-pushed the fix-in-empty-line-separator branch 2 times, most recently from 496b04f to 12c6220 Compare November 18, 2016 18:53

kazachka force-pushed the fix-in-empty-line-separator branch 2 times, most recently from fff7302 to f3618ab Compare November 26, 2016 18:53

kazachka force-pushed the fix-in-empty-line-separator branch 2 times, most recently from 3e2b066 to 380705c Compare November 30, 2016 20:08

kazachka force-pushed the fix-in-empty-line-separator branch 4 times, most recently from 7848d5c to 831949a Compare December 2, 2016 12:01

romani requested changes Dec 3, 2016

View reviewed changes

kazachka force-pushed the fix-in-empty-line-separator branch from 4390076 to 5e5e932 Compare December 4, 2016 08:20

kazachka force-pushed the fix-in-empty-line-separator branch from 5e5e932 to 92fe995 Compare December 4, 2016 16:59

romani approved these changes Dec 4, 2016

View reviewed changes

rnveach approved these changes Dec 5, 2016

View reviewed changes

kazachka force-pushed the fix-in-empty-line-separator branch 3 times, most recently from 4cf2233 to 9f8bcfa Compare December 5, 2016 18:17

kazachka force-pushed the fix-in-empty-line-separator branch from 9f8bcfa to 8ae8603 Compare December 9, 2016 11:35

kazachka force-pushed the fix-in-empty-line-separator branch from 8ae8603 to b0eaddd Compare December 9, 2016 12:00

kazachka force-pushed the fix-in-empty-line-separator branch from b0eaddd to 4a57c96 Compare December 11, 2016 12:37

kazachka force-pushed the fix-in-empty-line-separator branch from 4a57c96 to cc84ae7 Compare December 12, 2016 10:21

Issue checkstyle#3426: remove warning on PACKAGE_DEF preceded by java…

2a32f8c

…doc not separated by line

kazachka force-pushed the fix-in-empty-line-separator branch from cc84ae7 to 2a32f8c Compare December 12, 2016 10:47

romani merged commit 65fe91d into checkstyle:master Dec 12, 2016

Issue #3426: remove warning on PACKAGE_DEF predicted by javadoc not separated by line #3549

Issue #3426: remove warning on PACKAGE_DEF predicted by javadoc not separated by line #3549

Conversation

kazachka commented Nov 12, 2016

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Nov 13, 2016

codecov-io commented Nov 15, 2016 • edited

Current coverage is 100% (diff: 100%)

kazachka commented Nov 15, 2016

rnveach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani Nov 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnveach commented Nov 22, 2016

romani commented Nov 30, 2016

kazachka commented Nov 30, 2016

romani commented Dec 1, 2016 • edited

kazachka commented Dec 2, 2016

kazachka commented Dec 2, 2016 • edited

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazachka Dec 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Dec 4, 2016

kazachka commented Dec 4, 2016

kazachka commented Dec 4, 2016 • edited

romani commented Dec 4, 2016

romani commented Dec 4, 2016

kazachka commented Dec 5, 2016 • edited

romani commented Dec 5, 2016 • edited

kazachka commented Dec 5, 2016

romani commented Dec 5, 2016

kazachka commented Dec 9, 2016

kazachka commented Dec 9, 2016

romani commented Dec 9, 2016

kazachka commented Dec 11, 2016

romani commented Dec 11, 2016

kazachka commented Dec 12, 2016

codecov-io commented Nov 15, 2016 •

edited

romani Nov 18, 2016 •

edited

romani commented Dec 1, 2016 •

edited

kazachka commented Dec 2, 2016 •

edited

kazachka Dec 4, 2016 •

edited

kazachka commented Dec 4, 2016 •

edited

kazachka commented Dec 5, 2016 •

edited

romani commented Dec 5, 2016 •

edited