StructFieldFinder's bound check is off by one, so index == size throws IndexOutOfBoundsException #1068

FieldReference.StructFieldFinder.visit(Type.Struct) guards its field lookup with expr.fields().size() < index, which is off by one — it should reject index >= size. It also has no lower bound. So the intended IllegalArgumentException("Undefined struct type.") fires only for index > size, and the two cases either side of it escape as a bare IndexOutOfBoundsException from List.get.

index == size is the off-by-one a caller is most likely to hit, and it is precisely the case that slips past the guard.

Observed

Dereferencing a struct with a single field:

Reached through any of FieldReference.dereferenceStruct(int), FieldReference.newStructReference(int, Expression), StructField.apply(...) and StructField.constructOnExpression(...), and therefore through FieldReference.ofExpression(...).

By contrast StructField.constructOnRoot(Type.Struct) gets the same check right, and with a message that names the offset and the field count:

if (offset() >= struct.fields().size()) {
  throw new IllegalArgumentException(
      String.format(
          "Field reference offset (%s) must be less than number of fields in struct (%s)",
          offset(), struct.fields().size()));
}

Impact

Cosmetic rather than a correctness bug — both paths reject the reference — but the exception a caller sees for an out-of-range struct field depends on how far out of range it is, and the common case yields the least informative of the two. ProtoExpressionConverter builds references straight from proto through ofExpression, so a malformed plan surfaces as IndexOutOfBoundsException: Index: 1, Size: 1 with nothing identifying the reference.

Suggested fix

Make the bound check index < 0 || index >= expr.fields().size(), and adopt constructOnRoot's message so both paths report the offset and the field count. Worth checking ListIndexFinder and MapKeyFinder in the same pass: ListIndexFinder deliberately ignores its index (the length of a list is not part of its type), which is correct but undocumented, and reads as an oversight next to this.

Changing the exception class is a small behavioural change for anyone catching IndexOutOfBoundsException from proto conversion, which is why it was left out of #1061 rather than fixed there.

Found while reviewing #1061.