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

UnusedImports processJavadoc fails with javadoc tags that span lines #2840

Closed
JensenPaul opened this Issue Jan 22, 2016 · 5 comments

Comments

Projects
None yet
6 participants
@JensenPaul

JensenPaul commented Jan 22, 2016

If you remove the line-break in the @link javadoc tag, the check will pass, otherwise it fails:

http://checkstyle.sourceforge.net/config_imports.html#UnusedImports

/var/tmp $ cat Foo.java 
import android.net.HttpResponseCache;
/**
 * {@link HttpResponseCache#install
 *        HttpResponseCache.install}
 */
class Foo {}

/var/tmp $ cat style.xml 
<?xml version="1.0" encoding="UTF-8"?>
<!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>

/var/tmp $ java -jar checkstyle-6.14.1-all.jar -c style.xml Foo.java 
Starting audit...
[ERROR] /var/tmp/Foo.java:1:8: Unused import: android.net.HttpResponseCache. Use :JavaImportOrganize (ECLIM) or Ctrl+Shift+O (Eclipse) to sort imports [UnusedImports]
Audit done.
Checkstyle ends with 1 errors.

@romani romani added javadoc approved and removed javadoc labels Jan 26, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 27, 2016

Member

parsing of javadoc is done there not by JavadocParser but by RegExp that is why chance for false positive is high. Javadoc is mixture of HTML + JavadocTags. Parsing by RegExp will never be good.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/UnusedImportsCheck.java#L236

We already did Grammar based parser, example:
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java#L236
such approach should be used.

Member

romani commented Jan 27, 2016

parsing of javadoc is done there not by JavadocParser but by RegExp that is why chance for false positive is high. Javadoc is mixture of HTML + JavadocTags. Parsing by RegExp will never be good.
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/UnusedImportsCheck.java#L236

We already did Grammar based parser, example:
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AbstractJavadocCheck.java#L236
such approach should be used.

@romani romani added the javadoc label Jan 27, 2016

@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Feb 28, 2017

Collaborator

As an easy and erroneous solution, you can fix the regular expression in
src/main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java:78 as follows:

    /** Inline tag pattern. */
    private static final Pattern INLINE_TAG_PATTERN = Pattern.compile(
        ".*?\\{@(\\p{Alpha}+)\\s+([\\}]*?)\\}");
Collaborator

pbludov commented Feb 28, 2017

As an easy and erroneous solution, you can fix the regular expression in
src/main/java/com/puppycrawl/tools/checkstyle/utils/JavadocUtils.java:78 as follows:

    /** Inline tag pattern. */
    private static final Pattern INLINE_TAG_PATTERN = Pattern.compile(
        ".*?\\{@(\\p{Alpha}+)\\s+([\\}]*?)\\}");
@pbludov

This comment has been minimized.

Show comment
Hide comment
@pbludov

pbludov Feb 28, 2017

Collaborator

Note that AbstractJavadocCheck won't fit in that case.
For now, it is not possible to code a "dual" check, for both java and javadoc.
It is possible to hack the system by a check with an inner check within.
Here we are:

////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2016 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
////////////////////////////////////////////////////////////////////////////////

package com.puppycrawl.tools.checkstyle.checks.imports;

import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.DetailNode;
import com.puppycrawl.tools.checkstyle.api.FullIdent;
import com.puppycrawl.tools.checkstyle.api.JavadocTokenTypes;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck;
import com.puppycrawl.tools.checkstyle.utils.CommonUtils;

import java.util.HashSet;
import java.util.Set;

/**
 * <p>
 * Checks for unused import statements.
 * </p>
 *  <p>
 * An example of how to configure the check is:
 * </p>
 * <pre>
 * &lt;module name="UnusedImports"/&gt;
 * </pre>
 * Compatible with Java 1.5 source.
 *
 * @author Oliver Burn
 * @author Pavel Bludov
 */
public class UnusedImportsCheck extends AbstractCheck {

    /**
     * A key is pointing to the warning message text in "messages.properties"
     * file.
     */
    public static final String MSG_KEY = "import.unused";

    /** Suffix for the star import. */
    private static final String STAR_IMPORT_SUFFIX = ".*";

    /** Set of the imports. */
    private final Set<FullIdent> imports = new HashSet<>();

    /** Set of references - possibly to imports or other things. */
    private final Set<String> referenced = new HashSet<>();

    /** Flag to indicate when time to start collecting references. */
    private boolean collect;
    /** Inner check to process Javadoc comments. */
    private JavadocCheck javadocCheck;
    /**
     * Sets whether to process JavaDoc or not.
     *
     * @param value Flag for processing JavaDoc.
     */
    public void setProcessJavadoc(boolean value) {
        javadocCheck = value ? new JavadocCheck() : null;
    }

    @Override
    public void init()
    {
        if (javadocCheck != null) {
            javadocCheck.init();
        }
    }

    @Override
    public void beginTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.setClassLoader(getClassLoader());
            javadocCheck.setFileContents(getFileContents());
            javadocCheck.beginTree(rootAST);
        }
        collect = false;
        imports.clear();
        referenced.clear();
    }

    @Override
    public void finishTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.finishTree(rootAST);
        }
        // loop over all the imports to see if referenced.
        imports.stream()
            .filter(imp -> !referenced.contains(CommonUtils.baseClassName(imp.getText())))
            .forEach(imp -> log(imp.getLineNo(),
                imp.getColumnNo(),
                MSG_KEY, imp.getText()));
    }


    @Override
    public int[] getDefaultTokens() {
        return new int[] {
            TokenTypes.IDENT,
            TokenTypes.IMPORT,
            TokenTypes.STATIC_IMPORT,
            TokenTypes.BLOCK_COMMENT_BEGIN,
        };
    }

    @Override
    public int[] getRequiredTokens() {
        return getDefaultTokens();
    }

    @Override
    public int[] getAcceptableTokens() {
        return new int[] {
            TokenTypes.IDENT,
            TokenTypes.IMPORT,
            TokenTypes.STATIC_IMPORT,
            TokenTypes.BLOCK_COMMENT_BEGIN,
        };
    }

    @Override
    public boolean isCommentNodesRequired() {
        return javadocCheck != null;
    }

    @Override
    public void visitToken(DetailAST ast) {
        if (ast.getType() == TokenTypes.IDENT) {
            if (collect) {
                processIdent(ast);
            }
        }
        else if (ast.getType() == TokenTypes.IMPORT) {
            processImport(ast);
        }
        else if (ast.getType() == TokenTypes.STATIC_IMPORT) {
            processStaticImport(ast);
        }
        else {
            collect = true;
            if (javadocCheck != null) {
                javadocCheck.visitToken(ast);
            }
        }
    }

    /**
     * Collects references made by IDENT.
     * @param ast the IDENT node to process
     */
    private void processIdent(DetailAST ast) {
        final DetailAST parent = ast.getParent();
        final int parentType = parent.getType();
        if (parentType != TokenTypes.DOT
            && parentType != TokenTypes.METHOD_DEF
            || parentType == TokenTypes.DOT
            && ast.getNextSibling() != null) {
                referenced.add(ast.getText());
        }
    }

    /**
     * Collects the details of imports.
     * @param ast node containing the import details
     */
    private void processImport(DetailAST ast) {
        final FullIdent name = FullIdent.createFullIdentBelow(ast);
        if (!name.getText().endsWith(STAR_IMPORT_SUFFIX)) {
            imports.add(name);
        }
    }

    /**
     * Collects the details of static imports.
     * @param ast node containing the static import details
     */
    private void processStaticImport(DetailAST ast) {
        final FullIdent name =
        FullIdent.createFullIdent(
        ast.getFirstChild().getNextSibling());
        if (!name.getText().endsWith(STAR_IMPORT_SUFFIX)) {
            imports.add(name);
        }
    }

    /** Ugly hack to get access to Javadoc comments. **/
    private class JavadocCheck extends AbstractJavadocCheck
    {
        @Override
        public int[] getDefaultJavadocTokens() {
            return new int[] {
                JavadocTokenTypes.CLASS,
                JavadocTokenTypes.CLASS_NAME,
            };
        }

        @Override
        public void visitJavadocToken(DetailNode ast) {
            referenced.add(ast.getText());
        }
    }
}

The problematic part is

    public void beginTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.setClassLoader(getClassLoader());
            javadocCheck.setFileContents(getFileContents());
            javadocCheck.beginTree(rootAST);
        }
    }

For the today, it works fine. Will it work in the future - who knows?

Collaborator

pbludov commented Feb 28, 2017

Note that AbstractJavadocCheck won't fit in that case.
For now, it is not possible to code a "dual" check, for both java and javadoc.
It is possible to hack the system by a check with an inner check within.
Here we are:

////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2016 the original author or authors.
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
////////////////////////////////////////////////////////////////////////////////

package com.puppycrawl.tools.checkstyle.checks.imports;

import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.DetailNode;
import com.puppycrawl.tools.checkstyle.api.FullIdent;
import com.puppycrawl.tools.checkstyle.api.JavadocTokenTypes;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
import com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck;
import com.puppycrawl.tools.checkstyle.utils.CommonUtils;

import java.util.HashSet;
import java.util.Set;

/**
 * <p>
 * Checks for unused import statements.
 * </p>
 *  <p>
 * An example of how to configure the check is:
 * </p>
 * <pre>
 * &lt;module name="UnusedImports"/&gt;
 * </pre>
 * Compatible with Java 1.5 source.
 *
 * @author Oliver Burn
 * @author Pavel Bludov
 */
public class UnusedImportsCheck extends AbstractCheck {

    /**
     * A key is pointing to the warning message text in "messages.properties"
     * file.
     */
    public static final String MSG_KEY = "import.unused";

    /** Suffix for the star import. */
    private static final String STAR_IMPORT_SUFFIX = ".*";

    /** Set of the imports. */
    private final Set<FullIdent> imports = new HashSet<>();

    /** Set of references - possibly to imports or other things. */
    private final Set<String> referenced = new HashSet<>();

    /** Flag to indicate when time to start collecting references. */
    private boolean collect;
    /** Inner check to process Javadoc comments. */
    private JavadocCheck javadocCheck;
    /**
     * Sets whether to process JavaDoc or not.
     *
     * @param value Flag for processing JavaDoc.
     */
    public void setProcessJavadoc(boolean value) {
        javadocCheck = value ? new JavadocCheck() : null;
    }

    @Override
    public void init()
    {
        if (javadocCheck != null) {
            javadocCheck.init();
        }
    }

    @Override
    public void beginTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.setClassLoader(getClassLoader());
            javadocCheck.setFileContents(getFileContents());
            javadocCheck.beginTree(rootAST);
        }
        collect = false;
        imports.clear();
        referenced.clear();
    }

    @Override
    public void finishTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.finishTree(rootAST);
        }
        // loop over all the imports to see if referenced.
        imports.stream()
            .filter(imp -> !referenced.contains(CommonUtils.baseClassName(imp.getText())))
            .forEach(imp -> log(imp.getLineNo(),
                imp.getColumnNo(),
                MSG_KEY, imp.getText()));
    }


    @Override
    public int[] getDefaultTokens() {
        return new int[] {
            TokenTypes.IDENT,
            TokenTypes.IMPORT,
            TokenTypes.STATIC_IMPORT,
            TokenTypes.BLOCK_COMMENT_BEGIN,
        };
    }

    @Override
    public int[] getRequiredTokens() {
        return getDefaultTokens();
    }

    @Override
    public int[] getAcceptableTokens() {
        return new int[] {
            TokenTypes.IDENT,
            TokenTypes.IMPORT,
            TokenTypes.STATIC_IMPORT,
            TokenTypes.BLOCK_COMMENT_BEGIN,
        };
    }

    @Override
    public boolean isCommentNodesRequired() {
        return javadocCheck != null;
    }

    @Override
    public void visitToken(DetailAST ast) {
        if (ast.getType() == TokenTypes.IDENT) {
            if (collect) {
                processIdent(ast);
            }
        }
        else if (ast.getType() == TokenTypes.IMPORT) {
            processImport(ast);
        }
        else if (ast.getType() == TokenTypes.STATIC_IMPORT) {
            processStaticImport(ast);
        }
        else {
            collect = true;
            if (javadocCheck != null) {
                javadocCheck.visitToken(ast);
            }
        }
    }

    /**
     * Collects references made by IDENT.
     * @param ast the IDENT node to process
     */
    private void processIdent(DetailAST ast) {
        final DetailAST parent = ast.getParent();
        final int parentType = parent.getType();
        if (parentType != TokenTypes.DOT
            && parentType != TokenTypes.METHOD_DEF
            || parentType == TokenTypes.DOT
            && ast.getNextSibling() != null) {
                referenced.add(ast.getText());
        }
    }

    /**
     * Collects the details of imports.
     * @param ast node containing the import details
     */
    private void processImport(DetailAST ast) {
        final FullIdent name = FullIdent.createFullIdentBelow(ast);
        if (!name.getText().endsWith(STAR_IMPORT_SUFFIX)) {
            imports.add(name);
        }
    }

    /**
     * Collects the details of static imports.
     * @param ast node containing the static import details
     */
    private void processStaticImport(DetailAST ast) {
        final FullIdent name =
        FullIdent.createFullIdent(
        ast.getFirstChild().getNextSibling());
        if (!name.getText().endsWith(STAR_IMPORT_SUFFIX)) {
            imports.add(name);
        }
    }

    /** Ugly hack to get access to Javadoc comments. **/
    private class JavadocCheck extends AbstractJavadocCheck
    {
        @Override
        public int[] getDefaultJavadocTokens() {
            return new int[] {
                JavadocTokenTypes.CLASS,
                JavadocTokenTypes.CLASS_NAME,
            };
        }

        @Override
        public void visitJavadocToken(DetailNode ast) {
            referenced.add(ast.getText());
        }
    }
}

The problematic part is

    public void beginTree(DetailAST rootAST) {
        if (javadocCheck != null) {
            javadocCheck.setClassLoader(getClassLoader());
            javadocCheck.setFileContents(getFileContents());
            javadocCheck.beginTree(rootAST);
        }
    }

For the today, it works fine. Will it work in the future - who knows?

nanaze added a commit to nanaze/checkstyle that referenced this issue May 3, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

nanaze added a commit to nanaze/checkstyle that referenced this issue May 4, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

nanaze added a commit to nanaze/checkstyle that referenced this issue May 5, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

nanaze added a commit to nanaze/checkstyle that referenced this issue May 5, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

nanaze added a commit to nanaze/checkstyle that referenced this issue May 5, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

nanaze added a commit to nanaze/checkstyle that referenced this issue May 8, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

@Vladlis Vladlis added GSoC2017 and removed GSoC2017 labels May 10, 2017

@Vladlis Vladlis assigned ps-sp and unassigned ps-sp May 22, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jun 27, 2017

Member

Last commit is referenced to almost complete implementation and could be continued by any person, confirmation from author is comment.

Member

romani commented Jun 27, 2017

Last commit is referenced to almost complete implementation and could be continued by any person, confirmation from author is comment.

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 8, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers and fix the inline tag parser to properly parse inline tags that span multiple lines. Put tests on both

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

Issue checkstyle#2840: Extract implementations of the block and inlin…
…e parsers to fix the inline tag parser to properly parse inline tags that span multiple lines.

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

Issue checkstyle#2840: Fix the javadoc inline tag parser to properly …
…parse inline tags that span multiple lines for UnusedImportsCheck

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

silentj pushed a commit to silentj/checkstyle that referenced this issue Jul 10, 2017

rnveach added a commit that referenced this issue Jul 11, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jul 11, 2017

Member

Fix was merged

Member

rnveach commented Jul 11, 2017

Fix was merged

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