Inference failure diagnostics name a different type variable between runs

[vlsi]

What happens

For a snippet whose inference constraints are unsatisfiable, which type variable NullAway names in inference failure: type variable %s constrained to be both @NonNull and @Nullable depends on the identity hash codes the JVM handed out earlier in the process. The same source, the same flags and the same NullAway build report T in one run and U in the next.

I hit this while asserting on the full set of diagnostics a test produces: GenericDiamondTests#genericMethodCallWithDiamondConstructorParameter reported type variable T in one full-suite run and type variable U in the next two, with nothing changed in between. Running that test class alone always reports U, which is why the suite has never shown it.

Minimal reproducer

HotSpot's default identity hash (-XX:hashCode=5) comes from a per-thread xorshift generator that advances every time an identity hash is computed. Advancing it a controlled number of times before the compilation reproduces the effect deterministically:

public class InferenceFailureDeterminismTest extends NullAwayTestsBase {

  private static final JavaFileObject SOURCE =
      FileObjects.forSourceLines(
          "Test.java",
          """
          import org.jspecify.annotations.*;
          @NullMarked
          public class Test {
            interface Foo<T extends @Nullable Object> {}
            static class FooImpl<T extends @Nullable Object> implements Foo<T> {
              FooImpl(Foo<T> value) {}
            }
            static Foo<@Nullable String> makeNullableFoo() {
              throw new RuntimeException();
            }
            static <U extends @Nullable Object> Foo<U> id(Foo<U> foo) {
              throw new RuntimeException();
            }
            static void takeFooString(Foo<String> foo) {}
            static void testPositive() {
              takeFooString(id(new FooImpl<>(makeNullableFoo())));
            }
          }
          """);

  @Test
  public void printInferenceFailure() {
    int churn = Integer.getInteger("nullaway.repro.churn", 0);
    for (int i = 0; i < churn; i++) {
      int unused = System.identityHashCode(new Object());
    }
    for (String diagnostic : compile()) {
      if (diagnostic.contains("inference failure")) {
        System.out.println("churn=" + churn + " -> " + diagnostic);
      }
    }
  }

  private List<String> compile() {
    DiagnosticCollector<JavaFileObject> collector = new DiagnosticCollector<>();
    List<String> args =
        new ArrayList<>(
            JSpecifyJavacConfig.withJSpecifyModeArgs(
                Arrays.asList(
                    "-d",
                    temporaryFolder.getRoot().getAbsolutePath(),
                    "-XepOpt:NullAway:OnlyNullMarked=true")));
    args.add("-proc:none");
    new ErrorProneJavaCompiler(ScannerSupplier.fromBugCheckerClasses(NullAway.class))
        .getTask(null, FileManagers.testFileManager(), collector, args, null, List.of(SOURCE))
        .call();
    return collector.getDiagnostics().stream()
        .filter(d -> d.getLineNumber() != Diagnostic.NOPOS)
        .map(d -> d.getMessage(Locale.getDefault()).replaceAll("\\s+", " "))
        .toList();
  }
}

Sweeping the churn count on JDK 21, master at 915b287:

identity hashes computed first	type variable named
0	U
1	U
2	T
3	U
4	T
5	T
20	U
50	T

-XX:+UnlockExperimentalVMOptions -XX:hashCode=<n> moves the answer the same way, and -XX:hashCode=2, which makes every identity hash the constant 1, pins it to U.

Why

ConstraintSolverImpl keys its state on javac Symbol objects, which inherit Object.hashCode:

• ConstraintSolverImpl:73 — private final Map<Element, VarState> vars = new HashMap<>();
• ConstraintSolverImpl:64-65 — final Set<Element> supertypes = new HashSet<>(); and the same for subtypes

solve() seeds its worklist with vars.forEach, so the order in which variables enter the worklist is the iteration order of a HashMap keyed on identity hash codes. updateNullness throws UnsatisfiableConstraintsException(typeVarElement) for whichever variable the propagation reaches first, and GenericsChecks#inferenceFailureMessage puts that variable into the message.

I have not checked whether anything beyond the name in the message can change. Substituting LinkedHashMap and LinkedHashSet would make the order follow constraint-recording order, which is a property of the source rather than of the heap.

Why it is worth fixing

• An error message that changes between builds is hard to trust and hard to search for.
• Two users compiling the same code can get different messages, so a bug report cannot be matched against a build.
• Any test that asserts the exact message becomes flaky, which rules out tightening the diagnostics assertions in this area.

Found while prototyping an exhaustive diagnostics check for the test suite; happy to send a PR if LinkedHashMap/LinkedHashSet is the direction you want.

[msridhar]

Thanks for catching this! Yes, a PR for using LinkedHashMap / LinkedHashSet would be welcome.