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

[Maurizio Cimadamore]

On Sat, 7 Jan 2023 21:08:07 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> This PR adds a new lint warning category `this-escape`.
>> 
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to allow the JDK to continue to compile with `-Xlint:all`.
>> 
>> A 'this' escape warning is generated for a constructor `A()` in a class `A` when the compiler detects that the following situation is _in theory possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could get invoked, perhaps indirectly
>> 
>> In the above scenario, `B.foo()` would execute before `A()` has returned and before `B()` has performed any initialization. To the extent `B.foo()` accesses any fields of `B` - all of which are still uninitialized - it is likely to function incorrectly.
>> 
>> Note, when determining if a 'this' escape is possible, the compiler makes no assumptions about code outside of the current compilation unit. It doesn't look outside of the current source file to see what might actually happen when a method is invoked. It does follow method and constructors within the current compilation unit, and applies a simplified union-of-all-possible-branches data flow analysis to see where 'this' could end up.
>> 
>> From my review, virtually all of the warnings generated in the various JDK modules are valid warnings in the sense that a 'this' escape, as defined above, is really and truly possible. However, that doesn't imply that any bugs were found within the JDK - only that the possibility of a certain type of bug exists if certain superclass constructors are used by someone, somewhere, someday.
>> 
>> For several "popular" classes, this PR also adds `@implNote`'s to the offending constructors so that subclass implementors are made aware of the threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>> 
>> More details and a couple of motivating examples are given in an included [doc file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html) that these `@implNote`'s link to. See also the recent thread on `amber-dev` for some background.
>> 
>> Ideally, over time the owners of the various modules would review their `@SuppressWarnings("this-escape")` annotations and determine which other constructors also warranted such an `@implNote`.
>> 
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch of different JDK modules. My apologies for that. Adding these annotations was determined to be the more conservative approach, as compared to just excepting `this-escape` from various module builds globally.
>> 
>> **Patch Navigation Guide**
>> 
>> * Non-trivial compiler changes:
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>>     * `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>> 
>> * Javadoc additions of `@implNote`:
>> 
>>     * `src/java.base/share/classes/java/io/PipedReader.java`
>>     * `src/java.base/share/classes/java/io/PipedWriter.java`
>>     * `src/java.base/share/classes/java/lang/Throwable.java`
>>     * `src/java.base/share/classes/java/util/ArrayDeque.java`
>>     * `src/java.base/share/classes/java/util/EnumMap.java`
>>     * `src/java.base/share/classes/java/util/HashSet.java`
>>     * `src/java.base/share/classes/java/util/Hashtable.java`
>>     * `src/java.base/share/classes/java/util/LinkedList.java`
>>     * `src/java.base/share/classes/java/util/TreeMap.java`
>>     * `src/java.base/share/classes/java/util/TreeSet.java`
>> 
>> * New unit tests
>>     * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>> 
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix incorrect @bug numbers in unit tests.

src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2345:

> 2343:         if (!innerType.hasTag(CLASS) || !outerType.hasTag(CLASS))
> 2344:             return false;
> 2345:         innerType = erasure(innerType);

The javac way to write this would be either to compare types using `Types.isSameType` or to compare the underlying class symbols with == (as class symbols are shared across multiple type instances).

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 85:

> 83:  *
> 84:  * <p>
> 85:  * When tracking references, we distinguish between direct references and indirect references,

I'm trying to understand the code and it's not obvious to me as to where this difference comes into play. I believe (but I'm not sure) the spirit is to treat reference to `this` in the original constructor as "direct" while treat a reference to a parameter "a" in a method called from the constructor, where "a" is assigned "this", as indirect?

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 1098:

> 1096:     private <T extends JCTree> void visitLooped(T tree, Consumer<T> visitor) {
> 1097:         this.visitScoped(tree, false, t -> {
> 1098:             while (true) {

Why is this needed? Can the tracking state of `this` change after multiple "execution" of the same code?

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

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