On Fri, 13 Jan 2023 04:04:36 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 16 additional > commits since the last revision: > > - Fix bug where all but the last yeild statement were being ignored. > - Add method RefSet.mapInto() and use to refactor/clean up. > - Fix possible assertion failure when handling if statements. > - Use Symbol methods isSubClass() and isEnclosedBy() instead of homebrew > stuff. > > Suggested-by: mcimadamore > - Add comment regarding limitations of expresison type filtering. > - Add a few more DISABLED_WARNINGS to unbreak build. > - Clean up handling of switch expressions a bit. > - Remove unused method variant of analyzeTree(). > - Avoid all caps in comments. > - Clarify confusing comment. > - ... and 6 more: https://git.openjdk.org/jdk/compare/6e96a7d7...edf3c3f5 src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java line 533: > 531: if (sym != null && > 532: sym.owner.kind == TYP && > 533: ((ClassSymbol)sym.owner).fullname == names.java_lang_Object > && nit: in general we use another idiom for this in the compiler: sym.owner.type.tsym == syms.objectType.tsym where `syms` is an instance of: `com.sun.tools.javac.code.Symtab` ------------- PR: https://git.openjdk.org/jdk/pull/11874