This change affects both top-level subdirectory names as well as
Eclipse plug-in feature names. Perhaps it would have been possible to
change only the latter, but I don't like the idea of the two being
different.
These name changes fix three Eclipse plug-in warnings of the form:
Illegal value '...-feature' for attribute 'id'.
Legal token characters are "a-z", "A-Z", "0-9", "_". Tokens
must be separated by "."
I'll be the first to admit that I know nearly nothing about Eclipse
plug-in development. If changing these plug-in feature IDs has
broader implications that the automated regression tests won't detect,
then I probably overlooked them too. I would greatly appreciate
skeptical review of this change by someone who knows Eclipse plug-in
development well.
Note that personal Eclipse workspaces may need some manual adjustment
after this change. The three "...-feature" Eclipse projects should be
removed from the workspace, and the three corresponding "..._feature"
Eclipse projects should be added. If you do your git pull using
Eclipse's team features, perhaps it is smart enough to do this for
you? I don't know, but it wouldn't surprise me if fixing things
manually were still needed even in that case.
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.
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.
In general, these diagnostics are now errors in projects for which all
such warnings have been fixed. There are three unfixed warnings in
two projects, so this diagnostic remains a warning (not an error) in
those projects.
There are also many places where rwa-types-usage warnings have been
locally suppressed using @SuppressWarnings annotations. I haven't
systematically revisited those to see if any can be fixed properly.
So for those projects this diagnostic must also remain a warning (not
an error), since @SuppressWarnings does not work on things Eclipse is
configured to treat as errors.
Along the way, I also converted many "for (;;)" loops into modern
"for (:)" loops. I didn't systematically look for all opportunities
to do this, though. I merely made this change where I was already
converting raw Iterator uses into modern Iterator<...> uses.
Better use of generics also allowed many casts to become statically
redundant. I have removed all such redundant casts.
Only three raw-types warnings remain after this batch of fixes. All
three involve raw uses of CallGraphBuilder. I've tried to fix these
too, but it quickly snowballs into a cascade of changes that may or
may not eventually reach a statically-type-save fixed point. I may
give these last few problem areas another go in the future. For now,
though, the hundreds of other fixes seem worth keeping even if there
are a few stragglers.
This commit may change some public APIs, but only by making weaker
type signatures stronger by replacing raw types with generic types.
For example, we may change something like "Set" into "Set<String>",
but we're not adding new arguments, changing any
underlying (post-generics-erasure) types, etc.
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.
This fixes five Eclipse "Source folder '...' does not have the output
folder in corresponding output entry 'output..'" warnings in the
"Plug-in Development" category.
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.
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.
Specifically, these are all warnings of the form "The
'javacProjectSettings' build entry should be set when there are project
specific compiler settings".
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.
This fixes 33 out of 37 Eclipse "Potential resource leak: '...' may
not be closed" warnings. It also fixes 3 out of 37 Eclipse "Resource
'...' should be managed by try-with-resource" warnings, although that
was not the main focus of this effort.
The remaining 4 warnings about potential resource leaks all involve a
leaked JarFile instance that is passed to a JarFileModule constructor
call. JarFileModile never attempts to close its underlying JarFile;
this code is written as though JarFile cleanup were the caller's
responsibility. However, the JarFile often cannot be closed by the
code that creates the JarFileModule either, since the JarFile needs to
remain open while the JarFileModule is in use, and some of these
JarFileModules stay around beyond the lifetime of the code that
created them. Truly fixing this would essentially require making
JarFileModule implement Closeable, which in turn would probably
require that Module implement Closeable, which in turn would require
changes to lots of code that deals with Module instances to arrange
for them to be properly closed. That's more invasive than I'm
prepared to take on right now.
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's XML validator warns about missing grammar constraints in
several XML files that come from non-WALA projects. We are not in a
position to do anything about these problems.
As created by Tycho Surefire, these files are XML documents without
DTD or XML Schema declarations. The XML validator warns about this
omission. However, Surefire is not a WALA component. We are not in a
suitable position to change it to include XML schema or DTD
declarations in the XML files it generates. Better, then, to ignore
this benign problem so we can focus on warnings that we can act on
directly.
It's unclear whether the original authors of these pages intended them
to be valid or invalid. Certainly there is merit in testing against
invalid HTML, since the vast majority of real-world HTML is indeed
invalid. I'm going to assume that any errors in this collection of
test inputs are intentional, and therefore not worth reporting when
running Eclipse HTML validation.
Plugin documentation includes plenty of invalid HTML. However, we
don't maintain 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.
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.
In general, the WALA code base is not really ready for nullness
checking. It would be nice if we got there some day, but I'm not
planning to take that on now or any time soon. Until then, it's not
useful to warn about missing @NonNullByDefault declarations on WALA
packages.
See also older commit 7b6811b.
Ben Liblit
Julian Dolby
Manu Sridharan
Manu Sridharan
Manu Sridharan
Julian Dolby