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

MissingDeprecated: refactor to use javadoc AST #4983

Closed
romani opened this issue Aug 22, 2017 · 8 comments

Comments

@romani
Copy link
Member

commented Aug 22, 2017

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/MissingDeprecatedCheck.java
use old approach to parse javadoc and creates false positives on code like - https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/JavadocTokenTypes.java#L72.

    /**
     * '{@literal @}deprecated' literal in {@literal @}deprecated Javadoc tag.
     *
     * <p>Such Javadoc tag can have one argument - {@link #DESCRIPTION}</p>
     *
     * <p><b>Example:</b></p>
     * <pre>{@code @deprecated it is deprecated method}</pre>
     * <b>Tree:</b>
     * <pre>{@code
     *   |--JAVADOC_TAG[3x0] : [@deprecated it is deprecated method]
     *   |--DEPRECATED_LITERAL[3x0] : [@deprecated]
     *   |--WS[3x11] : [ ]
     *   |--DESCRIPTION[3x12] : [it is deprecated method]
     *       |--TEXT[3x12] : [it is deprecated method]
     * }</pre>
     *
     * @see
     * <a href="https://docs.oracle.com/javase/8/docs/technotes/tools/unix/javadoc.html#deprecated">
     * Oracle Docs</a>
     * @see #JAVADOC_TAG
     */
    public static final int DEPRECATED_LITERAL = JavadocParser.DEPRECATED_LITERAL;

output:

[checkstyle] Running Checkstyle 8.8-SNAPSHOT on 969 files
[checkstyle] [ERROR] .../api/JavadocTokenTypes.java:72: Duplicate @deprecated tag. [MissingDeprecated]
[checkstyle] [ERROR] .../api/JavadocTokenTypes.java:84: Must include both @java.lang.Deprecated annotation and @deprecated Javadoc tag with description. [MissingDeprecated]

TODO: re-implement Check it to use AST traverse approach, see example in JavadocSummaryCheck.

Migration Notes

skipNoJavadoc property is removed. Users should just remove it from config.

package-info.java will now be allowed to print violations. While this doesn't work for all JDK versions, it can be disabled using our filters.

Example Suppression:

    <module name="SuppressionSingleFilter">
        <property name="checks" value="MissingDeprecatedCheck"/>
        <property name="files" value="package-info\.java"/>
    </module>
@rnveach

This comment has been minimized.

@romani

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

There is protected DetailAST getBlockCommentAst it should be enough, but traversing java AST should be done manually.

@rnveach

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

@romani How will we implement skipNoJavadoc if we move to AbstractJavadocCheck?
http://checkstyle.sourceforge.net/config_annotation.html#MissingDeprecated

When this property is set to true check ignore cases when JavaDoc is missing

AbstractJavadocCheck only gets called on Javadocs so we will always be skipping methods without a Javadoc. Will we just deprecate this property?

@romani

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2017

Hmm, yes, here is nuance, when skipNoJavadoc=false and there is no javadoc comment then Check will not be called, so no validation.

If we convert it to javadoc check when we trigger validation from javadoc presence.
If we keep it Java, we trigger validation by method/class token which are always present. But search for javadoc tag could NOT be done on regexp search in plain text comment content (see description of issue)

We need to convert this Check to javadoc Check and ask users to use New MissingXXXXXJavadocCheck(s) #5411 to demand javadoc presence if deprecated annotation is present, to let MissingDeprecatedCheck to recheck that java annotation is present too.

existing Duplicate @deprecated tag validation should be removed, as it is out of scope of this check, duplication of tags should be separate Check.

@romani

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2018

@rnveach , please verify idea.
I removed out of context conversations in this issue to make issue clear.

@rnveach

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

existing Duplicate @deprecated tag validation should be removed, as it is out of scope of this check, duplication of tags should be separate Check.

IMO, this should be added to JavadocType and JavadocMethod and JavadocField.
I have already opened an issue in JavadocMethod to report duplicate @throws, See #3469 , which is very similar to this validation.

@romani

This comment has been minimized.

Copy link
Member Author

commented Jan 6, 2018

I have already opened an issue in JavadocMethod to report duplicate @throws

we should avoid supper big Checks with multiple properties, it will be very hard to support them, it is better to keep Checks simple and focused on specific bad practices. Conflicts with mutiple properties are very hard to resolve.

rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 29, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 29, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Jun 29, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 3, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 4, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 4, 2019
romani added a commit that referenced this issue Aug 5, 2019

@romani romani added this to the 8.24 milestone Aug 5, 2019

@romani romani closed this Aug 5, 2019

@romani

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

Fix is merged

@romani romani changed the title MissingDeprecatedCheck is using regexp parsing inspead of AST MissingDeprecated: refactor to use javadoc AST Aug 5, 2019

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