RFR: 8015831: Add lint check for calling overridable methods from a constructor [v5]

[Archie L. Cobbs]

On Mon, 9 Jan 2023 15:03:10 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Archie L. Cobbs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix incorrect @bug numbers in unit tests.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 85:
> 
>> 83:  *
>> 84:  * <p>
>> 85:  * When tracking references, we distinguish between direct references and indirect references,
> 
> I'm trying to understand the code and it's not obvious to me as to where this difference comes into play. I believe (but I'm not sure) the spirit is to treat reference to `this` in the original constructor as "direct" while treat a reference to a parameter "a" in a method called from the constructor, where "a" is assigned "this", as indirect?

It's slightly different from that.

Considering any of the various things in scope that can point to an object (these are: the current 'this' instance, the current outer 'this' instance, method parameter/variable, method return value, top of Java stack), such a thing has a "direct" reference if it might possibly _directly_ point to the 'this' we're tracking, while a thing has an "indirect" reference if it might possibly point to the 'this' we're tracking through _at least one level of indirection_.

This is just an attempt to eliminate some false positives by distinguishing between those two cases. Originally I was going to try to track fields (in a very limited way), and so this distinction was going to be more important, but even without tracking fields it's still useful. For example, if some method invokes `x.y.foo()` and `x` represents a direct but not an indirect 'this' reference, then there is no leak declared.

Considering the other options... (a) if you only track direct references, then you suffer from more false negatives (how many? unclear); (b) if you lump direct and indirect references into one bucket, then you suffer from more false positives (as in previous example `x.y.foo()`).

You can see an example of an indirect reference being tracked and exposed in the unit test `ThisEscapeArrayElement.java`.

Another motivating example is lambdas. The act of simply _creating_ a lambda never creates a leak, and a lambda never represents a _direct_ reference. But it might represent an _indirect_ reference.

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 1098:
> 
>> 1096:     private <T extends JCTree> void visitLooped(T tree, Consumer<T> visitor) {
>> 1097:         this.visitScoped(tree, false, t -> {
>> 1098:             while (true) {
> 
> Why is this needed? Can the tracking state of `this` change after multiple "execution" of the same code?

Yes, because the 'this' reference can bounce around through different variables in scope each time around the loop. So we have to repeat the loop until all 'this' references have "flooded" into all the nooks and crannies.

The `ThisEscapeLoop.java` unit test demonstrates:

public class ThisEscapeLoop { 
    
    public ThisEscapeLoop() { 
        ThisEscapeLoop ref1 = this;
        ThisEscapeLoop ref2 = null;
        ThisEscapeLoop ref3 = null;
        ThisEscapeLoop ref4 = null;
        for (int i = 0; i < 100; i++) {
            ref4 = ref3;
            ref3 = ref2;
            ref2 = ref1;
            if (ref4 != null)
                ref4.mightLeak();
        }
    }

    public void mightLeak() {
    }
}

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

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