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

InvalidJavadocPosition: false positive when comment is between javadoc and package #7430

Closed
rnveach opened this issue Dec 31, 2019 · 8 comments
Milestone

Comments

@rnveach
Copy link
Member

rnveach commented Dec 31, 2019

$ cat TestClass.java
/** 1. **/
/** 2. **/
// abc
package a;


$ 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="InvalidJavadocPositionCheck" />
    </module>
</module>

$ java -jar checkstyle-8.28-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:1:1: Javadoc comment is placed in the wrong location. [InvalidJavadocPosition]
[ERROR] TestClass.java:2:1: Javadoc comment is placed in the wrong location. [InvalidJavadocPosition]
Audit done.
Checkstyle ends with 2 errors.

Violation on line 1 is valid. There should be no error on line 2.

Javadoc tool seems to always choose the javadoc closest to the target. This is why 2 is valid and 1 isn't.

@rnveach
Copy link
Member Author

rnveach commented Dec 31, 2019

This is different than #43 because of the code being used in checkstyle. In the other issue we have a token and are searching for the javadoc. This code we have a javadoc and are looking for the target to see if it is in the correct position.

@romani
Copy link
Member

romani commented Jan 2, 2020

@rnveach , why you think we should report violation on 1st comment?

There are cases then user use license header in javadoc format (not very good, but not that much people care about difference between multiline comment and javadoc comment), and here is just another example - http://rveach.no-ip.org/checkstyle/regression/reports/285/pmd/index.html#A1
I remember there were other cases when user complained about this.

It is kind hard decision when we should do same as javadoc tool and when behave differently.

@rnveach
Copy link
Member Author

rnveach commented Jan 3, 2020

@romani As stated above, first comment is not a javadoc and will not be sent to the html report in the javadoc tool. I confirmed this locally before issue. If users want this in the javadoc they should merge the 2.

@romani
Copy link
Member

romani commented Jan 3, 2020

problem is when user do not want to merge comments: first comment is license header, second is javadoc.
as we have in PMD case

/**
   * BSD-style license; for more info see http://pmd.sourceforge.net/license.html
  */
/**
   * Here is some documentation details .....  
  */
// stolen from XPath Explorer (http://www.xpathexplorer.com)
package net.sourceforge.pmd.cpd;

@rnveach
Copy link
Member Author

rnveach commented Jan 3, 2020

It doesn't change that it isn't a valid javadoc. It will not go to html report regardless of what user wants. Purpose of making it a javadoc is so it is treated as one but no tool will do that in this scenario. Tool will only pick the javadoc closest to the target. All others are ignored like regular comments. If this one file is issue for user then they should suppress it.

I remember there were other cases when user complained about this.

I don't know what you are referring to. No one has opened any issues to check's logic of identifying javadocs.
https://github.com/checkstyle/checkstyle/issues?utf8=%E2%9C%93&q=is%3Aissue+InvalidJavadocPosition

@romani
Copy link
Member

romani commented Jan 4, 2020

I also can not find .... ok lets make it our Check stricter than tool.

@romani romani added the approved label Jan 4, 2020
@rnveach
Copy link
Member Author

rnveach commented Jan 4, 2020

ok lets make it our Check stricter than tool.

Its not stricter. Tool will not see 1 as a javadoc and will only use 2.

@romani romani added the bug label Jan 4, 2020
@romani romani added this to the 8.29 milestone Jan 4, 2020
@romani
Copy link
Member

romani commented Jan 4, 2020

fix is merged.

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

No branches or pull requests

2 participants