`Iterable` and `Iterator` immutability iconsistency

[andreoss]

Many Iterable implementations are built on top of an Iterator with similar fuctionality.

While Iterator would be inherently mutable, it's unclear why Iterable object should be mutated by operations performed on it's underlying iterator.

For example, new IterableOf<>(() -> new IteratorOf<>(1,2,3)) - produces immutable Iterable new IterableOf<>(new IteratorOf<>(1,2,3)); - produces mutable Iterable, which is empty after values were traversed.

This causes behaviour such as below

    @Test
    public void rangeOfUnexpectedBehaviour() throws Exception {
        final Iterable<? extends Number> nums = new RangeOf<>(
            1,
            100,
            value -> ++value
        );
        new FirstOf<>(nums, 1).value();
        new FirstOf<>(nums, 1).value();
        MatcherAssert.assertThat(
            new FirstOf<>(nums, 1),
            new HasValue<>(3) // Should be 1
        );
    }

[victornoel]

@andreoss I'm not sure what it is that you are saying exactly? Are you saying the implementation of RangOf is broken?

[andreoss]

@victornoel new IterableOf<>(new IteratorOf<>(...)).iterator() should not return a copy of the original iterator, not the iterator itself.

[victornoel]

@andreoss I don't think that's possible to do :/ you can show if you know how but I don't see it.

Nevertheless, we can fix RangeOf without ressorting to this I guess.

[victornoel]

@andreoss ah, I see you already fixed RangeOf, so that's a good thing.

[andreoss]

@andreoss It's impossible to copy an Iterator without changing it. But IterableOf can exhaust Iterator once to create a copy, and than use this copy.

    public IterableOf(final Iterator<? extends X> list) {
        this(new Scalar<Iterator<? extends X>>() {
            /** Exhaust & copy iterator */
            private final Iterable<? extends X> copy = .... ;
            @Override
            public Iterator<? extends X> value() throws Exception {
                return this.copy.iterator();
            }
        });
    }

[victornoel]

@andreoss that is something we don't want to do. If you need an iterable with a copy of an iterator, you can use ListOf.

The contracts of iterable and iterator are clear and it is the responsibility of the user to compose them correctly (and the PR with the fix shows the composition don't in RangeOf was incorrect until you fixed it).

What we can do is two things:

• prevent this kind of composition error by having iterable of only take a scalar of iterator (it won't prevent all mistakes, because it's always possible to pass on the lambda an iterator that can be consumed only once, but it's a user mistake, not a cactoos bug)
• improve the documentation of IterableOf to explain this and that ListOf exists.

[victornoel]

@andreoss so, do you see more than what I expressed in the commit above? To be honest, all of this has nothing to do with immutability from my point of view, it's just that the implementation of the expected behaviour was broken.

[victornoel]

@andreoss closing this, don't hesitate to come back to me if we need to reopen this