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

Checkstyle ignores javadoc that placed over Annotation type elements #4169

Closed
pbludov opened this Issue Apr 7, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@pbludov
Collaborator

pbludov commented Apr 7, 2017

http://checkstyle.sourceforge.net/config_javadoc.html#AtclauseOrder
http://checkstyle.sourceforge.net/config_javadoc.html#SummaryJavadoc

/var/tmp $ javac TestClass.java

/var/tmp $ cat YOUR_FILE.java
package test;

/**
 * Text
 * @version 1.0
 * @author Tester Testerson
 *
 */
public @interface TestClass
{
  /**
   * Text
   * @version 1.0
   * @author Tester Testerson
   */
  @TestClass("foo")
  String value() default "";
}

/var/tmp $ 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="SummaryJavadocCheck">
        <property name="forbiddenSummaryFragments" value="^Text*"/>
    </module>
    <module name="AtclauseOrder">
      <property name="tagOrder" value="@author, @version"/>
    </module>
  </module>
</module>

/var/tmp $ java -jar checkstyle-7.6.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] /var/tmp/TestClass.java:3: Forbidden summary fragment. [SummaryJavadoc]
[ERROR] /var/tmp/TestClass.java:6: At-clauses have to appear in the order '[@author, @version]'. [AtclauseOrder]
Audit done.
Checkstyle ends with 2 errors.

There must be 4 errors from, but Checkstyle does not process second javadoc .
I don't think AbstractJavadocCheck#isCorrectJavadocPosition supports this token.

@romani romani added approved easy medium and removed easy labels Apr 7, 2017

@romani romani changed the title from AtClauseOrder ignores annotation definitions to Checkstyle ignores javadoc that placed over Annotation type elements Apr 9, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 9, 2017

Member

after discussion with @sagar-shah94 , and @rnveach , I updated original description and title to clearly define problem.

Member

romani commented Apr 9, 2017

after discussion with @sagar-shah94 , and @rnveach , I updated original description and title to clearly define problem.

@romani romani added the javadoc label Apr 9, 2017

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Apr 11, 2017

Collaborator

There are two different issues:

  1. AtClauseOrder and the like should check all possible doc comments. There is no point in making any exceptions by default.
  2. AbstractJavadocCheck#isCorrectJavadocPosition does not process the annotation fields. This blocks the first issue.
    If someone fixes the first issue, then I can do the second issue on my own. The last one looks quite easy.
Collaborator

pbludov commented Apr 11, 2017

There are two different issues:

  1. AtClauseOrder and the like should check all possible doc comments. There is no point in making any exceptions by default.
  2. AbstractJavadocCheck#isCorrectJavadocPosition does not process the annotation fields. This blocks the first issue.
    If someone fixes the first issue, then I can do the second issue on my own. The last one looks quite easy.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 11, 2017

Member

pre-asigned to @kuri-leo .

@kuri-leo , if you agree please make a comment "I am on it"

Member

romani commented Apr 11, 2017

pre-asigned to @kuri-leo .

@kuri-leo , if you agree please make a comment "I am on it"

@kuri-leo

This comment has been minimized.

Show comment
Hide comment
@kuri-leo

kuri-leo Apr 12, 2017

Contributor

I am on it, thanks!

Contributor

kuri-leo commented Apr 12, 2017

I am on it, thanks!

@kuri-leo

This comment has been minimized.

Show comment
Hide comment
@kuri-leo

kuri-leo Apr 17, 2017

Contributor

Hi @pbludov ,
I've read the source code in past few days and now want to talk to you to make sure I have correct understading of your issue:
The output you expect is “two errors: one for outer javadoc and one for inner javadoc”, but right now there was only one.
If I misunderstood it, please tell me. then I can begin to debug.
Thanks!

Contributor

kuri-leo commented Apr 17, 2017

Hi @pbludov ,
I've read the source code in past few days and now want to talk to you to make sure I have correct understading of your issue:
The output you expect is “two errors: one for outer javadoc and one for inner javadoc”, but right now there was only one.
If I misunderstood it, please tell me. then I can begin to debug.
Thanks!

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Apr 18, 2017

Collaborator

Hi @kuri-leo !

As @rnveach noted, AbstractJavadocCheck#isCorrectJavadocPosition is too strict:

Documentation comments are recognized only when placed immediately before class, interface, constructor, method, or field declarations

But the parser produces different tags for fields and annotation fields. That is the problem. The tag ANNOTATION_FIELD_DEF is missed in BlockCommentPosition.isOnField.
Or, may be there must be BlockCommentPosition.isOnAnnotationField method and isCorrectJavadocPosition should use it.

Some other cases may be also missing. That is what should be fixed.

If I were you, I'd made a list of all tags that can have a comment. I would write a test where all these cases are. Then I corrected the code so all the tests were passing.

I'm not too familiar with this library, so I can't do it myself. I need your help.

Collaborator

pbludov commented Apr 18, 2017

Hi @kuri-leo !

As @rnveach noted, AbstractJavadocCheck#isCorrectJavadocPosition is too strict:

Documentation comments are recognized only when placed immediately before class, interface, constructor, method, or field declarations

But the parser produces different tags for fields and annotation fields. That is the problem. The tag ANNOTATION_FIELD_DEF is missed in BlockCommentPosition.isOnField.
Or, may be there must be BlockCommentPosition.isOnAnnotationField method and isCorrectJavadocPosition should use it.

Some other cases may be also missing. That is what should be fixed.

If I were you, I'd made a list of all tags that can have a comment. I would write a test where all these cases are. Then I corrected the code so all the tests were passing.

I'm not too familiar with this library, so I can't do it myself. I need your help.

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Oct 26, 2017

Collaborator

Hold on. There is an issue similar to #5226.

BlockCommentPosition.isOnPlainClassMember() fails to recognize javadoc comments on methods without modifiers but with full qualified types, like this:

interface Test {
  java.lang.String method();
}

I'll update the PR soon.

Collaborator

pbludov commented Oct 26, 2017

Hold on. There is an issue similar to #5226.

BlockCommentPosition.isOnPlainClassMember() fails to recognize javadoc comments on methods without modifiers but with full qualified types, like this:

interface Test {
  java.lang.String method();
}

I'll update the PR soon.

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Oct 26, 2017

Collaborator

That's what I'm talking about:

    |--ANNOTATION_FIELD_DEF -> ANNOTATION_FIELD_DEF [243:10]
    |   |--MODIFIERS -> MODIFIERS [243:10]
    |   |--TYPE -> TYPE [243:10]
    |   |   `--DOT -> . [243:10]
    |   |       |--DOT -> . [243:5]
    |   |       |   |--BLOCK_COMMENT_BEGIN -> /* [242:1]
    |   |       |   |   |--COMMENT_CONTENT -> *Javadoc [242:3]
    |   |       |   |   |   `--JAVADOC -> JAVADOC [242:4]
    |   |       |   |   |       |--TEXT -> Javadoc [242:4]
    |   |       |   |   |       `--EOF -> <EOF> [242:11]
    |   |       |   |   `--BLOCK_COMMENT_END -> */ [242:10]
    |   |       |   |--IDENT -> java [243:1]
    |   |       |   `--IDENT -> lang [243:6]
    |   |       `--IDENT -> String [243:11]

between the BLOCK_COMMENT_BEGIN tag and the TYPE tag may be any count (including zero) of the 'DOT' tag.

Collaborator

pbludov commented Oct 26, 2017

That's what I'm talking about:

    |--ANNOTATION_FIELD_DEF -> ANNOTATION_FIELD_DEF [243:10]
    |   |--MODIFIERS -> MODIFIERS [243:10]
    |   |--TYPE -> TYPE [243:10]
    |   |   `--DOT -> . [243:10]
    |   |       |--DOT -> . [243:5]
    |   |       |   |--BLOCK_COMMENT_BEGIN -> /* [242:1]
    |   |       |   |   |--COMMENT_CONTENT -> *Javadoc [242:3]
    |   |       |   |   |   `--JAVADOC -> JAVADOC [242:4]
    |   |       |   |   |       |--TEXT -> Javadoc [242:4]
    |   |       |   |   |       `--EOF -> <EOF> [242:11]
    |   |       |   |   `--BLOCK_COMMENT_END -> */ [242:10]
    |   |       |   |--IDENT -> java [243:1]
    |   |       |   `--IDENT -> lang [243:6]
    |   |       `--IDENT -> String [243:11]

between the BLOCK_COMMENT_BEGIN tag and the TYPE tag may be any count (including zero) of the 'DOT' tag.

romani added a commit that referenced this issue Dec 2, 2017

@romani romani added the bug label Dec 2, 2017

@romani romani added this to the 8.6 milestone Dec 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 2, 2017

Member

fix is merged.
@pbludov , thank you very much for your work!

Member

romani commented Dec 2, 2017

fix is merged.
@pbludov , thank you very much for your work!

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