checkstyle

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.

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?

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.

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

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

Any thoughts on the updated patch?

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

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

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?