Iterable wrapping an iterator¶
ID: java/iterable-wraps-iteratorKind: problemSecurity severity: Severity: warningPrecision: very-highTags: - quality - reliability - correctnessQuery suites: - java-security-and-quality.qls
Click to see the query in the CodeQL repository
Java has two interfaces for dealing with iteration,Iterable<T> andIterator<T>. AnIterable<T> represents a sequence of elements that can be traversed, and anIterator<T> represents thestate of an ongoing traversal. As an example, all theCollection<T> classes in the Java standard library implementIterable<T>. Comparing this to a traditionalfor loop that increments an integer index and iterates over the elements of an array, then theIterable<T> object corresponds to the array, whereas theIterator<T> object corresponds to the index variable.
Implementations ofIterable<T> are generally expected to support multiple traversals of the element sequence they represent, although there can be exceptions if the underlying data somehow makes this undesirable, see for exampleDirectoryStream<T>. If an implementation ofIterable<T> does not support multiple iterations, then itsiterator() method should throw an exception on its second and subsequent calls. This makes bugs easier to find if such anIterable<T> is used more than once, for example in two different for-each loops.
Recommendation¶
When writing theiterator() method in anIterable<T> then it is important to make sure that each call will result in a freshIterator<T> instance containing all the necessary state for keeping track of the iteration. If the iterator is stored in theIterable<T>, or somehow refers to iteration state stored in theIterable<T>, then subsequent calls toiterator() can result in loops that only traverse a subset of the elements or have no effect at all.
Example¶
The following example returns the same iterator on every call, and therefore causes the second loop to terminate immediately without any effect.
classMySequenceimplementsIterable<MyElem>{// ... some reference to datafinalIterator<MyElem>it=data.iterator();// Wrong: reused iteratorpublicIterator<MyElem>iterator(){returnit;}}voiduseMySequence(MySequences){// do some work by traversing the sequencefor(MyEleme:s){// ...}// do some more work by traversing it againfor(MyEleme:s){// ...}}
This second example returns a newly created iterator each time, but still relies on iteration state stored in the surrounding class, and therefore also causes the second loop to terminate immediately.
classMySequenceimplementsIterable<MyElem>{// ... some reference to datafinalIterator<MyElem>it=data.iterator();// Wrong: iteration state outside returned iteratorpublicIterator<MyElem>iterator(){returnnewIterator<MyElem>(){publicbooleanhasNext(){returnit.hasNext();}publicMyElemnext(){returntransformElem(it.next());}publicvoidremove(){// ...}};}}
The code should instead be written like this, such that each call toiterator() correctly gives a fresh iterator that starts at the beginning.
classMySequenceimplementsIterable<MyElem>{// ... some reference to datapublicIterator<MyElem>iterator(){returnnewIterator<MyElem>(){// Correct: iteration state inside returned iteratorfinalIterator<MyElem>it=data.iterator();publicbooleanhasNext(){returnit.hasNext();}publicMyElemnext(){returntransformElem(it.next());}publicvoidremove(){// ...}};}}
References¶
Java Language Specification:The enhanced for statement.
Java API Specification:Interface Iterable<T>,Interface Iterator<T>,Interface DirectoryStream<T>.