<div dir="ltr"><div>Sorry for the slow response due to vacation...</div><div><br></div><div dir="ltr">On Fri, Dec 30, 2022 at 1:00 PM Brian Goetz <<a href="mailto:brian.goetz@oracle.com" target="_blank">brian.goetz@oracle.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><br><div><blockquote type="cite"><div><div dir="ltr">
<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>One thing that makes me a little uncomfortable is that @SW usually is used to say “the compiler can’t prove this is safe, but I am asserting it is.” But this is different; the compiler has discovered the class
is unsafe, and @SW is saying “I know, I know, stop telling me.” So I think there’s an additional documentation opportunity here where, for the classes we had to annotate with @SW, we include an additional @apiNote of the form “This class behaves badly, oops”,
with a link to a common doc page explaining the problem. (While we don’t have to do this necessarily immediately, the chance of it falling on the floor if we don’t are higher.). Basically, what you’ve identified is that there are a number of classes in the
JDK which fail to follow best practices with regard to self-use from constructors. We can’t turn back the clock, but we can mark these as “don’t code like my brother”, so people see these patterns of coding and realize they are not to be emulated.
</div>
</blockquote>
<div><br>
</div>
<div>Good point.</div>
<div><br>
</div>
<div>I think it's also worth considering which scenarios require such documentation. We have drawn the "boundary lines" at the compilation unit boundary, but this means (for example) there are package-private classes that generate warnings. Because
these classes will end up in a module, which requires special effort to crack open, the warnings only really apply to JDK developers working in that same package.<br>
</div>
</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>For package-private classes, we should know all the subclasses, and therefore may have the opportunity to seal it. Then we are not subject to “arbitrary subclass might do X”, but only “these specific subclasses do X.” So perhaps this is an opportunity
to make final / seal some package-private classes that don’t need extensibility.</div></div></div></blockquote><div><br></div><div>Yes, there are probably a bunch of classes out there that could be made 'final'. Seems like this would make for a good follow-up change.<br></div><div><br></div><div>Another consideration regarding <span style="font-family:monospace"><a class="gmail_plusreply" id="plusReplyChip-1">@apiNote</a></span> is how likely the problem really is. For example, several warnings come from code like this:</div><div><br></div><div style="margin-left:40px"><span style="font-family:monospace">private PropertyChangeSupport support = new PropertyChangeSupport(this);</span></div><div><br></div><div>Yes, it's possible in theory that <span style="font-family:monospace">PropertyChangeSupport</span> could someday be modified to invoke some method of 'this', but in reality we know all it does is stash 'this' in a field and return and that's not likely to change anytime soon. Not much to be gained from including an <span style="font-family:monospace">@apiNote</span> warning in those cases.<br></div><div><br></div><div>At any rate, I did a pass through and updated these "popular" classes with leaky constructors with a Javadoc note:</div><div><br></div><div style="margin-left:40px"><span style="font-family:monospace">java/io/PipedReader.java<br>java/io/PipedWriter.java<br>java/lang/Throwable.java<br>java/util/ArrayDeque.java<br>java/util/EnumMap.java<br>java/util/HashSet.java<br>java/util/Hashtable.java<br>java/util/LinkedList.java<br>java/util/TreeMap.java<br>java/util/TreeSet.java</span><br></div><div><br></div><div>You can see these changes in <a href="https://github.com/archiecobbs/jdk/commit/1dee60c44f578f6a8f9786f03c76a3d0c8911674">this commit</a>.</div><div><br></div><div>FYI, I used <span style="font-family:monospace">@implNote</span> instead of <span style="font-family:monospace">@apiNote</span> because this seemed like more of an implementation issue than an API issue (but please correct me if I'm wrong).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_quote"><div>From what I can tell, these are leftovers from about 10 years ago when some developers were using NetBeans, which had just such a warning. I don't think they are relevant any more but I didn't want to presume anything so I left them in there.</div>
</div>
</div>
</blockquote>
<div><br>
</div>
<div>So, LTIC is not a lint category of javac? When Jan gets back from break, maybe he can shed some more light on this. But unless we have a good reason to keep the LTICs, we should consider garbage-collecting them.<br></div></div></div></blockquote><div><br></div><div><span style="font-family:monospace">"LeakingThisInConstructor"</span> is definitely not a current lint category for javac. I've replaced it with <span style="font-family:monospace">"this-escape"</span>.</div><div><br></div><div>I ran across another one as well: <span style="font-family:monospace">"OverridableMethodCallInConstructor"</span> (in <span style="font-family:monospace">src/demo/share/jfc/Notepad/Notepad.java</span>).</div><div> </div><div>Also, since the last post I found and fixed a few bugs, which revealed more 'this' escapes. Here are the new numbers, which have gone up a bit:<br></div></div><div style="margin-left:40px"><span style="font-family:monospace"><br></span></div><div style="margin-left:40px"><span style="font-family:monospace"> #SUPPRESS #FILES MODULE<br> 207 3076 java.base<br> 0 128 java.compiler<br> 4 18 java.datatransfer<br> 475 2899 java.desktop<br> 0 10 java.instrument<br> 9 23 java.logging<br> 52 330 java.management<br> 0 16 java.management.rmi<br> 15 199 java.naming<br> 32 142 java.net.http<br> 0 20 java.prefs<br> 14 106 java.rmi<br> 0 15 java.scripting<br> 0 1 <a href="http://java.se">java.se</a><br> 46 216 java.security.jgss<br> 1 30 java.security.sasl<br> 0 23 java.smartcardio<br> 13 77 java.sql<br> 6 56 java.sql.rowset<br> 0 5 java.transaction.xa<br> 277 1848 java.xml<br> 70 271 java.xml.crypto<br> 3 20 jdk.accessibility<br> 1 21 jdk.attach<br> 1 18 jdk.charsets<br> 84 333 jdk.compiler<br> 2 76 jdk.crypto.cryptoki<br> 4 35 <a href="http://jdk.crypto.ec">jdk.crypto.ec</a><br> 1 11 jdk.crypto.mscapi<br> 7 68 jdk.dynalink<br> 0 3 jdk.editpad<br> 120 942 jdk.hotspot.agent<br> 5 56 jdk.httpserver<br> 0 5 jdk.incubator.concurrent<br> 0 50 jdk.incubator.vector<br> 0 3 jdk.internal.ed<br> 8 61 jdk.internal.jvmstat<br> 16 113 jdk.internal.le<br> 4 52 jdk.internal.opt<br> 5 209 <a href="http://jdk.internal.vm.ci">jdk.internal.vm.ci</a><br> 0 1 jdk.internal.vm.compiler<br> 0 1 jdk.internal.vm.compiler.management<br> 0 18 jdk.jartool<br> 20 228 jdk.javadoc<br> 1 41 jdk.jcmd<br> 44 64 jdk.jconsole<br> 23 124 jdk.jdeps<br> 44 254 jdk.jdi<br> 0 1 jdk.jdwp.agent<br> 0 263 jdk.jfr<br> 2 77 jdk.jlink<br> 5 80 jdk.jpackage<br> 8 88 jdk.jshell<br> 0 4 jdk.jsobject<br> 2 11 jdk.jstatd<br> 2 281 jdk.localedata<br> 1 25 jdk.management<br> 0 19 jdk.management.agent<br> 0 15 jdk.management.jfr<br> 1 16 jdk.naming.dns<br> 0 8 jdk.naming.rmi<br> 0 11 <a href="http://jdk.net">jdk.net</a><br> 0 2 jdk.nio.mapmode<br> 0 11 jdk.random<br> 1 37 jdk.sctp<br> 0 30 jdk.security.auth<br> 0 16 jdk.security.jgss<br> 0 9 jdk.unsupported<br> 0 8 jdk.unsupported.desktop<br> 0 94 jdk.xml.dom<br> 1 14 jdk.zipfs</span></div><div><br></div><div>-Archie<br></div><br>-- <br><div dir="ltr">Archie L. Cobbs<br></div></div>