Please review several fixes and improvements to the `this-escape` lint warning
analyzer.
The goal here is to apply some relatively simple logical fixes that improve the
precision and accuracy of the analyzer, and capture the remaining low-hanging
fruit so we can consider the analyzer relatively complete with respect to
what's feasible with its current design.
Although the changes are small from a logical point of view, they generate a
fairly large patch due to impact of refactoring (sorry!). Most of the patch
derives from the first two changes listed below.
The changes are summarized here:
1. Generalize how we categorize references
The `Ref` class hierarchy models the various ways in which, at any point during
the execution of a constructor or some other method/constructor that it
invokes, there can be live references to the original object under construction
lying around. We then look for places where one of these `Ref`'s might be
passed to a subclass method. In other words, the analyzer keeps track of these
references and watches what happens to them as the code executes so it can
catch them trying to "escape".
Previously the `Ref` categories were:
* `ThisRef` - The current instance of the (non-static) method or constructor
being analyzed
* `OuterRef` - The current outer instance of the (non-static) method or
constructor being analyzed
* `VarRef` - A local variable or method parameter currently in scope
* `ExprRef` - An object reference sitting on top of the Java execution stack
* `YieldRef` - The current switch expression's yield value(s)
* `ReturnRef` - The current method's return value(s)
For each of those types, we further classified the "indirection" of the
reference, i.e., whether the reference was direct (from the thing itself) or
indirect (from something the thing referenced).
The problem with that hierarchy is that we could only track outer instance
references that happened to be associated with the current instance. So we
might know that `this` had an outer instance reference, but if we said `var x =
this` we wouldn't know that `x` had an outer instance reference.
In other words, we should be treating "via an outer instance" as just another
flavor of indirection along with "direct" and "indirect".
As a result, with this patch the `OuterRef` class goes away and a new
`Indirection` enum has been created, with values `DIRECT`, `INDIRECT`, and
`OUTER`.
2. Track the types of all references
By keeping track of the actual type of each reference (as opposed to just
looking at its compile-time type), we can make more precise calculations. We
weren't doing this before.
For example consider this class:
public class Leaker {
public Leaker() {
Runnable r = new Runnable() {
public void run() {
Leaker.this.mightLeak();
}
};
r.run(); // there is a leak here
}
protected void mightLeak() {
// a subclass could override this method
}
}
Previously we would not have detected a 'this' escape because we wouldn't have
known that variable `r` is a `Leaker$1`, which allows us to deduce that
`r.run()` is actually `Leaker$1.run()` (which we can analyze) instead of just
`Runnable.run()` (which we can't).
Because of this change, some `Ref` types that were previously singletons are no
longer singletons. For example, now there can be more than one `ReturnRef` in
scope at any time, representing different possible types for the method's
return value.
3. Handling of non-public outer instances
To eliminate some false positives, we no longer warn if a leak happens solely
due to a "non-public" outer instance.
For example, consider this code:
public class Test {
public Test() {
System.out.println(new Inner());
}
private class Inner {
Object getMyOuter() {
return Test.this;
}
}
}
The outer `Test` instance associated with objects of type `Test.Inner` is
considered "non-public" because it's not directly accessible to the outside
world (because `Test.Inner` is a private class). So while it's true that
instances of `Test.Inner` retain a reference to their enclosing instance, an
unrelated method like `System.out.println()` wouldn't know how to access them.
The only way a `Test.Inner` could leak its outer instance to such a method is
if it did so (perhaps indirectly) through some entrée that the method "knows
about". Of course, that's possible, but absent any further analysis (which
could be added in the future), the cost/reward trade-off doesn't seem worth
generating a warning here.
This is an area for future research. In other words: when a `Ref` is passed to
unknown code, when is it worth complaining about a leak, and when should we
just be silent? Previously we always complained, but now that we have more
information about references (namely, their types) we can be a little more
discriminating.
In any case, th
