On Mon, 16 Jan 2023 23:22:00 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 three > additional commits since the last revision: > > - Remove unused type variable on method visitScoped(). > - Remove expression type filtering; it doesn't seem to be needed. > - Clean up unused import. I was looking more at the warnings generated with the Lint enabled - and found this one, which is interesting (src/classes/build/tools/cldrconverter/Bundle.java): private static final Map<String, Bundle> bundles = new HashMap<>(); ... Bundle(String id, String cldrPath, String bundles, String currencies) { // escaping constructor this.id = id; this.cldrPath = cldrPath; if ("localenames".equals(bundles)) { bundleTypes = EnumSet.of(Type.LOCALENAMES); } else if ("currencynames".equals(bundles)) { bundleTypes = EnumSet.of(Type.CURRENCYNAMES); } else { bundleTypes = Type.ALL_TYPES; } if (currencies == null) { currencies = "local"; } this.currencies = currencies; addBundle(); } private void addBundle() { Bundle.bundles.put(id, this); // escaping here } Now, this reminds me a lot of some of the examples with static factories we discussed last week. The Bundle constructor is non-private, so it gets the analysis. There is a call to the private method `addBundle` - which also gets inspected. This method accesses the static hashmap and then calls "put" on it, passing the `this` parameter. Now, according to my understanding, this is not, strictly speaking, a classic case of escaping this through a virtual method call (which can be overridden in a subclass). Here `this` is just escaping "somewhere" (a static hashmap). Yes, other clients might be able to access the hashmap, and observe a possibly uninitialized value - but this is in the same bucket as the factory example I gave last week, where a private constructor (called by public factory) was leaking `this` via a static method. Given this, I believe the above case is a "false positive". The problem seems to be that we don't know what the code for HashMap::put is, so we cannot know if that code might end up calling some virtual method on our `this` - and so we warn. Correct? It is a bit sad because an idiom such as this is not infrequent - e.g. a constructor setting up some "cached" value somewhere - but, because of the limitations of the analysis, this is treated as a case of escaping. I wonder if leveraging module boundaries here could help: the only way for `Map::put` to leverage some virtual method in `this` (whose type is Bundle) is if if the map implementation can access Bundle - but in this case we know that the map is an HashMap (static final constant) - and HashMap is defined in a module that doesn't have access to Bundle. Actually, in this case, since Bundle is package-private, we know that no code outside this package will be able to do much with it (unless the package also defines a public/protected sublass). I think stuff like this could be used to reduce the rate of false positives when calling methods we can't inspect - but it's probably something that is hard to generalize. While we can reason about module and package boundaries - one thorny problem here is that what Map::put implementation is invoked depends on what map instance is assigned to the final field (because, dynamic dispatch). So, unless the escaping method is static, the analysis required to detect these false positive would likely be prohibitive (and, rarely applicable). As I said, a bit sad, because this idiom is frequent - I counted how many times did a ".put(... this)" occured in the Lint warnings generated by this PR and found 88 matches. Not sky-high, but not negligible either. And the issue here is that the programmer is likely doing "the right" thing, so the warning will be perceived as a tad annoying. ------------- PR: https://git.openjdk.org/jdk/pull/11874
