CSR :
|
Summary ------- The addition of a new `this-escape` lint warning to the compiler. Problem ------- The Java language requires subclass constructors to invoke a superclass constructor before performing any other work, but superclass constructors are allowed to invoke instance methods as part of their initialization. Therefore, if a superclass constructor invokes an instance method that a subclass overrides, then that method will be invoked before the subclass has done any of its own initialization, possibly leading to unexpected behavior. The following class demonstrates the problem: import java.util.*; import java.util.function.*; /** * A {@link Set} that only allows elements matching some {@link Predicate}. */ public class FilteredSet<E> extends HashSet<E> { private final Predicate<? super E> filter; public FilteredSet(Predicate<? super E> filter, Collection<? extends E> elems) { super(elems); this.filter = filter; } @Override public boolean add(E elem) { if (!this.filter.test(elem)) throw new IllegalArgumentException("disallowed element"); return super.add(elem); } public static void main(String[] args) { new FilteredSet<>(s -> true, Arrays.asList("abc", "def")); // NullPointerException } } The above example appears bug-free at first glance, but it actually throws a NullPointerException if run. Moreover, how this error happens is due to a tricky interplay of three different classes: 1. The `FilteredSet` constructor invokes the `HashSet(Collection)` superclass constructor; 1. `HashSet(Collection)` constructor invokes `AbstractCollection.addAll()`; 1. `AbstractCollection.addAll()` invokes `add()` for each element; 1. `FilteredSet.add()` invokes `this.filter.test()`, but `this.filter` is still null. This bug is possible because the `HashSet(Collection)` constructor has a 'this' escape. This term refers to any time a reference to this "escapes" from a superclass constructor's control to some other code before it has been fully initialized. The example above illustrates a key difficulty with 'this' escape bugs, which is that they typically involve an interplay among multiple source files. Inspecting any one file at a time does not reveal the bug. Another example of a subtle 'this' escape bug would be a superclass constructor that adds `this` to some `HashSet`, perhaps when registering itself as a listener. If a subclass overrides `hashCode()` to include one or more of its own fields, then the `HashSet` will record the object under the wrong hash value, because at that time those subclass fields won't be initialized yet. Solution -------- Update the Java compiler so that it can generate warnings about possible 'this' escapes. As with various other similar warnings, to enable 'this' escape detection, one would include the `-Xlint:this-escape` flag (or `Xlint:all`) on the command line, and to suppress it locally, one would use `@SuppressWarnings("this-escape")`. Perfect analysis is not possible, so to the extent the analysis must be imperfect, it is preferable for it to err on the side of generating false positives rather than false negatives. This way, if your code does not generate any warnings, then you can feel confident that there is a high probability that there are no leaks. This is analogous to how you can be confident that you won't get `ClassCastExceptions` at runtime if your code doesn't generate any `unchecked` warnings at compile time. Specification ------------- The 'this' escape warning only looks for "escapes" in which some _subclass_ might still be uninitialized. It doesn't look for escapes in which the class doing the leaking might itself be uninitialized. In other words, the warning assumes any individual constructor knows what it's doing. Instead, it only looks for potential problems that necessarily span multiple classes. In fact, the boundary is actually larger than that: potential problems must span multiple *source code files*. Warnings are only reported for constructors that could be invoked from some subclass defined _in a different file_. So, for example, `private` constructors and constructors in `private` classes don't generate warnings. This design choice reflects the "key difficulty" of 'this' escape bugs mentioned above. The analysis covers constructors, fields with initializers, and non-static initialization blocks. As with other similar warnings, `@SuppressWarnings("this-escape")` can be used to suppress this warning as needed. This suppression will work at the class, constructor, or field level (for fields with initializers). Unfortunately, to cover leaks from initialization blocks, there's nowhere to put a `@SuppressWarnings("this-escape")` except on the containing class. Implementation ------------- This warning is prototyped and discussed [here][1]. You can see some examples of it being used in the [unit tests][2]. Enabling this warning causes several JDK modules to fail to build, because they are all compiled with `-Xlint:all`. As far as I can tell, almost all of the warnings are valid (i.e., true positives). To fix the build, the corresponding make/build files are updated to exclude `this-escape`. From discussion on the PR, this choice was preferable to the alternative, which is adding `@SuppressWarnings("this-escape")` everywhere, which increased the number of files touched in the patch to 1200+ (that option is archived [here][3]). [1]: https://github.com/openjdk/jdk/pull/11874 [2]: https://github.com/archiecobbs/jdk/tree/ThisEscape/test/langtools/tools/javac/warnings/ThisEscape [3]: https://github.com/openjdk/jdk/compare/master...archiecobbs:jdk:ThisEscapeAnnotations