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

[Archie L. Cobbs]

On Fri, 13 Jan 2023 00:57:14 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> Ok - I thought false negative was the thing to absolutely avoid - and that was the no. 1 concern. I think a possible approach to keep both the filtering and the code more or less similar to what you have, is to save the type of the expression in the ExprRef. Then, I believe that, at the end of scan() you can just get whatever type is there, and check that it's the correct one, if not drop it.
>
>>  Ok - I thought false negative was the thing to absolutely avoid - and that was the no. 1 concern.
> 
> You're right. I think at the time I reasoned that it would be unusual enough for the type of an expression to start as an instanceof X, then change to not being an instanceof X, and then change back, that it was worth it to go ahead and filter these out. But admittedly that was based on intuition, not science.
> 
>> I think a possible approach to keep both the filtering and the code more or less similar to what you have, is to save the type of the expression in the ExprRef. Then, I believe that, at the end of scan() you can just get whatever type is there, and check that it's the correct one, if not drop it.
> 
> That's a nice idea... thanks. I'll play around with it some.

I was curious how much of a difference this type filtering makes, so I built the JDK with and without it.

The results were identical except for one case:

package java.lang.invoke;
...
public abstract sealed class CallSite permits ConstantCallSite, MutableCallSite, VolatileCallSite {
...
    CallSite(MethodType targetType, MethodHandle createTargetHook) throws Throwable {
        this(targetType); // need to initialize target to make CallSite.type() work in createTargetHook
        ConstantCallSite selfCCS = (ConstantCallSite) this;
        MethodHandle boundTarget = (MethodHandle) createTargetHook.invokeWithArguments(selfCCS);
        setTargetNormal(boundTarget); // ConstantCallSite doesn't publish CallSite.target
        UNSAFE.storeStoreFence(); // barrier between target and isFrozen updates
    }

When we do type filtering, `(ConstantCallSite) this` causes the 'this' reference to be discarded so no leak is reported on the next line for `invokeWithArguments(selfCCS)`. Just a side note, there is also a leak on the next line because final method `setTargetNormal()` invokes `MethodHandleNatives.setCallSiteTargetNormal(this, newTarget)`, so a leak does get reported anyway even with type filtering.

When type filtering is disabled, we report a leak at `invokeWithArguments(selfCCS)` - which is accurate.

So what did we learn?

First it looks like type filtering has very minimal effect. I think this is because it requires some version of two casts in a row in and out of type compatibility, and this is probably very rare.

But looking at the one data point we do have, the type filtering did in fact cause a false negative.

And when building the entire JDK, it causes zero net new false positives.

So to me this is evidence that we should just remove the type filtering altogether...

@vicente-romero-oracle your thoughts?

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

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