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

[Maurizio Cimadamore]

On Mon, 16 Jan 2023 16:29:31 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

> It looks like you might be counting the "here via invocation" lines as separate warnings. These are really part of the previous `possible 'this' escape` line, e.g.:

yes, I really did the simplest possible thing (e.g. just counting the number of escape-this warnings, regardless of their kinds). Perhaps some of my measurements might be skewed as a result - but I think that magnitude-wise they are still telling.
> 
> ```
> .../java/awt/Frame.java:429: Note: possible 'this' escape before subclass is fully initialized
>         init(title, null);
>             ^
> .../java/awt/Frame.java:460: Note: previous possible 'this' escape happens here via invocation
>         SunToolkit.checkAndSetPolicy(this);
>                                      ^
> ```
> 
> Semi-related... I was also curious what would happen if we changed the semantics of the warning from "subclass must be in a separate compilation unit" to "subclass must be in a separate package".

To be fair, this is what my brain was reading when you talked about "compilation unit" - but then saw that the code was doing it differently. You see, for "sealed" classes we have a notion of "maintenance domain". E.g. the classes in a `permits` clause of a `sealed` declaration must belong to the same module (if available) or same package (if no module is available). That's how you get the exhaustiveness analysis and all the goodies, by essentially making a close-world assumption on the classes that are passed to javac for a given compilation task. I think it would make a lot of sense to apply these very same boundaries to the escape-this analysis (and, in fact, when looking at warnings coming out of the JDK, I often found false positives that were caused by this).
> 
> I'm not proposing we change this definition, and obviously there are trade-offs in where you draw this boundary, but was curious anywan (at one point I thought it might be worth having an option for this, e.g., with variants like `-Xlint:this-escape` vs. `-Xlint:this-escape:package`, or even `-Xlint:this-escape:module`, etc.).

Perhaps - but, as said above, `sealed` already does this by default - so there's a (strong) precedent, I believe, should we want to bend the analysis that way.
> 
> In any case, here are the results:
> 
>     * Warnings for subclass in separate compilation unit: 2093
> 
>     * Warnings for subclass in separate package: 1334
> 
> 
> So a 36% reduction - not too surprising since the JDK includes a bunch of non-public implementation classes.

That corresponds to what I've sampled (unscientifically).

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

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