Julian Dolby assures me that WALA is now supposed to be using Java 8
everywhere. This covers nearly all remaining places that I can find
where an earlier Java version was still being used. (The few
exceptions are places where switching to Java 8 causes test failures.
I'll address those separately, probably by reaching out to the WALA
maintainers for help.)
E-mail exchanged with Julian Dolby suggests that this is the right
thing to do, and that it should have been done back when we converted
other parts of the build configuration to Java 8.
In modern Java, we would not need to do this. Java 1.8 can infer the
generic type, which would allow us to write simply
"Collections.emptySet()" instead of
"Collections.<TypeReference>"emptySet()". Unfortunately, Android is
behind the times, so this specific project targets Java 1.5. That
older Java definition does not do the inference we would want here, so
we have to be explicit instead.
The former returns Set<T> for any T, whereas the latter merely returns
a raw Set. Using the former instead of the latter fixes four Eclipse
warnings about using raw generics.
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.
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.
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.
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.
Ben Liblit
Julian Dolby
Manu Sridharan
Manu Sridharan