We already have plenty of examples of Serializable classes with this
field, and the vast majority of those fields have generated IDs rather
than "1L". From this I infer that using proper serialVersionUID
fields is considered appropriate WALA coding style.
Also report unused variables as errors in the future, not just
warnings. We've fixed all of these as of right now, so let's keep it
clean in the future too.
Previously we had 227 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.
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.
Previously some of these were accessing such fields through a subclass
of the declaring class. That creates an unnecessary extra inter-class
dependency lower in the type hierarchy than necessary.
Also, suppress this warning in an automated test input where the
indirect static accesses are explicitly intentional.
These methods access only static members. They are methods of a final
class, which means no subclass can ever override these methods and use
dynamic dispatch to choose the implementation at run time. So
declaring these methods static is (statically) safe.
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.
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.
Specifically, we're turning off Eclipse warnings about missing version
constraints on required bundles ("Require-Bundle"), exported
packages ("Export-Package"), and imported packages ("Import-Package").
We're not turning these off absolutely everywhere, though: only in
packages where one or more such warnings were actually being reported.
So if a given package was already providing all version constraints
for, say, package imports, then we've kept that warning on in that
package.
Honestly I don't entirely understand the practical implications of
these warnings. However, there were 355 of them across many WALA
subprojects. I take this as evidence that the WALA developers do not
consider these version constraints to be important. Therefore, we may
as well stop warning about something we have no intention of fixing.
That being said, if we *do* want to fix some or all of these, I
welcome any advice on what those fixes should look like. I am rather
ignorant about all things OSGi.
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.
Fixing these Javadoc comments would require adding packages to various
other packages' build paths. In some of the cases suppressed,
changing build paths in that manner would create circular build
dependencies. In other cases, it would simply add a Javadoc-motivated
dependency that does not exist for the real code, which seems
undesirable. For a few cases, the reference seems to be to types in
code we don't even have here, such as code from "android" or
"org.mozilla" packages.
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 arise, for example, when Javadoc documentation on a public class
includes a @link to a private field. I can see how this would be
problematic for closed-source Java code where private items are
invisible to outsiders. However, given that WALA is open source,
nothing is truly non-visible. If the WALA documentation authors
considered non-visible references useful when explaining things,
that's fine with me.
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.
Manu Sridharan
Ben Liblit