RFR: 8317376: Minor improvements to the 'this' escape analyzer

Archie Cobbs acobbs at openjdk.org
Tue Dec 19 22:53:16 UTC 2023


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, this decision is encapsulated in a new method `triggersUnknownInvokeLeak()`.

#### 4. Fix tracking of new instances

Previously we were not tracking references from a constructor's "return value", i.e., the new instance. We fix this by pretending that constructors end with `return this`.

#### 5. Fix tracking of enhanced `for()` loops

Previously we were not tracking the implicit invocations of `iterator()`, `hasNext()`, and `next()` associated with enhanced `for()` loops. This patch adds that tracking.

#### 6. Fix leaks via return values from lambdas and method references

Leaks inside lambdas and methods referred-to by method reference were being detected properly, but leaks from the _return values_ of those things were not.

So for example, this code would escape notice:

import java.util.function.Supplier;
public class Test {

    public Test() {
        ((Supplier<Test>)this::self).get().mightLeak();
    }   

    public final Test self() {
        return this;
    }

    protected void mightLeak() {
        // a subclass could override this method
    }
}

This patch fixes this omission.

#### 7. Some minor refactoring to make the code clearer

* Some methods were renamed
* Some bits of code were extracted into separate methods
* Added `Ref`  methods `toDirect()`, `toIndirect()`, `toOuter()`, and `fromOuter()`

#### 8. Remove some unnecessary suppression

Several `*.gmk` build files were suppressing `this-escape` warnings unnecessarily. These have been updated.

-------------

Commit messages:
 - Merge branch 'master' into JDK-8317376
 - Merge branch 'master' into JDK-8317376
 - Javadoc++
 - Merge branch 'master' into JDK-8317376
 - Several improvements to the 'this' escape analyzer.

Changes: https://git.openjdk.org/jdk/pull/16208/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16208&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8317376
  Stats: 870 lines in 20 files changed: 512 ins; 148 del; 210 mod
  Patch: https://git.openjdk.org/jdk/pull/16208.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16208/head:pull/16208

PR: https://git.openjdk.org/jdk/pull/16208


More information about the build-dev mailing list