Annotate Gatherer APIs.

[cpovirk]

No description provided.

[wmdietl]

These look like they were missed, but I don't know this API:

default <RR> Gatherer<T, ?, RR> andThen(Gatherer<? super R, ?, ? extends RR> that) on line 274 in Gatherer: shouldn't RR allow null?
static <A> Supplier<A> defaultInitializer() on line 289 and static <A> BinaryOperator<A> defaultCombiner() on line 304 in Gatherer: should A allow null?
static <A, R> BiConsumer<A, Downstream<? super R>> defaultFinisher() { on line 321 in Gatherer: A and R should be nullable?

[cpovirk]

Thanks, I had totally missed those.

andThen looks straightforward, assuming that I haven't been missing anything big throughout this PR.

The others are more interesting. The implementations of default* methods all look capable of handling null—or, in the case of the BinaryOperator for combiner(), as incapable of handling null as of any other value (and I think this point is that the Gatherer infrastructure treats combiner() as a sentinel that says not to invoke the method). There are two things that make that interesting:

• First, the initializer() returns null. I think that means that we actually want to return a Supplier<@Nullable A>.
• Second, the Gatherer API doesn't do PECS. That means that we probably shouldn't force other methods (like defaultCombiner()) to return a more general type, which is to say BinaryOperator<@Nullable A> (since we can't change it to the fully general type BinaryOperator<@Nullable Object>, since that would require a change to the base type). So I think it makes sense to let callers pick their nullness, just as they can pick the base type.

I have never actually used these APIs, so I could well still be missing things. I would be OK with waiting to merge this until I can justify taking the time to investigate more deeply, or we could go for it and figure that we'll get feedback if users encounter actual problems.

[wmdietl]

Sorry, I had missed that this was updated. I merged in main and things look good to me.

[msridhar]

@cpovirk are we good to merge this one? Or are we holding off until someone can do a deeper investigation?

[cpovirk]

Thanks for checking. (And thanks for all the other reviews!) I have been holding off until we can take a closer look. At this point, I don't remember even the level of detail in my comment above. (That might be a good thing, since I could try annotating the API again someday with fresh eyes and then see if the results match my earlier attempt :)) I don't have any specific plans to come back to this, though.

[cpovirk]

I pointed Gemini at this PR. It suggested that I use <A extends @Nullable Object> on defaultInitializer(), even though the return type is already (appropriately) Supplier<@Nullable A>. It cited some precedent, but I contended that that precedent doesn't quite hold here and that Gemini was neglecting the possibility of calling Gatherers.<@NonNull T>defaultInitializer() (or hopefully having that type inferred automatically by a nullness checker). It signed onto my counterarguments once I laid them out.

It's hard to know precisely how much of an endorsement to view all that as, but clearly it's better than if Gemini had immediately identified some obvious problems :)

Overall, the annotations across Gatherer, Gatherers, and Stream.gather are very well-matched to both JSpecify's rules and the runtime behavior of java.util.stream.

There is one small improvement worth considering on Gatherer.defaultInitializer(), and the rest of the design choices hold up on close inspection.

---

1. Recommended Change: Upper bound of <A> on Gatherer.defaultInitializer()

Currently, line 287 of Gatherer.java is:

static <A> Supplier<@Nullable A> defaultInitializer()

Compare this with:

static <A extends @Nullable Object> Supplier<@Nullable A> defaultInitializer()

(analogous to how Comparator.nullsFirst and Objects.compare declare <T extends @Nullable Object> even when T appears as @Nullable T in the signature).

Why <A extends @Nullable Object> is preferable here while still keeping Supplier<@Nullable A>:

1. Still enforces nullability of the supplied state: Returning Supplier<@Nullable A> still prevents a caller from assigning Gatherer.defaultInitializer() to a Supplier<NonNullState> (or passing it to Gatherer.of(defaultInitializer(), integrator, ...) with an Integrator<NonNullState, T, R>), because @Nullable A is always a nullable type regardless of whether A is instantiated as State or @Nullable State.
2. Allows explicit or inferred nullable type arguments: With <A> bounded by non-null Object, an explicit call like Gatherer.<@Nullable Void>defaultInitializer() or Gatherer.<A>defaultInitializer() (where A is a type variable with bound A extends @Nullable Object, as in Gatherer.initializer() or Gatherer.ofSequential when matching target type Supplier<@Nullable Void>) violates A's upper bound in checkers that validate type arguments against bounds. Adding extends @Nullable Object avoids this while preserving the exact same return-type guarantee.

---

2. Verification of the Other Key Decisions

A. defaultInitializer(), defaultCombiner(), and defaultFinisher()

Your reasoning in PR #79 holds up when verified against Gatherers.Value.DEFAULT, Gatherers.Composite, and GathererOp:

• Why defaultInitializer() must return Supplier<@Nullable A>: Whenever a gatherer's initializer is defaultInitializer(), GathererOp.GatherSink.begin and GathererOp.Sequential elide calling initializer.get() and leave A state as null (and if defaultInitializer().get() is called directly, Gatherers.Value.DEFAULT.get() returns null). That null state is then passed directly to integrator.integrate(state, ...) and finisher.accept(state, ...). Having defaultInitializer() return Supplier<@Nullable A> ensures that Gatherer.of(defaultInitializer(), ...) cannot infer a non-null state type A.
• Why Gatherer.initializer() must stay Supplier<A> (not Supplier<@Nullable A>): The four methods on Gatherer<T, A, R> (initializer(), integrator(), combiner(), finisher()) all share A. If initializer() returned Supplier<@Nullable A>, then for a stateful Gatherer<T, NonNullState, R>, A state = gatherer.initializer().get() could not be passed to gatherer.integrator().integrate(state, t, downstream) (as in the class Javadoc snippet at Gatherer.java:78-84). When a gatherer is stateless, its A type argument itself should be nullable (such as @Nullable Void).
• Why defaultCombiner() and defaultFinisher() use A (with <A extends @Nullable Object>) rather than @Nullable A: Unlike defaultInitializer(), defaultCombiner() and defaultFinisher() are routinely paired with a stateful, non-null initializer (e.g., Gatherer.of(State::new, integrator, Gatherer.defaultCombiner(), Gatherer.defaultFinisher())). Because Gatherer.of takes Supplier<A>, BinaryOperator<A>, and BiConsumer<A, Downstream<? super R>> without wildcards on A, returning BinaryOperator<@Nullable A> or BiConsumer<@Nullable A, ...> would force A to be @Nullable even when State::new produces a non-null State. Using <A extends @Nullable Object> lets A be either non-null or @Nullable.

B. @Nullable Void on Stateless Factory Methods (Gatherer.of / Gatherer.ofSequential)

• In Gatherer.ofSequential and Gatherer.of, replacing Void with @Nullable Void in Gatherer<T, @Nullable Void, R>, Integrator<@Nullable Void, T, R>, and BiConsumer<@Nullable Void, Downstream<? super R>> is necessary and accurate:
  • The state parameter passed to the user's Integrator and BiConsumer at runtime is null.
  • Because A in Gatherer<T, A, R> has upper bound @Nullable Object, Gatherer<T, @Nullable Void, R> is a subtype of Gatherer<T, ?, R> (since an unbounded wildcard ? in @NullMarked code inherits the @Nullable Object upper bound of its corresponding type parameter), so it composes cleanly with Stream.gather and Gatherer.andThen.

C. Built-in Gatherers in Gatherers

• windowFixed and windowSliding: Both store input elements into an Object[] and construct the output List<TR> via SharedSecrets.getJavaUtilCollectionAccess().listFromTrustedArrayNullsAllowed(...). Thus <TR extends @Nullable Object> Gatherer<TR, ?, List<TR>> accurately reflects that the windows themselves are non-null List instances whose element nullness matches TR.
• fold, scan, and mapConcurrent: All three validate their function arguments with Objects.requireNonNull and pass input (T) and output (R) elements through without dereferencing them (in mapConcurrent, FutureTask<R> permits null results). Annotating <T extends @Nullable Object, R extends @Nullable Object> on all three is accurate.

---

You're right—that distinction holds, and defaultInitializer() belongs in the WeakReference<T> / Equivalence<T> (jspecify#78) bucket rather than the Comparator.nullsFirst bucket.

Why defaultInitializer() matches WeakReference / Equivalence

1. A only appears as @Nullable A:
   In Comparator.nullsFirst (<T extends @Nullable Object> Comparator<@Nullable T> nullsFirst(@Nullable Comparator<? super T> comparator)), T also appears unannotated in Comparator<? super T>, so <T extends @Nullable Object> is needed to allow passing a Comparator<@Nullable Foo> without a bound mismatch.
   In static <A> Supplier<@Nullable A> defaultInitializer(), A appears only once and is already @Nullable A. Instantiating A with Foo and with @Nullable Foo would produce the exact same return type (Supplier<@Nullable Foo>), so leaving <A> with a non-null upper bound avoids giving callers two ways (Gatherer.<Foo>defaultInitializer() and Gatherer.<@Nullable Foo>defaultInitializer()) to express the same type.

2. How callers interact with <A> in practice:

   • Unqualified calls (Gatherer.defaultInitializer()): When passed to Gatherer.of(defaultInitializer(), ...) where the target type is Supplier<@Nullable Void> (or Supplier<@Nullable State>), the constraint @Nullable A = @Nullable Void is satisfied by A = Void (non-null), which meets A extends Object.
   • Explicit type arguments: A caller writing an explicit type argument for a concrete type writes Gatherer.<Void>defaultInitializer() (getting back Supplier<@Nullable Void>), and only a caller forwarding a nullable-bounded type variable A extends @Nullable Object explicitly would need Gatherer.<@NonNull A>defaultInitializer() (or to rely on target-type inference, as Gatherer.initializer() and Gatherer.ofSequential() do). That is the exact same tradeoff made for WeakReference<T> and Equivalence<T>.

With that accounted for, I don't see any changes needed in the branch relative to main—the PR looks ready as-is.