> 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

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/11874/files
  - new: https://git.openjdk.org/jdk/pull/11874/files/6e96a7d7..edf3c3f5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=07
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11874&range=06-07

  Stats: 540 lines in 8 files changed: 120 ins; 92 del; 328 mod
  Patch: https://git.openjdk.org/jdk/pull/11874.diff
  Fetch: git fetch https://git.openjdk.org/jdk pull/11874/head:pull/11874

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

Reply via email to