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

Issue with UnusedImports and javadoc usage. #3453

Closed
liutikas opened this Issue Sep 20, 2016 · 6 comments

Comments

Projects
None yet
7 participants
@liutikas

liutikas commented Sep 20, 2016

$ javac Test.java
[no output]

$ cat Test.java

package foo.bar;

import java.lang.Thread;

/**
 * Use {@link Thread.State} in this javadoc.
 */
public class Test {
}

$ cat config.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">
    <module name="TreeWalker">
        <module name="UnusedImports">
            <property name="processJavadoc" value="true"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-7.2-SNAPSHOT-all.jar -c config.xml Test.java

Starting audit...
[ERROR] /usr/local/google/ssd1/checkstyle/target/Test.java:3:8: Unused import - java.lang.Thread. [UnusedImports]
Audit done.
Checkstyle ends with 1 errors.

I expected no errors as the import is used by the Javadoc.


This happens on the latest master of Checkstyle as well as older versions like 6.12.1

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Sep 20, 2016

Contributor

@romani
It is a FP since javadoc links can require import and personally I usually use links to certain classes or methods in javadoc. UnusedImports is not subscribed to comment nodes and we should put off the fix for the issue due to the problem with cache in DetailAST that we discussed in email.

Contributor

MEZk commented Sep 20, 2016

@romani
It is a FP since javadoc links can require import and personally I usually use links to certain classes or methods in javadoc. UnusedImports is not subscribed to comment nodes and we should put off the fix for the issue due to the problem with cache in DetailAST that we discussed in email.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 20, 2016

Member

@MEZk , please make an issue on DetailAST cache problem

Member

romani commented Sep 20, 2016

@MEZk , please make an issue on DetailAST cache problem

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 20, 2016

Member

This issue is due to fact that this Check is using regexp parsing for javadoc, but it should use javadoc parser http://checkstyle.sourceforge.net/writingjavadocchecks.html

Member

romani commented Sep 20, 2016

This issue is due to fact that this Check is using regexp parsing for javadoc, but it should use javadoc parser http://checkstyle.sourceforge.net/writingjavadocchecks.html

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 1, 2017

Member

This Check is unfortunate implementation based on regexp parsing of javadoc
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/UnusedImportsCheck.java
attention to .... extends AbstractCheck

such implementations are marked as deprecated till we/smb convert this implementation to use AST traversing that is more reliable.
Example of AST based Check: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
attention to ..... extends AbstractJavadocCheck {

Chance that you/smb will fix this issue in the same design withtout extra regression is not that big.
I could allow this issue, but if on moment of regression we found unwanted/extra violations , I could withdraw approval and reject a fix. So try to fix only you need so much.

All are welcome to rewrite this Check from scratch to use AbstractJavadocCheck.

Member

romani commented Sep 1, 2017

This Check is unfortunate implementation based on regexp parsing of javadoc
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/UnusedImportsCheck.java
attention to .... extends AbstractCheck

such implementations are marked as deprecated till we/smb convert this implementation to use AST traversing that is more reliable.
Example of AST based Check: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/SummaryJavadocCheck.java
attention to ..... extends AbstractJavadocCheck {

Chance that you/smb will fix this issue in the same design withtout extra regression is not that big.
I could allow this issue, but if on moment of regression we found unwanted/extra violations , I could withdraw approval and reject a fix. So try to fix only you need so much.

All are welcome to rewrite this Check from scratch to use AbstractJavadocCheck.

@cushon

This comment has been minimized.

Show comment
Hide comment
@cushon

cushon Sep 1, 2017

Contributor

I think the regex-based approach isn't the issue here. It's correctly extracting e.g. Thread.State from the javadoc comment, but the check compares the references in javadoc to the simple names of imports. Extracting the left-most identifier from qualified names (e.g. Thread.State -> Thread) fixes this issue and doesn't cause any regressions, see #5046.

Contributor

cushon commented Sep 1, 2017

I think the regex-based approach isn't the issue here. It's correctly extracting e.g. Thread.State from the javadoc comment, but the check compares the references in javadoc to the simple names of imports. Extracting the left-most identifier from qualified names (e.g. Thread.State -> Thread) fixes this issue and doesn't cause any regressions, see #5046.

@romani romani added the bug label Sep 2, 2017

cushon added a commit to cushon/checkstyle that referenced this issue Sep 2, 2017

rnveach added a commit that referenced this issue Sep 2, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Sep 2, 2017

Member

Fix was merged

Member

rnveach commented Sep 2, 2017

Fix was merged

@rnveach rnveach closed this Sep 2, 2017

@rnveach rnveach added this to the 8.3 milestone Sep 2, 2017

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