JavadocType: False negative for unknown tag in the format abc.xyz

[sktpy]

(Documentation)

Discovered this while working on the fix for #14573

Example of such usages in real-code can be found here.

Test.java 🐜

/**
 * @ant.task category="test"
 * @abc.xyz
 */
public class Test {}

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

attention that allowUnknownTags is false by default.

Current CLI:

java -jar checkstyle-10.14.0-all.jar -c config.xml Test.java
Starting audit...
Audit done.

Expected:

Starting audit...
[ERROR] Test.java:2:4: Unknown tag 'ant.task'. [JavadocType]
[ERROR] Test.java:3:4: Unknown tag 'abc.xyz'. [JavadocType]
Audit done.
Checkstyle ends with 2 errors.

Attention note from maintainers:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocTypeCheck.java

Lines 149 to 150 in 72cf172

	public class JavadocTypeCheck
	extends AbstractCheck {

this Check is not AST based, so fix might be not trivial. We can accept fix only if it is simple and does not cause more regressions or false positives. Ideally we should reimplement this Check to be ast based, example:

checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/checks/javadoc/AtclauseOrderCheck.java

Line 110 in 72cf172

public class AtclauseOrderCheck extends AbstractJavadocCheck {

[nrmancuso]

Here is my thoughts on this: yes, this should be fixed. However, as you have demonstrated above, there may be commonly used "unknown tags" that we aren't placing violations on now, and this check does not seem to allow us to specify tags that should be considered "known"; so when we would release this change, it could be painful for users.

Imagine you are Apache, and have something set up to generate documentation based off of the ant.task tags above. You update your checkstyle version, suddenly have hundreds of new violations, and are faced with the decision of either changing your documentation generation (not likely) or not updating checkstyle (or removing this check from your config). This is not a good situation.

Before proceeding here, please do this:

• create a test input with a few methods that have currently supported "unknown tags", like unknown1 and unknown2
• create a suppression for unknown1 violations only (we still allow violations to be placed on unknown2)
• run checkstyle over this input with the new configuration

When users visit this issue with a bunch of new violations, this will allow them to easily suppress them in their project. Also, this exercise will demonstrate that we CAN successfully suppress these violations. Sadly, we cannot use XPath for this.

I will approve this issue once we can prove that suppression model is effective.

[sktpy]

Before proceeding, how do we feel about adding a new property allowedUnkownTags?
It wouldn't complicate the check too much from the implementation point of view, but would be very user friendly, as this appears to be a popular case where documentation generation is based off the custom tags.

Because I feel like for those cases, our users are probably setting allowUnknownTags property as true,
which just puts a blanket over all the unknown tags, increasing the chances of unintended side effects.

And due to that, it is very likely that our users won't be getting any new violations, as currently supported unknown tags that are not in format abc.xyz are also likely to be used in such cases, forcing them to set allowUnknownTags as true, or as you have mentioned, removing the check altogether.

[nrmancuso]

Before proceeding, how do we feel about adding a new property allowedUnkownTags?

This is a logical proposal, anyone is welcome to open issues for further discussion :)

In my limited experience, I find it is easier to make incremental changes to deliver features/bug fixes to users faster and get actual feedback from a few users. If we have some reliable suppression and this is enough, then it is enough :) So, we deliver this bug fix with a suppression that can be extended to deliver functionality like allowedUnkownTags would, then if users request a new property to make this easier, then we do that.

Otherwise, we end up with checks with a lot of properties; extra properties typically increase the potential for bugs and maintenance overhead. Also, we want to avoid falling into the rabbit hole of perfecting things; nothing is ever perfect.

[sktpy]

In response to #14618 (comment).

Note: Click on "▶Heading" to expand its contents.

Test.java

/**
 * @ignored ok
 * @checked violation
 */
public class Test {
  /**
   * @ignored ok
   * @checked violation
   */
  class Test2 {

    /**
     * @WignoredW violation
     * @ignored ok
     * @author ok
     */
    static class Test3 {
      static class Test4 {

        /**
         * @ignore violation
         * @ignored ok
         */
        static class Test5 {

          /**
           * @gnored violation
           * @ignored ok
           */
          enum Test6 {

          }
        }

        record Test7() {
          /**
           * @ignOred violation
           * @ignored ok
           */
          record Test8() {

          }
        }
      }

      /**
       * @checked violation
       * @ignored ok
       */
      interface Test9 {

      }
    }

    /**
     *
     * @IGNORED violation
     * @ignored ok
     */
    @interface Test10 {}

    /**
     *
     * @param ignored
     */
    record Test11() {}
  }
}

---

Without Suppression:

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

CLI

java -jar checkstyle-10.14.0-all.jar -c config.xml Test.java
Starting audit...
[ERROR] Test.java:2:4: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:3:4: Unknown tag 'checked'. [JavadocType]
[ERROR] Test.java:7:8: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:8:8: Unknown tag 'checked'. [JavadocType]
[ERROR] Test.java:13:12: Unknown tag 'WignoredW'. [JavadocType]
[ERROR] Test.java:14:12: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:21:20: Unknown tag 'ignore'. [JavadocType]
[ERROR] Test.java:22:20: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:27:24: Unknown tag 'gnored'. [JavadocType]
[ERROR] Test.java:28:24: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:37:24: Unknown tag 'ignOred'. [JavadocType]
[ERROR] Test.java:38:24: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:47:16: Unknown tag 'checked'. [JavadocType]
[ERROR] Test.java:48:16: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:57:12: Unknown tag 'IGNORED'. [JavadocType]
[ERROR] Test.java:58:12: Unknown tag 'ignored'. [JavadocType]
[ERROR] Test.java:64:12: Unused @param tag for 'ignored'. [JavadocType]
Audit done.
Checkstyle ends with 17 errors.

---

Suppressing violation for unknown tag ignored:

• Here we have suppressed unknown tag violation for the tag which is exactly named ignored.

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="JavadocType"/>
  </module>
  <module name="SuppressionSingleFilter">
    <property name="checks" value="JavadocType"/>
    <property name="message" value="Unknown tag 'ignored'"/>
  </module>
</module>

CLI

java -jar checkstyle-10.14.0-all.jar -c config.xml Test.java
Starting audit...
Test.java:3:4: Unknown tag 'checked'. [JavadocType]
Test.java:8:8: Unknown tag 'checked'. [JavadocType]
Test.java:13:12: Unknown tag 'WignoredW'. [JavadocType]
Test.java:21:20: Unknown tag 'ignore'. [JavadocType]
Test.java:27:24: Unknown tag 'gnored'. [JavadocType]
Test.java:37:24: Unknown tag 'ignOred'. [JavadocType]
Test.java:47:16: Unknown tag 'checked'. [JavadocType]
Test.java:57:12: Unknown tag 'IGNORED'. [JavadocType]
Test.java:64:12: Unused @param tag for 'ignored'. [JavadocType]
Audit done.
Checkstyle ends with 9 errors.

---

Result: We are successfully able to suppress violation for the specified unknown tag via SuppressionSingleFilter.

---

• Have sprinkled in some variations of the unknown tag (eg case-mismatch, content-mismatch) we want to ignore in various applicable places, to verify that it is suppressing exactly the specified tag in various places.
• Have placed input(at last) where the name of the unknown tag we want to suppress matches exactly with what would appear in the violation that we do not want to suppress here (missing param in this case), to show that it won't conflict with other violations of the Check reported for the exact name.
• Unknown tag 'tagName' only appears in the violation message for javadoc.unknownTag which is only used by JavadocType check, so technically we can also remove line <property name="checks" value="JavadocType"/> from the suppression but I haven't given it a deep thought yet.

If a concise/extended version of this will be more preferred, let me know.

Also, I wasn't familiar with suppressions before I worked on this so my knowledge regarding this is very limited as of now,
so a better solution can exist, or it may be the case that I have messed something in this.

[nrmancuso]

I am good to approve, @romani please take a look and add approved label if you are good.

[romani]

This Check is not AST based, if you start heavily test it you would never stop opening new issues and fixing them.

All not AST based Checks are kind of deprecated, we should reimplement them first.

We allow some updates to such Checks, that are easy and annoying users.

This issue doesn't looks like bothering anyone. It is valid issue but I don't think we need fix it before refactoring/redesign.

Redesign might also required, as this Checks too much validation, it is already proven to be error prone.

[romani]

I am approving this issue, I placed "Attention note from maintainers:" in issue description.