On Fri, 13 Jan 2023 22:48:59 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 two > additional commits since the last revision: > > - Fix bug where field initializer warnings could be incorrectly suppressed. > - Consolidate all the unit tests that generate warnings into one. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 293: > 291: return !currentClass.sym.isFinal() && > 292: !(currentClass.sym.isSealed() && > currentClass.permitting.isEmpty()) && > 293: !(currentClass.sym.owner.kind == MTH) && Maybe `isDirectlyOrIndirectlyLocal`? Suggestion: !currentClass.sym.isDirectlyOrIndirectlyLocal() (Or enhance the `privateOuter` flag to also handle local classes? something like `tree.sym.kind.matches(Kinds.KindSelector.VAL_MTH)`.) src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 298: > 296: }.scan(env.tree); > 297: > 298: // TODO: eliminate sealed classes where all permitted subclasses > are in this compilation unit +1 on better check for permitted (shouldn't be too difficult, or am I mistaken?) src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 327: > 325: JCBlock block = (JCBlock)defs.head; > 326: visitTopLevel(klass, () -> > analyzeStatements(block.stats)); > 327: continue; Nit: this continue is unnecessary; maybe use `instanceof JCBlok block` instead of `.hasTag(BLOCK)` (inside javac, where we know the implementation classes, we decided to do pattern matching `instanceof`, I believe). ------------- PR: https://git.openjdk.org/jdk/pull/11874
