Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#156 Check suppression with @SuppressWarnings

Unstable_(example)
closed
Oliver Burn
None
5
2014-07-15
2008-11-28
Trevor Robinson
No

Checkstyle warning suppression "the Java 5 way". :-)

Some examples from the unit test:

@SuppressWarnings("parameternumber")
public void needsLotsOfParameters(int a, int b, int c, int d, int e, int f,
    int g, int h)
{
}

@SuppressWarnings("illegalcatch")
public void needsToCatchException()
{
    try {
    } catch (Exception ex) {
        // should NOT fail IllegalCatchCheck
    }
}

Addresses these feature requests:

[ 1306338 ] Turn off checks with SuppressWarnings?
http://sourceforge.net/tracker/index.php?func=detail&aid=1306338&group_id=29721&atid=397081
[ 1841127 ] filter annotation
https://sourceforge.net/tracker/?func=detail&atid=397081&aid=1841127&group_id=29721

Functional design notes: I decided to use "javac -Xlint"-style warning names (i.e. lower-case, no punctuation) rather than using a prefix (e.g. "checkstyle:parameternumber") or Checkstyle class names (e.g. "ParameterNumberCheck") in the hopes of moving toward a standard set of names across different tools. For the same reason, I included no provision for wildcards, regex, abbreviations, or user-configurable aliases.

Technical design notes: Since filters do not have access to the AST, I followed the SuppressionCommentFilter pattern of using a helper Check, SuppressWarningsHolder, to store the necessary AST-derived information for the last file processed in a thread-local variable. However, since the necessary information is more specific and complex than the comments gathered by FileContentsHolder, SuppressWarningsHolder goes the extra step of building a list of suppression regions itself. This allows it to completely encapsulate the suppression checking, making SuppressWarningsFilter a trivial wrapper.

Discussion

1 2 3 > >> (Page 1 of 3)
  • Oliver Burn
    Oliver Burn
    2008-11-29

    Thanks for the patch Trevor. Although I personally like to keep suppressions out of the code base, I can understand why people will want them.

    I have been reading http://java.sun.com/j2se/1.5.0/docs/api/java/lang/SuppressWarnings.html and do not see any real guidelines for naming conventions. Would it not be simpler to a convention that uses a prefix and the Check name. E.g. @SuppressWarnings("checkstyle:ParameterNumberCheck,checkstyle:DeclarationOrder").

    The reason for the prefix "checkstyle:" is prevent naming collisions. Also a flag for Checkstyle that value is intended for it.

    The reason for the check classname is that it makes it very simple configure for users.

    If there was momentum for the feature, we could move to using "javac -Xlint"-style warning
    name.

    Thoughts?

     
  • I understand that suppressions are a controversial issue. We've recently been having a very long, active email thread about whether to use them at my company. While clearly they can be misused to subvert what Checkstyle is trying to help accomplish (consistent, readable, correct code), they are useful when a check is for a guideline that has rare but reasonable exceptions, such as "almost never 'catch (Exception)'". You want to leave the check generally enabled, but don't want the distraction of a constant warning in those exceptional places. Some people advocate instead that Checkstyle only be run against diffs, but the concern is that this is a form of invisible suppression. Some teams would rather have any exceptions to guidelines (that would cause a Checkstyle warning) clearly called out (which the suppression accomplishes) and explained in the code. To verify that this is done, Checkstyle should run cleanly against the entire file (not just the diff against the previous revision). This also provides a reasonable path to benefit from enabling new checks. Anyway, I think we've come close to reaching a consensus that suppressions are acceptable if done in a standardized and highly constrained fashion, namely @SuppressWarnings. It's up to code reviewers to ensure they're used only when truly necessary.

    You're right that there aren't any real naming guidelines. Even the Sun bug to implement @SuppressWarnings support in javac (http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4986256) says only this:

    The set of names you can use with @SuppressWarnings for javac can be loosely inferred by going "javac -X" and looking at the spec of -Xlint.
    Any -Xlint option that does not begin with "-", and excluding "all" and "path", can be used with SuppressWarnings. E.g.
    @SuppressWarnings("deprecation")
    @SuppressWarnings("unchecked")
    etc.

    However, assuming that checks are appropriately named, a naming collision shouldn't generally be a problem. For example, if both javac and Checkstyle both have a switch case-fallthrough warning suppressed with "fallthrough" (which based on this patch they would), you actually want the suppression to apply to both tools.

    Is there a reason you think that Checkstyle needs to know that a suppression is intended for it? I'd think that a good check would be for Checkstyle recognize every suppression used (even ones it doesn't implement) to ensure they are not misspelled and are actually necessary. (Is there a way to do the latter in Checkstyle? It's essentially a filter generating its own errors.) Looked at this way, every value is intended for Checkstyle.

    On the other hand, having a prefix makes it easier for other tools (such as compilers and IDEs) to recognize Checkstyle check names and avoid issuing their own warnings for unknown suppressions. However, I don't think any tools currently implement such a prefix filter.

    The reason I wanted to avoid waiting for momentum before using "-Xlint"-style names is that it creates a proliferation of names for the same warnings. Ideally, teams and companies could share code using a single consistent set of suppression names so that they won't have to continually add suppression aliases to their Checkstyle configurations.

    The check names that I'm currently using are simply the class name, minus the "Check" suffix, converted to lower case (Eclipse at least is case-sensitive about suppression names). I put them in a table in case anyone thought some should be renamed. If this isn't the case, we could just programmatically generate such names. I just didn't know if it would be acceptable to constrain the names of classes and suppressions to match. As long as you're okay with it though, I'm fine with losing the map, or perhaps using it only for exceptional cases. That would simplify the case of adding a new Check that isn't aware of this filter.

    Anyway, those are my thoughts. You've had much more experience in maintaining this project, so you probably know better. If in doubt, you may want to open the discussion to the mailing list. The thread on this patch is only noticeable to people that have actively chosen to monitor it, right?

    Thanks for Checkstyle and for taking the time to review this patch,
    Trevor

     
  • I've updated the patch file to automatically generate suppression names for Check classes: "com.foo.SuperCheck" -> @SuppressWarnings("super"). The alias hash table is now only used to override the default name. I've also added the ability to set aliases via the configuration properties: <property name="aliasList" value="com.foo.SuperCheck=oldsuper,com.foo.SuperCheck2=newsuper"/>. Also, "checkstyle:" can be used as a prefix to limit suppression to checkstyle only: @SuppressWarnings("checkstyle:fallthrough"). Finally, I fixed some omissions (support for ctor and fully qualified annotations) and improved test coverage (now 100% except for invalid AST and package annotations, which aren't very useful anyway in checkstyle, because they can only be in the package-info.java file).

     
  • Any thoughts on the updated patch?

     
  • Ulli Hafner
    Ulli Hafner
    2009-01-14

    I can't wait for this patch to be integrated! Thanks for the patch Trevor.

    BTW: PMD uses the style @SuppressWarnings("PMD.RuleClassName") and findbugs @SuppressWarnings("ID") where ID is a short identifier of the rule. So it will be hard to have a consistent scheme.

    Typically the name of the suppression shouldn't matter much since the corresponding IDE plug-in could generate them on the fly (e.g., as a Eclipse quick fix).

     
  • I haven't put a lot of time evaluating your check (it looks well thought out though based on this thread -- kudos) but I wanted to give my 2 cents on something Oliver said. I have been thinking about submitting a checkstyle check to control the usage @SuppressWarnings. My idea was to have it highly configurable for example:

    Only allow the "unchecked" warning on variable definitions but not method definitions except if in a class called com.foo.Violater.

    This way checkstyle could help ensure that certain warnings never get suppressed or only get suppressed in a certain way. I believe that my check would fit nicely with this check. I do agree with Oliver that the warning names should be prefixed (e.g. checkstyle:ParameterNumberCheck). This way in my proposed check could use regex for acting on just checkstyle related suppressions or at the very least be able know with certainty that a suppression is for checkstyle. Anyway, it looks like you have that under control but I just wanted to give you my thoughts.

    Thanks,

    Travis

     
  • Oliver Burn
    Oliver Burn
    2009-03-08

    OK - I would like to apply this patch since there is clearly demand for it. Can you please apply a patch to provide documentation. Cheers.

     
  • Rasmus Kaj
    Rasmus Kaj
    2009-07-24

    I for one would very much like to see this patch (or something like it) committed.

    Is there currently any reason not to commit the latest version of the patch by trevor_robinson?

     
  • Oliver Burn
    Oliver Burn
    2009-07-30

    Yes, the current patch is not applied due to lack of documentation. A patch should include code changes, unit tests AND documentation. Without all three things, a patch is much less likely to be applied as it means somebody else needs to write the missing pieces.

     
1 2 3 > >> (Page 1 of 3)