On Sat, 7 Jan 2023 21:08:07 GMT, Archie L. Cobbs <d...@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
