Loosening requirements for super() invocation
Archie Cobbs
archie.cobbs at gmail.com
Wed Jan 4 18:23:34 UTC 2023
Sorry for the slow response due to vacation...
On Fri, Dec 30, 2022 at 1:00 PM Brian Goetz <brian.goetz at oracle.com> wrote:
>
> 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.
>>
>
> Good point.
>
> 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.
>
>
> 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.
>
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.
Another consideration regarding @apiNote is how likely the problem really
is. For example, several warnings come from code like this:
private PropertyChangeSupport support = new PropertyChangeSupport(this);
Yes, it's possible in theory that PropertyChangeSupport 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 @apiNote warning in
those cases.
At any rate, I did a pass through and updated these "popular" classes with
leaky constructors with a Javadoc note:
java/io/PipedReader.java
java/io/PipedWriter.java
java/lang/Throwable.java
java/util/ArrayDeque.java
java/util/EnumMap.java
java/util/HashSet.java
java/util/Hashtable.java
java/util/LinkedList.java
java/util/TreeMap.java
java/util/TreeSet.java
You can see these changes in this commit
<https://github.com/archiecobbs/jdk/commit/1dee60c44f578f6a8f9786f03c76a3d0c8911674>
.
FYI, I used @implNote instead of @apiNote because this seemed like more of
an implementation issue than an API issue (but please correct me if I'm
wrong).
>
> 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.
>
>
> 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.
>
"LeakingThisInConstructor" is definitely not a current lint category for
javac. I've replaced it with "this-escape".
I ran across another one as well: "OverridableMethodCallInConstructor" (in
src/demo/share/jfc/Notepad/Notepad.java).
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:
#SUPPRESS #FILES MODULE
207 3076 java.base
0 128 java.compiler
4 18 java.datatransfer
475 2899 java.desktop
0 10 java.instrument
9 23 java.logging
52 330 java.management
0 16 java.management.rmi
15 199 java.naming
32 142 java.net.http
0 20 java.prefs
14 106 java.rmi
0 15 java.scripting
0 1 java.se
46 216 java.security.jgss
1 30 java.security.sasl
0 23 java.smartcardio
13 77 java.sql
6 56 java.sql.rowset
0 5 java.transaction.xa
277 1848 java.xml
70 271 java.xml.crypto
3 20 jdk.accessibility
1 21 jdk.attach
1 18 jdk.charsets
84 333 jdk.compiler
2 76 jdk.crypto.cryptoki
4 35 jdk.crypto.ec
1 11 jdk.crypto.mscapi
7 68 jdk.dynalink
0 3 jdk.editpad
120 942 jdk.hotspot.agent
5 56 jdk.httpserver
0 5 jdk.incubator.concurrent
0 50 jdk.incubator.vector
0 3 jdk.internal.ed
8 61 jdk.internal.jvmstat
16 113 jdk.internal.le
4 52 jdk.internal.opt
5 209 jdk.internal.vm.ci
0 1 jdk.internal.vm.compiler
0 1 jdk.internal.vm.compiler.management
0 18 jdk.jartool
20 228 jdk.javadoc
1 41 jdk.jcmd
44 64 jdk.jconsole
23 124 jdk.jdeps
44 254 jdk.jdi
0 1 jdk.jdwp.agent
0 263 jdk.jfr
2 77 jdk.jlink
5 80 jdk.jpackage
8 88 jdk.jshell
0 4 jdk.jsobject
2 11 jdk.jstatd
2 281 jdk.localedata
1 25 jdk.management
0 19 jdk.management.agent
0 15 jdk.management.jfr
1 16 jdk.naming.dns
0 8 jdk.naming.rmi
0 11 jdk.net
0 2 jdk.nio.mapmode
0 11 jdk.random
1 37 jdk.sctp
0 30 jdk.security.auth
0 16 jdk.security.jgss
0 9 jdk.unsupported
0 8 jdk.unsupported.desktop
0 94 jdk.xml.dom
1 14 jdk.zipfs
-Archie
--
Archie L. Cobbs
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/amber-dev/attachments/20230104/e483e8f8/attachment-0001.htm>
More information about the amber-dev
mailing list