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

JavadocUtil doesn't recognize Javadocs in certain places #6516

Closed
rnveach opened this issue Mar 6, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@rnveach
Copy link
Member

commented Mar 6, 2019

While looking into #5411 and verifying the statement I made:

We should have a missing for every place a javadoc is valid.

I made a bare bones check to verify we recognize all valid javadoc placements.
InvalidJavadocPositionCheck
https://github.com/rnveach/checkstyle/commits/InvalidJavadocPositionCheck

    public void visitToken(DetailAST ast) {
        final String commentContent = JavadocUtil.getBlockCommentContent(ast);

        if (JavadocUtil.isJavadocComment(commentContent)) {
            if (!JavadocUtil.isCorrectJavadocPosition(ast)) {
                log(ast, "Invalid Javadoc Position");
            }
        }
    }

I expected no violations from this check on Checkstyle's own sources but I get the following:

[ERROR] \src\main\java\com\puppycrawl\tools\checkstyle\api\Configuration.java:32:5: Invalid Javadoc Position [InvalidJavadocPosition]
[ERROR] \src\main\java\com\puppycrawl\tools\checkstyle\api\Configuration.java:46:5: Invalid Javadoc Position [InvalidJavadocPosition]
[ERROR] \src\main\java\com\puppycrawl\tools\checkstyle\api\DetailNode.java:56:5: Invalid Javadoc Position [InvalidJavadocPosition]
[ERROR] \src\main\java\com\puppycrawl\tools\checkstyle\api\TextBlock.java:29:5: Invalid Javadoc Position [InvalidJavadocPosition]

It looks like currently Checkstyle doesn't work with javadocs on methods with no modifiers and returns an array.

You can also see an example with:

$ cat TestClass.java
public class TestClass {
    /** A<p>b */
    void method() {}
    /** A<p>b */
    String[] method2() { return null; }
}

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

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

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

$ java -jar checkstyle-8.18-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:2: <p> tag should be preceded with an empty line. [JavadocParagraph]
Audit done.
Checkstyle ends with 1 errors.

There should be 2 violations as both are methods with the same javadoc.

I am going to run regression to see if there are any other cases.

Let me know if we should open another issue to add this check to Checkstyle. It is pretty simple.

@rnveach

This comment has been minimized.

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 8, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 8, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 8, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 9, 2019

@romani romani added the approved label Mar 10, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 15, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 15, 2019

rnveach added a commit to rnveach/checkstyle that referenced this issue Mar 15, 2019

romani added a commit that referenced this issue Mar 16, 2019

@romani romani added the bug label Mar 16, 2019

@romani romani added this to the 8.19 milestone Mar 16, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

fix is merged.

@romani romani closed this Mar 16, 2019

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

As stated in the PR, this wasn't the end to the issue. First regression was clouded with too many results. I wanted to rerun after the first merge to see if there was anymore cases.
I will reopen issue if regression shows more.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

Regression: http://rveach.no-ip.org/checkstyle/regression/reports/236/

I reviewed almost all the violations and I didn't see anymore false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.