<div dir="ltr"><div dir="ltr">On Wed, Nov 9, 2022 at 12:06 PM Archie Cobbs <<a href="mailto:archie.cobbs@gmail.com" target="_blank">archie.cobbs@gmail.com</a>> wrote:</div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I'll try to come up with a concrete but simple starting point.</div></blockquote><div><br></div><div>It's taken me a little while, but I finally have a prototype for 'this' escape warnings ready to play with.</div><div><br></div><div>To review this discussion...</div><div><br></div><div>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.<br></div><div><br></div><div>Two related proposals are on the table to help address this:</div><div><br></div><div><b>1. <a href="https://bugs.openjdk.org/browse/JDK-8194743" target="_blank">JDK-8194743</a> Permit additional statements before this/super in constructors</b></div><div><br></div><div>If a subclass is allowed to do initialization prior to invoking <span style="font-family:monospace">super()</span>, the possible damage from a superclass 'this' escape can be reduced or eliminated.</div><div><br></div><div>A prototype is implemented and described in detail <a href="https://github.com/archiecobbs/jdk/tree/JDK-8193760" target="_blank">here</a>.</div><div><br></div><div>Of course this change would also require a JLS spec change.<br></div><div><br></div><div><b>2. Adding a compiler warning to help reveal situations where a 'this' escape can happen.</b></div><div><br></div><div>This is now prototyped <a href="https://github.com/archiecobbs/jdk/tree/ThisEscape" target="_blank">here</a>. The main logic is encapsulated in this class: <a href="https://github.com/archiecobbs/jdk/blob/ThisEscape/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java" target="_blank">ThisEscapeAnalyzer.java</a>.</div><div><br></div><div>Some notes on this analysis:<br></div></div><div class="gmail_quote"><ul><li>We "execute" constructors and track where the 'this' reference goes as the constructor executes.</li><li>We use a very simplified flow analysis that you might call a "flood analysis", where the union of every possible code branch is taken.<br></li><li>A "leak" is defined as the possible passing of a subclassed 'this' reference to code defined outside of the current compilation unit.</li><ul><li>In other words, we don't try to protect the current compilation unit from itself.</li><li>We can ignore private constructors - they can never be used by external subclasses, etc.<br></li></ul><li>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.</li><ul><li>When this occurs the warning displays each step in the stack trace to help in comprehension<br></li></ul><li>The possible locations for a 'this' reference that we try to track are:</li><ul><li>Current 'this'</li><li>Current outer 'this'</li><li>Local parameter/variable</li><li>Method return value</li><li>Current expression value (i.e. top of stack)<br></li></ul><li>We don't try to track assignments to & from fields (for future study).</li><li>We don't try to follow <span style="font-family:monospace">super()</span> invocations<br></li><li>We categorize tracked references as direct or indirect to add a tiny bit of nuance.<br></li></ul><div>Playing around with this, it seems to work pretty well. <br></div><br><div>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 <span style="font-family:monospace">-Xlint:all</span> 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.<br></div><div><div><br></div><div>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 <span style="font-family:monospace">-Xlint:all</span> to compile again. So that's about 50% hit rate... maybe not so bad considering how much code this represents. <br></div></div><div><div><br></div></div><div>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).<br></div><div><br></div><div>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.<br></div></div><div class="gmail_quote"><br></div><div class="gmail_quote">There are some potential refinements, e.g., noticing when all permitted subclasses of a sealed class are in the same compilation unit.</div><div class="gmail_quote"><br></div><div class="gmail_quote">Any thoughts or comments greatly appreciated.<br></div><div class="gmail_quote"><br><div>-Archie<br></div><div> </div></div>-- <br><div dir="ltr">Archie L. Cobbs<br></div></div>