Loosening requirements for super() invocation

[Archie Cobbs]

On Wed, Nov 9, 2022 at 12:06 PM Archie Cobbs <archie.cobbs at gmail.com> wrote:

> I'll try to come up with a concrete but simple starting point.
>

It's taken me a little while, but I finally have a prototype for 'this'
escape warnings ready to play with.

To review this discussion...

Java suffers from a 'this' escape problem, which is when a constructor
invokes a method that could be overridden in a subclass, in which case the
method will execute before the subclass constructor has finished
initializing the instance, possibly leading to chaos, or at least
unexpected and difficult to debug behavior.

Two related proposals are on the table to help address this:

*1. JDK-8194743 <https://bugs.openjdk.org/browse/JDK-8194743> Permit
additional statements before this/super in constructors*

If a subclass is allowed to do initialization prior to invoking super(),
the possible damage from a superclass 'this' escape can be reduced or
eliminated.

A prototype is implemented and described in detail here
<https://github.com/archiecobbs/jdk/tree/JDK-8193760>.

Of course this change would also require a JLS spec change.

*2. Adding a compiler warning to help reveal situations where a 'this'
escape can happen.*

This is now prototyped here
<https://github.com/archiecobbs/jdk/tree/ThisEscape>. The main logic is
encapsulated in this class: ThisEscapeAnalyzer.java
<https://github.com/archiecobbs/jdk/blob/ThisEscape/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java>
.

Some notes on this analysis:

   - We "execute" constructors and track where the 'this' reference goes as
   the constructor executes.
   - We use a very simplified flow analysis that you might call a "flood
   analysis", where the union of every possible code branch is taken.
   - A "leak" is defined as the possible passing of a subclassed 'this'
   reference to code defined outside of the current compilation unit.
      - In other words, we don't try to protect the current compilation
      unit from itself.
      - We can ignore private constructors - they can never be used by
      external subclasses, etc.
      - If a constructor invokes a method defined in the same compilation
   unit, and that method cannot be overridden, then our analysis can safely
   recurse into the method.
      - When this occurs the warning displays each step in the stack trace
      to help in comprehension
      - The possible locations for a 'this' reference that we try to track
   are:
      - Current 'this'
      - Current outer 'this'
      - Local parameter/variable
      - Method return value
      - Current expression value (i.e. top of stack)
      - We don't try to track assignments to & from fields (for future
   study).
   - We don't try to follow super() invocations
   - We categorize tracked references as direct or indirect to add a tiny
   bit of nuance.

Playing around with this, it seems to work pretty well.

As you might expect, when you turn this on with existing code like
java.base, you can get a lot of warnings. One fear with adding this warning
is that developers who like to use -Xlint:all by default will suddenly see
a zillion new warnings and freak out. So it's probably worth understanding
how likely this warning is to fire with existing code out there.

As one rough data point, when I build OpenJDK's 71 total modules, 33 of
them have a 'this' escape somewhere and require me to add an exclusion for
'this-escape' from the -Xlint:all to compile again. So that's about 50% hit
rate... maybe not so bad considering how much code this represents.

At least the warnings that I've looked at so far are all indeed true
positives, in the sense that the constructor does in fact pass a 'this'
reference to a method that a subclass, someday, somewhere, could override.
Mostly it's one of two cases: the constructor invokes an overridable
instance method, or the constructor invokes a method with 'this' as a
parameter (e.g. registering a listener).

Some of the warnings come from package-private classes. This is consistent
with the current design, but it brings up the notion that one could draw
the "external" line differently... i.e., instead of at the compilation unit
boundary, at the Java package boundary, or even the Java module boundary.

There are some potential refinements, e.g., noticing when all permitted
subclasses of a sealed class are in the same compilation unit.

Any thoughts or comments greatly appreciated.

-Archie

-- 
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/amber-dev/attachments/20221215/e2d67b8d/attachment-0001.htm>