In general, my approach was to try to eliminate each unused parameter
using Eclipse's "Change Method Signature" refactoring. That did not
always succeed: a parameter may be unused in some base class method,
but then be used in subclass's override of that method. In cases
where refactoring to eliminate a parameter failed, I instead annotated
the parameter with '@SuppressWarnings("unused")' to silence the
warning.
Note: this group of changes creates a significant risk of
incompatibility for third-party WALA code. Some removed parameters
change externally-visible APIs. Furthermore, these changes do not
necessarily lead to Java compilation errors. For example, suppose
third-party code subclasses a WALA class or interface, overrides a
method, but does not annotate that method as @Override. Removing a
parameter means that the third-party method no longer overrides. This
can quietly change code behavior without compile-time errors or
warnings. This is exactly why one should use @Override wherever
possible, but we cannot guarantee that third-party WALA users have
done that.
Previously we had 242 such warnings. That large number suggests that
the WALA developers consider this to be an acceptable coding style.
If that's so, then it's better to hide these warnings rather than keep
them around as a perpetual distraction.
There are two such diagnostics: one for collection methods and one for
equals(). See
<https://www.eclipse.org/eclipse/news/4.7/jdt.php#unlikely-argument-types>
for more information about these two new diagnostics.
For each of these diagnostics, I've set the severity level to
"warning" in projects that have some instances of the suspicious code,
or to "error" in projects that have no instances of the suspicious
code.
These should mostly be things that we've already decided earlier that
we explicitly don't want to "fix" because they simply disagree with
the WALA project's coding style.
The additional diagnostics are ones that were previously being
ignored, but which we seem to have been ignoring by default rather
than as a conscious choice.
For diagnostics of which we currently have *zero* instances, treat
these as errors rather than merely warnings. The intent is to
permanently lock out future regressions of things we've completely
fixed. In the future, whenever we fix the last instance of a given
warning in a given Eclipse project, we should also promote that
diagnostic to an error to keep things clean into the future.
If a method is private, there's no risk that a subclass elsewhere
might be overriding it and depending on dynamic dispatch to choose the
right implementation. So all of these private methods can safely be
declared static without risk of regression in either WALA code or
unseen third-party code.
The "potentially" qualifier is here because these methods are visible
outside the WALA source tree. These methods may seem OK to be static
based on the code we have here, but we have no way of knowing whether
third-party code expected to be able to subclass and override. I'm
going to play it safe and assume that we want to allow that.
Note that we are still allowing Eclipse warnings about methods that
can *definitely* be declared static; a different configuration option
controls these. For private methods, final methods, and methods in
final classes, if the code seems static-safe based on what we have
here, then that's good enough: we don't need to worry about
third-party overrides.
In all of these cases, the code used to initialize the unused local
seems nontrivial to me. I think this can all be removed without
erasing any needed side effects, but the code might still become
useful to someone in the future. So I'm not really removing the code
entirely, but merely commenting it out.
Previously we were saving these return values into local variables
that were never checked or used in any way. So effectively we were
already discarding these results anyway, but in a manner that produced
Eclipse warnings about unused local variables.
This fixes 49 Eclipse code style warnings. I'm not sure why these
were overlooked in my previous sweep of missing-@Override warnings.
Ah well; got 'em this time around.
* Fix warnings about unset javacProjectSettings build entries
Specifically, these are all warnings of the form "The
'javacProjectSettings' build entry should be set when there are project
specific compiler settings".
* Add @Override annotations to all methods that do override
This fixes 287 Eclipse code style warnings.
* Cannot add @Override annotations here, so suppress warnings instead
We should be able to add these @Override annotations in the future,
one Eclipse Mars and earlier are no longer supported. For now,
though, they have to go away in order to be compatible with older
Eclipse releases.
* Fix warnings about unset javacProjectSettings build entries
Specifically, these are all warnings of the form "The
'javacProjectSettings' build entry should be set when there are project
specific compiler settings".
* Turn off Eclipse warnings about undocumented empty blocks
The presence of 65 such warnings in two packages suggests that the de
facto WALA coding style does not mandate documenting empty blocks.
Better to avoid printing warnings that will be routinely ignored, so
that other important warnings are more likely to be noticed.
* Turn off Eclipse warnings about synthetic accessor methods
Synthetic accessor methods allow access to otherwise inaccessible
members of enclosing types. The presence of 246 such warnings in
three packages suggests that the de facto WALA coding style does not
consider synthetic accessor methods to be problematic. Better to
avoid printing warnings that will be routinely ignored, so that other
important warnings are more likely to be noticed.
Removing an unused field sometimes means removing constructor code
that used to initialize that field. Removing that initialization code
sometimes leaves whole constructor arguments unused. Removing those
unused arguments can leave us with unused code to compute those
arguments in constructors' callers, and so on. This commit tries to
clean all of this up, working backward from the unused fields that an
earlier commit already removed. Hopefully I have avoided removing
upstream code that had other important side effects, but it wouldn't
hurt for a WALA expert to review this change carefully.
If the true block of an `if` statement is guaranteed to exit early,
such as by a `return` or `throw`, then any code appearing in a
corresponding `else` clause could just as well have appeared after the
`if` statement entirely. Eclipse can warn about this.
However, Manu prefers to let such code stay in the `else` clauses.
OK, sure: this is more a matter of personal taste than something truly
problematic. Per Manu's request, then, we're turning off that Eclipse
warning in the subprojects in which it currently arises.
Manu requested that we use this approach instead of adding
`@SuppressWarnings("unused")` at each affected catch block. That
seems reasonable to me, given the large number of such warnings and
the lack of likely harm from ignoring such caught exceptions.
These changes turn off Eclipse warnings for Javadoc comments with
missing tags, such as "@throw" or "@param".
We don't turn this warning off in all projects. Rather, we turn it
off only in projects that were producing at least one such warning.
In other words, if a project was already completely "clean" with
respect to this warning, then we leave this warning enabled for that
project.
Turning off these warnings is a partial declaration of Javadoc
bankruptcy. In an ideal world, we would enable and fix all of these
warnings. However, there are 327 of them. Apparently the WALA team's
implicit coding style says that omitting Javadoc tags is OK. If
there's no intent to systematically add these tags, then we may as
well turn off these warnings so that we can see other warnings that we
may want to fix.
These changes turn off Eclipse warnings for documentable items without
Javadoc comments. In some subprojects, we turn these off entirely.
In others, we leave these warnings on for public items but not for
items whose visibility is protected or below.
We don't turn this warning off in all projects. Rather, we turn it
off only in projects that were producing at least one such warning.
In other words, if a project was already completely "clean" with
respect to this warning, then we leave this warning enabled for that
project.
Turning off these warnings is a partial declaration of Javadoc
bankruptcy. In an ideal world, we would enable and fix all of these
warnings. However, there are 1,366 of them. Apparently the WALA
team's implicit coding style says that omitting Javadoc comments is
OK. If there's no intent to systematically add documentation, then we
may as well turn off these warnings so that we can see other warnings
that we may want to fix.
These changes turn off Eclipse warnings for Javadoc tags without
descriptions. In some subprojects, we turn these off entirely. In
others, leave on missing-descrption checks for "@return" tags only.
We don't turn this warning off in all projects. Rather, we turn it
off only in projects that were producing at least one such warning.
In other words, if a project was already completely "clean" with
respect to this warning, then we leave this warning enabled for that
project.
Turning off these warnings is a partial declaration of Javadoc
bankruptcy. In an ideal world, we would enable and fix all of these
warnings. However, there are 576 of them. Apparently the WALA team's
implicit coding style says that omitting descriptions is OK. If
there's no intent to systematically add descriptions, then we may as
well turn off these warnings so that we can see other warnings that we
may want to fix.
In the cases addressed here, the caught exception was being "handled"
by throwing some new exception. Instead of discarding the old
exception, pass it to the new exception's constructor to indicate the
original cause of the newly-created exception. This practice, called
"exception chaining", can often be useful in debugging.
Specifically, these warnings are always of the form "Statement
unnecessarily nested within else clause. The corresponding then clause
does not complete normally". I can see this being a matter of
personal taste, actually. If the WALA maintainers decide that they
prefer to keep this code as it was before, that's OK with me: I'll
replace this group of code changes with a group of settings changes
that tell Eclipse to quietly ignore this "problem".
This fixes the remaining 34 Eclipse "Resource '...' should be managed
by try-with-resource" warnings that were still left after the previous
commit.
Unlike the fixes in that previous commit, the changes here are *not*
plugging potential resource leaks. However, in many cases that is
simply because the code before the close() call cannot currently throw
exceptions. If exceptions became possible in the future, leaks could
result. Using try-with-resource preemptively avoids that.
Furthermore, in code that was already dealing with exceptions, the
try-with-resource style is usually considerably simpler.
Ben Liblit
Manu Sridharan
Manu Sridharan
Julian Dolby