Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 21:33:02 GMT, Archie L. Cobbs wrote:
>> Sounds good - thanks.
>
> Ah. I just realized that we need to do it your way because of the following
> bug:
>
> Suppose you have a constructor and a field with initializer that both leak,
> but you have `@SuppressWarnings("this-escape")` on the constructor.
>
> Then the leak for the field will never be reported because we never get to it.
>
> I'll fix.
yep, right in that case that leak wouldn't be reported
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 16:20:41 GMT, Archie L. Cobbs wrote:
>> I'm OK either way we can revisit this later either as part of this PR or in
>> a future one. I let it to your consideration
>
> Sounds good - thanks.
Ah. I just realized that we need to do it your way because of the following bug:
Suppose you have a constructor and a field with initializer that both leak, but
you have `@SuppressWarnings("this-escape")` on the constructor.
Then the leak for the field will never be reported because we never get to it.
I'll fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 17:49:05 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 685:
>>
>>> 683:
>>> 684: @Override
>>> 685: public void visitDoLoop(JCDoWhileLoop tree) {
>>
>> I was thinking, code can also loop using labels and `break` / `continue`,
>> not something we need to cover as part of this prototype but could be a
>> future TODO that we can document
>
> Hah - I didn't think of that. But actually I don't think we would miss
> anything (see if you agree).
>
> The code "executes" every loop, in a sort-of simulation, adding references
> until the set of references converges. Moreover, that reference set is
> "append only" while this is happening.
>
> Therefore, during actual execution, a break or continue may cause less code
> to be executed than in our simulation, but never more code than our
> simulation. So during actual execution it might be that fewer actual 'this'
> references are created, but never more.
>
> Therefore, this effect might cause false positives (which of course we
> already have with loops and code in general because we take all possible
> branches), but never false negatives.
yep I agree
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 17:35:08 GMT, Vicente Romero wrote:
>> 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 685:
>
>> 683:
>> 684: @Override
>> 685: public void visitDoLoop(JCDoWhileLoop tree) {
>
> I was thinking, code can also loop using labels and `break` / `continue`, not
> something we need to cover as part of this prototype but could be a future
> TODO that we can document
Hah - I didn't think of that. But actually I don't think we would miss anything
(see if you agree).
The code "executes" every loop, in a sort-of simulation, adding references
until the set of references converges. Moreover, that reference set is "append
only" while this is happening.
Therefore, during actual execution, a break or continue may cause less code to
be executed than in our simulation, but never more code than our simulation. So
during actual execution it might be that fewer actual 'this' references are
created, but never more.
Therefore, this effect might cause false positives (which of course we already
have with loops and code in general because we take all possible branches), but
never false negatives.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 GMT, Archie L. Cobbs 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.
> -
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 16:12:50 GMT, Vicente Romero wrote: >> Yes... I did it that way is so that it doesn't require any adaptation >> if/when JDK-8194743 ever gets implemented. And it keeps the code a little >> simpler in exchange for a little redundancy. >> >> I'm happy to fix this if you think it is necessary though. > > I'm OK either way we can revisit this later either as part of this PR or in a > future one. I let it to your consideration Sounds good - thanks. - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 15:14:05 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 516:
>>
>>> 514: Name name = TreeInfo.name(invoke.meth);
>>> 515: if (name == names._super) {
>>> 516: scanInitializers();
>>
>> it seems like the code scan initializers every time it finds a super()
>> invocation, I guess that this scanning could be done once per class
>
> Yes... I did it that way is so that it doesn't require any adaptation if/when
> JDK-8194743 ever gets implemented. And it keeps the code a little simpler in
> exchange for a little redundancy.
>
> I'm happy to fix this if you think it is necessary though.
I'm OK either way we can revisit this later either as part of this PR or in a
future one. I let it to your consideration
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 12:42:24 GMT, Vicente Romero wrote:
>> 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 516:
>
>> 514: Name name = TreeInfo.name(invoke.meth);
>> 515: if (name == names._super) {
>> 516: scanInitializers();
>
> it seems like the code scan initializers every time it finds a super()
> invocation, I guess that this scanning could be done once per class
Yes... I did it that way is so that it doesn't require any adaptation if/when
JDK-8194743 ever gets implemented. And it keeps the code a little simpler in
exchange for a little redundancy.
I'm happy to fix this if you think it is necessary though.
> 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`
Thanks - will fix.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 GMT, Archie L. Cobbs 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.
> -
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
On Fri, 13 Jan 2023 04:04:36 GMT, Archie L. Cobbs 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.
> -
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v8]
> 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
