These settings files currently are generated with an initial timestamp
comment line, which is not something we'd want to track in version
control. Fortunately, the contents of these files are entirely
mundane, so there should be no problem with having Buildship generate
them anew each time a developer imports WALA into Eclipse as an
existing Gradle project.
Using constructor references apparently pulls in something involving
nullness annotations. However, we don't actually build with a jar
file that defines those annotations, so this leads to Eclipse build
failures. I don't know the right way to add such a jar file to our
current configuration mishmash of Ant, Maven, and Eclipse. So the
easier thing to do is just disable annotation-based nullness analysis.
I doubt we were getting any benefit from such an analysis anyway,
given that WALA itself doesn't use those annotations at all.
In Eclipse projects that currently have no definite or potential
resource leaks, treat any such diagnostics as errors in the future.
In `com.ibm.wala.core`, enable warnings about definite or potential
resource leaks. Previously these diagnostics were turned off entirely
in this project. So we actually end up with more warnings now than we
had before, but they are all warnings we should eventually look into.
These are all problems that Eclipse can detect, but that it detects no
instances of right now. Treating these as warnings instead of errors
should help prevent us from slipping backward in the future.
Each of these required careful consideration of what the original
developer *intended* as distinguished from what the developer's code
actually *does*. I believe I got each one right, and WALA's
regression tests agree. A second opinion by a core WALA developer
would be welcome, though.
Most of these are harmless, and are best fixed simply by removing the
redundant check or assignment. The one in FlowType.compareBlocks,
however, revealed a real problem. This code checks for nullness of
`a` *after* having called a method on `a`. Assuming that `a` can
indeed be `null` here, the check must come first to avoid a
`NullPointerException`.
In several places, I saw code of this form:
if (thing == null)
assert thing != null : ... ;
I honestly don't understand the purpose of that `if` statement. Why
not just have the `assert` statement there directly? I removed the
seemingly irrelevant `if` statements in these cases, but if this is
some intentional pattern, please explain it to me.
In a few places where nullness is statically known but non-obvious,
add assert statements to point out what's going on to help future
developers.
Upgrade future such warnings to errors to keep us moving in a cleaner
direction.
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.
Unnecessary "throws" declarations tend to cascade. If foo() calls
bar() and bar() falsely declares that it might throw IOException, that
often leads a programmer to declare that foo() might throw IOException
as well. Fixing the bar() throws declaration then reveals that we can
fix the foo() throws declaration too. By the time we reach a fixed
point with cleaning these up, we have removed roughly 320 unnecessary
throws declarations.
In a few cases, this cleanup even lets us remove entire "try
... catch" statements where the only thing being caught was an
exception that we now statically know cannot be thrown. Nice!
In Eclipse project configurations, upgrade any future such shenanigans
from warnings to errors. Now that we've fixed this, we don't want it
coming back again.
There is a potential drawback to this change. Conceivably some public
WALA API entry point might have declared that it could throw some
exception merely to reserve the *option* of throwing that exception in
third-party code that subclasses and overrides the API entry point in
question. I have no idea whether this is a significant concern in
practice, though.
The fix is to add "static" where appropriate, of course. I've also
simplified calls to such methods to reflect the fact that they no
longer need a specific object to call the method on.
In projects that contain test inputs, I've left the non-static
declarations unchanged, and instead downgraded the warning to be
ignored. In all other projects, this warning has been upgraded to an
error.
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.
Generally, overriding one means you should be overriding the other
too.
Also, configure Eclipse to treat any similar cases as errors, rather
than merely warnings.
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.
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.
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 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.
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.
I think the "target/p2artifacts.xml" and "target/p2content.xml" files
are generated by Maven. They are well-formed XML but Eclipse's XML
validator legitimately warns that they lack grammar constraints.
Since we're not maintaining the tool that creates these files, we are
not in a position to do anything about that. Therefore, we may as
well exclude these from validation entirely. That way we can
more-clearly recognize warnings that we *can* do something about.
Eclipse validation warns about invalid HTML content in all
Maven-generated "target/site/dependency-convergence.html" files. The
warnings are legitimate: these HTML files are indeed invalid.
However, we don't maintain the tool that generates these files, so we
are not in a position to fix them. Better, therefore, to suppress
these warnings so that we can notice and fix other problems over which
we do have control.
Ben Liblit
Julian Dolby
Manu Sridharan
Juergen Graf
Manu Sridharan
Manu Sridharan
sjfink
msridhar1