Inference failure raised from dataflow is reported against an unrelated file, and can be the only diagnostic

[vlsi]

Version

NullAway 0.14.0, and current master (21f02dd). Error Prone 2.50.0, JDK 21.

Flags: -XepOpt:NullAway:JSpecifyMode=true -XepOpt:NullAway:WarnOnGenericInferenceFailure=true -XDaddTypeAnnotationsToSymbol=true.

Summary

An inference failure raised while dataflow is running is attributed to an unrelated compilation unit. This is the defect in #1725, but with two additions that change what it costs a user:

1. It can be the only diagnostic in the build. In WarnOnGenericInferenceFailure redundantly reporting in wrong file #1725 the misplaced report sits next to a correct one, so it reads as redundant noise. When the failure is raised only from dataflow (calledFromDataflow=true), nothing else is emitted, and the build fails with a single error naming a file that has nothing to do with the cause.
2. The stale path is not the cause. Instrumenting the failure branch shows the TreePath, the VisitorState path, and javac's current source file all pointing at the right file, while the diagnostic still lands in another one. The JSpecify mode: NPE in CFGTranslationPhaseOne.getReceiver for a lambda nested inside a lambda passed to a generic method #1680 fix in AccessPathNullnessPropagation.genericReturnIsNullable updates the path, but the DescriptionListener the VisitorState carries still belongs to the compilation unit where it was created, and that is what decides the file a diagnostic is attributed to.

Because the tree's character offset is then interpreted against the wrong file, the position lands past its end. The reported line is that file's last line plus one, and Error Prone prints a blank line where the source snippet would be.

Reproducer

// Unrelated.java
class Unrelated {}

// Caller.java
@org.jspecify.annotations.NullMarked
class Caller {
  <T> T call(java.util.function.Supplier<T> s) { return s.get(); }
  void f() {
    call(() -> { return ""; });
    call(() -> { return null; });
  }
}

/Unrelated.java:3: warning: [NullAway] inference failure: type variable T constrained to be both @NonNull and @Nullable

    (see http://t.uber.com/nullaway )
/Caller.java:6: warning: [NullAway] inference failure: type variable T constrained to be both @NonNull and @Nullable
    call(() -> { return null; });
        ^

Unrelated.java is one line long and the report is on line 3, with no source snippet. This is #1725's reproducer; it is included because it is the smallest case, and because the real-world one below could not be reduced to it.

The case where it is the only diagnostic

Apache Calcite at apache/calcite#5213, commit dd0c99af0e, ./gradlew -Pwerror=false -PenableErrorprone :core:compileJava:

core/src/main/java/org/apache/calcite/interpreter/MatchNode.java:41: error: [NullAway] inference failure: type variable R constrained to be both @NonNull and @Nullable

1 error

MatchNode.java is 40 lines long, and that is the whole NullAway output for the module.

Adding @SuppressWarnings("NullAway") to MatchNode moves the report to AbstractSingleNode.java:41 (a 40-line file), then to Node.java:28 (a 27-line file). The position is always some file's last line plus one.

I patched the failure branch in GenericsChecks to print what it was working with, published that build, and ran the same compile:

NULLAWAY-DBG inferenceFailure calledFromDataflow=true
  stateFile=.../org/apache/calcite/sql/validate/AggChecker.java
  pathFile=.../org/apache/calcite/sql/validate/AggChecker.java
  logCurrentSourceFile=SimpleFileObject[.../org/apache/calcite/sql/validate/AggChecker.java]
  callTree=call.accept(this)

One failure in the entire compilation, raised from dataflow, for a call in AggChecker.java — reported against MatchNode.java. All three of the path, the state's path and javac's current source file are correct at that moment, which is what rules out the stale path.

The finding itself is real. AggChecker extends SqlBasicVisitor<@Nullable Void> but declares its override as public Void visit(SqlIdentifier id), narrowed to non-null, so return call.accept(this); returns @Nullable Void into a non-null return type. A user has no way to learn that from the message they get.

I also ruled out the generated-code options as the reason the correct report is missing: rebuilding with TreatGeneratedAsUnannotated=false and disableWarningsInGeneratedCode(false) still yields exactly one error, in the same place.

Expected

The diagnostic is attributed to the file that contains the call.

Workaround

None that finds the cause. WarnOnGenericInferenceFailure=false silences it, at the price of losing every real inference failure.

Where we hit it

Replacing the Checker Framework with NullAway in Apache Calcite (apache/calcite#5213).

[vlsi]

A workaround exists on the Calcite side, and it is a real fix rather than a suppression.

The failure comes from AggChecker, which extends SqlBasicVisitor<@Nullable Void> and narrows its override to a non-null return type:

@Override public Void visit(SqlIdentifier id) {   // line 105
  ...
  return call.accept(this);                       // line 125, returns @Nullable Void
}

Giving the override the return type the base class already has removes the failure:

@Override public @Nullable Void visit(SqlIdentifier id) {

With NullAway master (no fix), ./gradlew -Pwerror=false -PenableErrorprone :core:compileJava then reports no NullAway error at all.

Finding that line without the fix is the hard part, and the obvious move does not work: @SuppressWarnings("NullAway") on the file the diagnostic names only moves the report to the next file. Three things do work:

1. Build NullAway with the fix locally (publishToMavenLocal, then -PenableMavenLocal -Pnullaway.version=…) and use it once, to read the real location.
2. Bisect the compilation set. The report lands in the first file of whatever set is compiled, so the cause is narrowed by which subsets still produce it.
3. Set WarnOnGenericInferenceFailure to false, at the price of every real inference failure.

Where the report lands is deterministic rather than random: it goes to the compilation unit that created the cached dataflow analysis, which is the first file of the javac task. In Calcite that is stably MatchNode.java, because :core:compileJava always recompiles in full — the -i log says Full recompilation is required because org.immutables.processor.ProxyProcessor is not incremental. It moves when the set or the order of sources changes: a new file earlier in the order, a suppression in the current victim, a subset compiled from an IDE.

The part worth flagging for anyone hitting this: until the fix lands, every finding of this class arrives as "one error in MatchNode.java:41, no snippet", wherever it actually comes from. core has around twenty narrowing public Void visit(...) overrides; only AggChecker fires today, because only there the result of accept(this) is returned into a non-null position.