Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8299995

Add lint check for calling overridable methods from a constructor

    XMLWordPrintable

Details

    • CSR
    • Resolution: Approved
    • P4
    • 21
    • tools
    • None
    • behavioral
    • low
    • Anyone who compiles code with `-Xlint:all` may have their builds start failing due to one or more new warnings being generated.
    • Java API, add/remove/modify command line option
    • JDK

    Description

      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;
      2. HashSet(Collection) constructor invokes AbstractCollection.addAll();
      3. AbstractCollection.addAll() invokes add() for each element;
      4. 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 modules (or packages if not using modules). Warnings are only reported for constructors that could be invoked from some subclass defined in a different module. So, for example, private constructors and constructors in non-public classes don't generate warnings. This design choice reflects the "key difficulty" of 'this' escape bugs mentioned above.

      Note: Choosing the module boundary here instead of, for example, the source file boundary, is a conservative choice to keep the warning from being too "aggressive" initially. The thought here is that any new lint option runs the risk of turning off developers if it generates too many warnings in their existing code (even if the warnings are valid), making them want to just disable it completely. As we gain experience over time, we can tighten the screws so to speak, perhaps in conjunction with increasing sophistication in the analysis.

      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.

      Changes to the module-info.java Javadoc for module jdk.compiler:

      diff --git a/src/jdk.compiler/share/classes/module-info.java b/src/jdk.compiler/share/classes/module-info.java
      index 7974f8cd6a3..142a185a050 100644
      --- a/src/jdk.compiler/share/classes/module-info.java
      +++ b/src/jdk.compiler/share/classes/module-info.java
      @@ -183,6 +183,7 @@ import javax.tools.StandardLocation;
        * <tr><th scope="row">{@code strictfp}             <td>unnecessary use of the {@code strictfp} modifier
        * <tr><th scope="row">{@code synchronization}      <td>synchronization attempts on instances of value-based classes
        * <tr><th scope="row">{@code text-blocks}          <td>inconsistent white space characters in text block indentation
      + * <tr><th scope="row">{@code this-escape}          <td>superclass constructor leaking {@code this} before subclass initialized
        * <tr><th scope="row">{@code try}                  <td>issues relating to use of {@code try} blocks
        *                                                      (that is, try-with-resources)
        * <tr><th scope="row">{@code unchecked}            <td>unchecked operations

      Changes to the javac(1) manual page in module jdk.compiler.

      • Add to section "Extra Options" under Xlint:

              · this‐escape:  Warns  about  constructors leaking this prior to
                subclass initialization.
      • Add to section "EXAMPLES OF USING ‐XLINT KEYS":

        this−escape
           Warns about constructors leaking this prior to subclass initial‐
           ization.  For example, this class:
        
                  public class MyClass {
                    public MyClass() {
                      System.out.println(this.hashCode());
                    }
                  }
        
           generates the following warning:
        
                  MyClass.java:3: warning: [this‐escape] possible ’this’ escape
                                           before subclass is fully initialized
                      System.out.println(this.hashCode());
                                                      ^
        
           A  ’this’  escape  warning  is generated when a constructor does
           something that might result in a subclass method  being  invoked
           before  the  constructor  returns.   In  such cases the subclass
           method would be operating on  an  incompletely  initialized  in‐
           stance.   In the above example, a subclass of MyClass that over‐
           rides hashCode() to incorporate its own fields would likely pro‐
           duce an incorrect result when invoked as shown.
        
           Warnings  are  only  generated if a subclass could exist that is
           outside of the current module (or package, if no  module)  being
           compiled.  So, for example, constructors in final and non‐public
           classes do not generate warnings.

      Attachments

        Issue Links

          Activity

            People

              acobbs Archie Cobbs
              darcy Joe Darcy
              Vicente Arturo Romero Zaldivar
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: