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.
Apparently Buildship generates these when one uses Import -> Existing
Gradle Project:
<https://discuss.gradle.org/t/buildship-eclipse-plug-in-multiproject-builds/24030/5>.
We can use the Gradle "eclipse" plugin if customizations are
necessary, but my impression is that the intent is to treat ".project"
and ".classpath" as generated files, not sources to be tracked in
source control.
I was confused about the differences among:
srcDir 'foo'
srcDirs ['foo']
srcDirs = ['foo']
As it turns out, the first two append to the set of source
directories, while the last replaces this set entirely. I generally
want replacement, since WALA's current directory layout never matches
Gradle's assumed defaults.
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.
Eclipse's automated code clean-up tool did most of the heavy lifting
here: it specifically has a clean-up option for converting functional
interfaces to lambdas. I merely had to revert the automated changes
for a single enumeration class for which it produced invalid results,
and for a few test inputs that apparently aren't set up to be compiled
with Java 8.
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.
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.
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.
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.
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.
Ben Liblit
Sungho Lee
Julian Dolby
Manu Sridharan
Manu Sridharan