currentSite should be ThreadLocal because not having this screws up a lot of the instrumentation-based dynamic call graphs we generate for the shootout benchmarks.
I have also conditionally changed how the string of the currentSite is created based on the output format that Julian came up with for the dynamic call graph. This support is necessary, because the Java std libraries are not instrumented. Therefore, they would appear as if calls from them show up from nowhere in the log that WALA generates for the dynamic call graph. This fix make those calls originate from a fake library BLOB node in the call graph.
Of course, doing nothing isn't always the right behavior. Sometimes a
previously-unhandled value is truly unexpected and one should fail by
throwing an exception. It may not always be clear whether an
exception or doing nothing is the right choice. For some `switch`
statements affected by this commit, I initially guessed that throwing
an exception was the right default behavior, but was proven wrong when
doing so caused WALA regression test failures. That's strong evidence
that the unmatched values were not really unexpected, but merely
should have been handled by doing nothing as before.
Previously each of these `switch` statements would implicitly do
nothing if an unanticipated `enum` value came along. My impression is
that each of these `switch` statements is supposed to be exhaustive,
such that an unexpected (unhandled) value should never appear. If one
does, we should recognize it and complain loudly.
Of course, sometimes the right behavior for previously-unhandled
values is to do nothing. It may not always be clear whether an
exception or doing nothing is the right choice. For this commit,
WALA's regression tests still pass even with the possibility of
throwing an exception for unexpected values. If we assume that the
test suite is thorough, that tells me that throwing an exception is
the right policy for each `switch` statement that I'm changing here.
This `switch` statement currently covers all possible values of the
`enum` it is testing. However, if a new value were introduced in the
future, the `switch` would have been silent about it instead of
printing a debug message as is done in all of the other cases. Better
to print *some* kind of debug in the default case too.
This `switch` statement currently covers all possible values of the
`enum` it is testing. However, if a new value were introduced in the
future, the `switch` would have allowed control-flow to fall through
by default instead of throwing an exception as is done in all of the
other cases. Better to throw *some* kind of exception in the default
case too.
Eclipse was warning that these `switch` statements had no `default`
cases. Each did have some default behavior, but implemented outside
the `switch`. By moving the default behavior into a `default` case
within the `switch`, we eliminate a static warning with no change
whatsoever to the run-time behavior.
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.
One such annotation was unnecessary because the thing it was
suppressing no longer happens. Any future unnecessary warning
suppressions of this kind will now be treated as errors.
The other annotations were unnecessary because the corresponding
warnings have been disabled entirely in the Eclipse projects'
configurations. There seems to be no way to tell Eclipse to treat
these as anything other than "info" diagnostics in the future, so
that's how they will remain.
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.
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.
I have *not* upgraded this problem to be treated as an error in the
future. Unfortunately Eclipse uses a single configuration setting for
both unnecessary semicolons and also for empty control-flow statements
like `while (p) ;`. I'm not convinced that it's worth rewriting all
instances of the latter into `while (p) { }`. So this is just going
to stay as a warning for now.
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.
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.
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.
In each of these cases, the constructor directly or indirectly has
side effects that we want to keep, even if the object itself is not
retained and used by eht code that invokes `new`.
* 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.
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.
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.
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.
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.
Instead, rely on Java's ability to infer type parameters in many
contexts. This removes 665 Eclipse warnings.
Note: a few of these changes are to files under "test" subdirectories.
Presumably those are files that serve as test inputs rather than being
part of WALA code proper. As far as I can tell, these changes do not
break any WALA tests. But if any of those tests were specifically
intended to exercise WALA on code with non-inferred generic type
parameters, then I really should be leaving those alone.
Most of the invalid HTML arose from bare "<" and ">" characters.
These should be escaped as "<" and ">" when not intended to
introduce HTML tags. When you have many such characters close
together, "{@literal ...}" is a nice, readable alternative that
automatically escapes its contents. If the text in question is
intended to be a code fragment, then "{@code ...}" is appropriate:
this is essentially equivalent to "<code>{@literal ...}</code>".
There were a few other HTML violations too, but none common enough to
be worth detailing here.
Julian Dolby
Karim Ali
Ben Liblit
Manu Sridharan
Martin Hecker
Andreas Sewe
Julian Dolby