Eclipse was warning about the lack of a `default` case in the `switch`
statement. None of the current `enum` values was actually missing
from the `switch`, but if the enum values list were extended in the
future, that would be an easy mistake to make.
Replacing the `switch` with dynamic dispatch to a method lets us
statically enforce the requirement that every enum value must
implement the behavior in some way.
Instead of having two `switch` statements on the Dot format
enumeration, give each Dot enumeration value a way to identify its own
preferred file suffix and command-line format name. This removes some
warnings about `switch` statements without default cases. It also
creates strong static enforcement that any new Dot format *must* also
provide this information.
Casting to `Foo<Bar>` results in an unchecked-cast warning due to Java
generics type erasure. However, sometimes we don't really need a
`Foo<Bar>`, but could simply use any `Foo<?>`. Casting to the latter
creates no warning.
This is the first time I can ever remember explicitly casting to
`Object`. Such a cast might seem useless, and certainly it is
statically known to always succeed. However, it is here for good
reason. Without the cast, we end up making a `Pair<Object, ?>` but
what we really want is a `Pair<Object, Object>`.
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.
The single "No grammar constraints (DTD or XML Schema) referenced in
the document" warning arises in a generated file. I doubt that we can
change the generation process to include grammar information. Even if
we could, I don't mind omitting validation here assuming that we can
trust the generator tool to be correct. Validation is much more
important for human-authored XML; for tool-authored XML, we can live
without it.
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.
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.