[ checkstyle-Patches-2356719 ] Check suppression with @SuppressWarnings

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[ checkstyle-Patches-2356719 ] Check suppression with @SuppressWarnings

SourceForge.net
Patches item #2356719, was opened at 2008-11-28 12:26
Message generated for change (Comment added) made by
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=397080&aid=2356719&group_id=29721

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Trevor Robinson (trevor_robinson)
Assigned to: Nobody/Anonymous (nobody)
Summary: Check suppression with @SuppressWarnings

Initial Comment:
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.


----------------------------------------------------------------------

Comment By: https://www.google.com/accounts ()
Date: 2012-04-26 04:47

Message:
Seems this is really old thread, but I agree that suppression via
annotation would be cool.

As for me prefix "checkstyle:" is ok. But yes, there is no common
convention.
For example PMD uses prefix "PMD." -
@SuppressWarnings("PMD.SignatureDeclareThrowsException")

On the other hand findbugs uses their own annotation
@edu.umd.cs.findbugs.annotations.SuppressWarnings (and SuppressFBWarnings)
to not to mix it with standard one...

Anyway any support of annotation-based warning suppression would be
awesome!

----------------------------------------------------------------------

Comment By: Nobody/Anonymous (nobody)
Date: 2011-09-09 11:46

Message:
CMxm2I  <a href="http://kskihigginwq.com/">kskihigginwq</a>,
[url=http://oatjxgazfbjj.com/]oatjxgazfbjj[/url],
[link=http://orqlkyziajga.com/]orqlkyziajga[/link],
http://oktxhbhlxbsx.com/

----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2010-12-16 14:22

Message:
I have opened the ticket again so that patches can be uploaded.

Michal-mech: the documentation in src/xdocs needs to be updated to fully
describe the functionality that is being provided with the patch. Without
this documentation, nobody will ever know about the new functionality and
how to use it.

----------------------------------------------------------------------

Comment By: MichaƂ Mech (michal-mech)
Date: 2010-12-02 03:34

Message:
I fixed patch and updated to current version but I don't know how to upload
it.
@oburn describe please what kind of documentation are You expected. I will
try to write it.

----------------------------------------------------------------------

Comment By: SourceForge Robot (sf-robot)
Date: 2010-12-01 20:20

Message:
This Tracker item was closed automatically by the system. It was
previously set to a Pending status, and the original submitter
did not respond within 14 days (the time period specified by
the administrator of this Tracker).

----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2010-11-17 19:38

Message:
changed the status to pending to allow some time for people to comment and
/ or add patches

----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2010-10-06 04:17

Message:
There is no documentation. If somebody wants to get this patch applied,
documentation is required.

----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2009-07-30 00:08

Message:
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.

----------------------------------------------------------------------

Comment By: Rasmus Kaj (rasmuskaj)
Date: 2009-07-24 07:02

Message:
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?

----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2009-03-08 03:53

Message:
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.

----------------------------------------------------------------------

Comment By: Travis Schneeberger (tschneeberger)
Date: 2009-01-16 05:41

Message:
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

----------------------------------------------------------------------

Comment By: Ulli Hafner (uhafner)
Date: 2009-01-14 12:27

Message:
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).

----------------------------------------------------------------------

Comment By: Trevor Robinson (trevor_robinson)
Date: 2009-01-13 13:48

Message:
Any thoughts on the updated patch?

----------------------------------------------------------------------

Comment By: Trevor Robinson (trevor_robinson)
Date: 2008-12-10 11:09

Message:
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).

----------------------------------------------------------------------

Comment By: Trevor Robinson (trevor_robinson)
Date: 2008-11-28 22:36

Message:
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


----------------------------------------------------------------------

Comment By: Oliver Burn (oburn)
Date: 2008-11-28 20:52

Message:
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?

----------------------------------------------------------------------

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=397080&aid=2356719&group_id=29721

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Checkstyle-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/checkstyle-devel
Loading...