StreamToIterable
Using stream::iterator creates a one-shot Iterable, which may cause surprising failures.

Severity
WARNING
Tags
FragileCode

The problem

Using stream::iterator to create an Iterable results in an Iterable which can only be iterated once, and will throw an IllegalArgumentException on subsequent attempts.

The contract of Iterable is poorly defined, but many APIs will assume that Iterables allow multiple iteration.

To give a concrete example using protocol buffers,

MyProto construct(Stream<Integer> ints) {
  return MyProto.newBuilder()
      .addAllSubMessage(
          ints.map(i -> SubMessage.newBuilder().setVal(i).build())::iterator)
      .build();
}

With the current implementation of the protocol buffer generated code, this will work, but the following minor change will lead to re-iteration and failure,

MyProto construct(Stream<Integer> ints) {
  MyProto.Builder builder = MyProto.newBuilder();
  builder.addSubMessageBuilder().setVal(0);
  return builder
      .addAllSubMessage(
          ints.map(i -> SubMessage.newBuilder().setVal(i).build())::iterator)
      .build(); // build iterates twice, and throws.
}

To avoid such pitfalls, the Stream can either be terminated with forEachOrdered

MyProto construct(Stream<Integer> ints) {
  MyProto.Builder builder = MyProto.newBuilder();
  ints.map(i -> SubMessage.newBuilder().setVal(i).build())
      .forEachOrdered(builder::addSubMessage);
  return builder.build();
}

or collected (with the caveat that the contents of the Stream will now be materialized into memory at once),

MyProto construct(Stream<Integer> ints) {
  return MyProto.newBuilder()
      .addAllSubMessage(
          ints.map(i -> SubMessage.newBuilder().setVal(i).build())
              .collect(toImmutableList()))
      .build();
}

or suppressed using @SuppressWarnings("StreamToIterable"). Only suppress if you’re sure the API you’re using will only iterate the resulting one-shot Iterable once, and can’t accept a Stream or an Iterator.