NullAway icon indicating copy to clipboard operation
NullAway copied to clipboard

field not initialized warning for an abstract class's field set by the subtype

Open ben-manes opened this issue 9 months ago • 2 comments

When trying to remove NullAway warnings I ran into this case for an embedded class from JCTools. The class uses inheritance to control the data layout for padding to protect against false sharing. The subtype initializes the field in its constructor.

> Task :caffeine:compileJava
/Users/ben/projects/caffeine/caffeine/src/main/java/com/github/benmanes/caffeine/cache/MpscGrowableArrayQueue.java:193: error: [NullAway] @NonNull field producerBuffer not initialized
  protected @Nullable E[] producerBuffer;
                          ^
    (see http://t.uber.com/nullaway )
  Did you mean '@SuppressWarnings("NullAway.Init") protected @Nullable E[] producerBuffer;'?
1 error
abstract class BaseMpscLinkedArrayQueueColdProducerFields<E>
    extends BaseMpscLinkedArrayQueuePad3<E> {
  protected long producerMask;
  protected volatile long producerLimit;
//  @SuppressWarnings("NullAway.Init")
  protected @Nullable E[] producerBuffer;
}

@SuppressWarnings("PMD")
abstract class BaseMpscLinkedArrayQueue<E> extends BaseMpscLinkedArrayQueueColdProducerFields<E> {
  static final VarHandle REF_ARRAY;
  static final VarHandle P_INDEX;
  static final VarHandle C_INDEX;
  static final VarHandle P_LIMIT;

  // No post padding here, subclasses must add

  private static final Object JUMP = new Object();

  /**
   * @param initialCapacity the queue initial capacity. If chunk size is fixed this will be the
   *        chunk size. Must be 2 or more.
   */
  BaseMpscLinkedArrayQueue(int initialCapacity) {
    if (initialCapacity < 2) {
      throw new IllegalArgumentException("Initial capacity must be 2 or more");
    }

    int p2capacity = ceilingPowerOfTwo(initialCapacity);
    // leave lower bit of mask clear
    long mask = (p2capacity - 1L) << 1;
    // need extra element to point at next array
    E[] buffer = allocate(p2capacity + 1);
    producerBuffer = buffer;
    producerMask = mask;
    consumerBuffer = buffer;
    consumerMask = mask;
    soProducerLimit(this, mask); // we know it's all empty to start with
  }

ben-manes avatar Apr 12 '25 03:04 ben-manes

Thanks for the report. We've thought about trying to fix false positives like this before, but it's a bit involved. Basically we'd need to know which inherited fields need to be initialized by a (concrete) class. But to be able to do separate compilation, we'd need to persist that information for each abstract class somewhere, like asking the user to write an annotation like@SubclassInitialized on each such field. I think it'd probably work out, but it hasn't yet been a high priority for us. For now your best bet is probably to suppress.

msridhar avatar Apr 12 '25 14:04 msridhar

Thanks, sgtm! I may consider refactoring the code to use the constructor chain to initialize it which is generally better imho. I am by default a little hesitant since it is complex embedded code and wanted to keep it similar to the original. However I've already had to diverge to use VarHandles because that project has been very reluctant towards switching to them, they prefer using Unsafe despite its terminal deprecation, its mature and fairly inactive at this point, and they did refactor their original code so my fork doesn't line up as neatly anymore anyway. I'd agree this bug is low priority since subtypes initializing the field is not very elegant.

ben-manes avatar Apr 12 '25 18:04 ben-manes