JavadocMethod: Javadoc Not Detected Above Multiline Comments

[checkstyle]

JavadocMethod: Javadoc Not Detected Above Multiline Comments

[m1shaq]

Hi, I have switched to version 8.30 and all of a sudden, the checkstyle has stopped noticing me of no javadoc comments before any method or before the constructor. Here is my JavadocMethod module

<module name="JavadocMethod">
        <property name="allowMissingParamTags" value="true"/>
        <property name="allowMissingReturnTag" value="true"/>
</module>

I don't know what else I should provide, since it's my first post here, so sorry for any obvious goofs.

[romani]

unfortunately Javadoc is handled by RegExp in Checkstyle for now, there is no simple way to six that in code in reliable way.

[dmzimmerman]

Hmm. So there's no way to change the regexp so that it also matches comments after the Javadoc comment? That's unfortunate.

[dmzimmerman]

It seems that Javadoc comments are found by calling getJavadocBefore() in com.puppycrawl.tools.checkstyle.api.FileContents, and that in turn explicitly skips over single line comments but not multi-line comments. Making it skip over multi-line comments as well would probably not be too difficult (I could take a stab at it but I'm pretty unfamiliar with the codebase).

[dmzimmerman]

In fact, it seems that there is a map in FileContents from line numbers to C comments; finding all the the C comments that are not possible Javadoc comments and that occur above the line number of the top of the method would probably work, with the possible exception of the degenerate case where the code looks like this:

/**
 * A Javadoc comment.
 */ /* A C comment
     * 
     */

I suspect such degenerate cases are exceptionally uncommon.

[romani]

Checkstyle is very simple and very modular , to fix a problem you need to debug only one file - check that have problem. So if you so care about this problem - please be welcome with patch.

FYI: do parsing by RegExp is never a good approach, one day we will update grammar to parser JavaDoc more properly - so after that happen, it will be a reasonable to spend maintainers time on it.
We already have numerous issues with JavaDoc support: https://sourceforge.net/p/checkstyle/bugs/?source=navbar .

[dmzimmerman]

Yes, Checkstyle is modular... but this particular check is not, since it explicitly relies, as I said, on that FileContents class to properly find the Javadoc comment before a particular point and it's the finding of the Javadoc that's a problem. I'll take a look at the FileContents class and see if I can convince it to do something more intelligent.

[dmzimmerman]

I believe that this diff on com.puppycrawl.tools.checkstyle.api.FileContents should fix it: instead of checking only for a line being blank or a single-line comment before moving to the previous line, it checks that the line isn't blank, doesn't intersect any comment (using existing intersection check functionality), and isn't the end of a Javadoc comment before moving to the previous line.

index 6014aac..77a3a74 100644
--- a/src/checkstyle/com/puppycrawl/tools/checkstyle/api/FileContents.java
+++ b/src/checkstyle/com/puppycrawl/tools/checkstyle/api/FileContents.java
@@ -213,8 +213,12 @@ public final class FileContents implements CommentListener
         // Lines start at 1 to the callers perspective, so need to take off 2
         int lineNo = aLineNo - 2;

-        // skip blank lines
-        while ((lineNo > 0) && (lineIsBlank(lineNo) || lineIsComment(lineNo))) {
+        // skip blank lines and comments
+        // we skip comments by checking to see if any part of the
+        // current line intersects one.
+        while ((lineNo > 0) && mJavadocComments.get(lineNo) == null && 
+               (lineIsBlank(lineNo) || 
+                hasIntersectionWithComment(lineNo, 0, lineNo, Integer.MAX_VALUE))) {
             lineNo--;
         }

[romani]

The more I look at JML JavaDoc, the more I think that Checkstyle should not be changed in favour of your project, sorry.
Even following code is bug in Checkstyle as it is wrong usage of comments in Java and bad practice to my mind.

/**
 * A Javadoc comment.
 */
// any comment
public void foo() {}

I would recommend you to make fork from us, and build JML-Checkstyle on your own with patch that you provided or even more make your own custom Checks see that project as example (https://github.com/sevntu-checkstyle/sevntu.checkstyle).
I will try to make other maintainers to review your request to have second opinion.

[dmzimmerman]

"It is wrong usage of comments in Java and bad practice to my mind" is an odd judgement call to make on your part, especially since Checkstyle currently explicitly supports the syntax you just wrote in your comment, while disallowing other forms of comments between Javadoc and a method declaration. Plus, there really is no such thing as "wrong usage of comments in Java"; comments are allowed anywhere and I'm not aware of any well-respected coding standards that say otherwise.

The placement of comments between Javadoc and a method declaration is understood perfectly by both the official Javadoc tool's standard doclet and by Eclipse's Javadoc parser. Whether non-Javadoc comments should be placed either above or below the Javadoc comments seems to me to be a personal preference issue, along the lines of whether braces should be placed at the ends of lines or on new lines, and as such - like the brace placement issue - is probably not a judgement that Checkstyle should be making for all its users.

Put another way: it seems to me that either Checkstyle should allow no comments between a Javadoc comment and a method declaration, or it should allow any comments between a Javadoc comment and a method declaration; it should not do something in between solely because of lazy coding.

[romani]

comments are allowed anywhere and I'm not aware of any well-respected coding standards that say otherwise.

Whole Checkstyle is about to forbid user to do that is allowed :), Nobody develop with 100% following of Sun guidelines, everyone invent style/rules that fit their team, and Checkstyle help to keep an eye on these rules.
Please give me real use case of such style, out of JML.

like the brace placement issue - is probably not a judgement that Checkstyle should be making for all its users.

Right, so that is why Check should have options, to let use check exactly as user want.

Put another way: .....

You are in right direction, so Check have to have an option to allow comments between or not, by default it will be not allowed. But before implementing that feature please prove that is required not only by JML :), I need examples and evidences.

[dmzimmerman]

You say "Please give me real use case of such style, out of JML" as though JML is some fly-by-night project that I just made up rather than a decade-old de facto standard for formal specification of Java programs, used both by actual developers and in educational contexts, which is understood by many Java specification, testing and verification tools and has inspired multiple other formal specification languages (including the commercial Frama-C for C programs).

Still, as an example of how the current implementation is broken, even outside the presence of JML, consider these two examples where a developer wants to explain why he used a particular Java annotation:

/**
 * A Javadoc comment.
 */
@SuppressWarnings("unchecked")
/* generic type warnings are suppressed for this method 
   because of reason xyz */
public void method() {}

/**
 * A Javadoc comment.
 */
/* generic type warnings are suppressed for this method 
   because of reason xyz */
@SuppressWarnings("unchecked")
public void method() {}

You'd expect those to have the same result in Checkstyle, right? But they don't. The first one, surprisingly enough, checks fine. The second one causes Checkstyle to complain about missing Javadoc. Of course if the comments were single-line comments, neither would cause Checkstyle to complain; but does it really make any sense at all to force developers to choose between single-line and multi-line comments based on how broken Checkstyle's Javadoc recognition is?

Perhaps you would prefer for the comment to appear above the Javadoc, like this:

/* generic type warnings are suppressed for this method 
   because of reason xyz */
/**
 * A Javadoc comment.
 */
@SuppressWarnings("unchecked")
public void method() {}

I think that looks ridiculous, and that it makes far more sense to place a comment about an annotation adjacent to that annotation - not inside the method, and not above the Javadoc; moreover, there's no legitimate reason for that comment to be restricted to be of the single-line variety.

Essentially, what you have here is a situation where a method has legal Javadoc, the Javadoc tool says the method has legal Javadoc, every other current Javadoc parser says the method has legal Javadoc, and Checkstyle is the only tool that complains. Morever, when Checkstyle complains, it doesn't say "the method has Javadoc in a location we don't like", but rather, "the method is missing Javadoc." Checkstyle is lying to me when it checks my code and says this. It shouldn't matter to Checkstyle whether I'm writing JML comments above my methods or whether I'm writing short stories above them; what should matter to Checkstyle is whether Javadoc for the method exists, since that is what it is claiming to check. To boil it down to its essence: this check as currently implemented is fundamentally broken because it disagrees with both the authoritative tools and the Javadoc specification about when a Javadoc comment for a method exists.

All that aside, though; I would be thrilled with the method Javadoc check having an option to allow or disallow comments in between, if it were possible, and I wouldn't even care if you chose the wrong default (hint: the correct default is the one that agrees with standard Javadoc parsers). I'd gladly implement it myself, and if you didn't want to include it in Checkstyle I'd distribute it both to my students and with the JML tools. However, the way Javadoc identification is currently implemented, it seems like it's not actually possible to do that simply by changing the check; the method Javadoc check relies on Checkstyle's FileContents API to tell it where Javadoc comments are, and the way Checkstyle searches for Javadoc comments above a particular line is undeniably broken. If that search were fixed (this is what the patch I wrote attempts to do), it would be relatively straightforward to also implement logic in the Javadoc check that says "ok, now that you've found the Javadoc for this method, was there any other comment in between?".

[isopov]

I think we should support this case since it is supported by other javadocs tools.

[isopov]

Please give me real use case of such style

public class Test{
    /**
    * Foo Method foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar foobar
    */
    //CHECKSTYLE:OFF
    public void FooBar(){
    //CHECKSTYLE:ON
        System.out.println("Hello World!");
    }
}

[romani]

as though JML is some fly-by-night project

I did not mean anything bad, I appreciate your work and your interest in Checkstyle and your time you spend in this issue. ....... .

because it disagrees with both the authoritative tools and the Javadoc specification about when a Javadoc

this just a bug, that you experience the most.

an option to allow or disallow comments in between

I take my idea back, as it would reasonable to do only after introduction of Comments to ANTLR grammar. Lets proceed with minor fixes.
For future reference : http://docs.oracle.com/javase/7/docs/technotes/tools/solaris/javadoc.html
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

I'd gladly implement it myself,

Ok, thanks for your persistence, I marked issue as "Approved", please provide pull request, but do not forget about UTs.

[romani]

@dmzimmerman , we implemented full support on comments and javadoc parsing in Checkstyle.
To support your comments notation you can do it yourself, comments Nodes are now inlined to java AST. You can see example of work :
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/TodoCommentCheck.java
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocParagraphCheck.java

I highly recommend you to create grammar for your comments and parse it as we do Javadoc.
As you be ready we could update https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java#L161 to call custom Comment parser.

[romani]

http://checkstyle.sourceforge.net/config_javadoc.html#JavadocMethod should be updated to use comments from AST tree, rather than from FileContent.java file. Check need to be reimplemented to use new API for JavaDocs.

[MEZk]

@romani
Can I try to reimplement the Check?

[rnveach]

@MEZk I think this will be covered by GSoC if the project is picked, https://github.com/checkstyle/checkstyle/wiki/Checkstyle-GSoC-2017-Project-Ideas#project-name-coverage-of-documentation-comments-style-guide-and-performance-optimization-of-javadoc-parser

[romani]

@MEZk , reimplementation is part of GSoC project.
I am not sure if issue be fixed after reimplementation, this issue and reimplementation are two different issues.

[rnveach]

Example of issue:

$ cat TestClass.java
public class TestClass {
/**
 * A Javadoc comment.
 */
//@ A JML Annotation
public int foo() { return 0; } 

/**
 * A Javadoc comment.
 */
/*@ A JML Annotation */
public int foo2() { return 0; } 
}

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

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="JavadocMethod" />
    </module>
</module>

$ java -jar checkstyle-8.27-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:6: Expected @return tag. [JavadocMethod]
Audit done.
Checkstyle ends with 1 errors.

Both methods should be handled the same but only 1 produces a violation.

As discussed above, issue is with FileContents.getJavadocBefore and this is used by 8 checks, so each one is affected by this bug. We have checks use this which are not AbstractJavadocCheck and probably will never be converted to it s it wouldn't make sense. Example: MissingJavadocMethodCheck .

So this issue needs to be fixed regardless of type of check it is. I believe we have other checks that use their own implementation of finding the javadoc.

[rnveach]

It seems FileContents.getJavadocBefore allows checks a way to bypass checkstyle restrictions by not needing to declare isCommentNodesRequired. JavadocMethodCheck does not declare it is comments aware via said method, but it is clearly examining javadocs which are comments.