(#1575) Generify org.cactoos.collection package

[rocket-3]

For #1575.

Generify parameter types in constructors of org.cactoos.collection package classses:

• Immutable
• NoNulls

It's done by using unchecked casts, because there are absolutely no other ways to achive this, due to JDK interfaces limitations.

[victornoel]

@rocket-3 instead of this, you should change the type of the col field. You won't need any cast in that case.

[rocket-3]

Then, I'll need to do unchecked cast in Collection::iterator method.
Collection<? extends X>::iterator return type is Iterator<? extends X>.
Which breaks the Iterable<X>::iterator return type Iterator<X> contract.

I also can do this without cast, but there's the unchecked cast inside IterableOf(Scalar<Iterator<? extends X>>)

public Iterator<X> iterator() {
    return new IterableOf<X>(this.col::iterator).iterator();
}

It seems messy for me because:

1. Unnecessary constructioning inside method
2. Unnecessary coupling on org.cactoos.iterable.IterableOf

@victornoel, WDYT?

[victornoel]

@rocket-3 then you need to propagate the same kind of change everywhere needed until everything types correctly :) that's the purpose of this series of tickets: that the whole codebase uses generic with variance like this.

[rocket-3]

@victornoel, sorry, I didn't get it

We can't propagate changes to JDK

As I see, we have two ways:

• to do unchecked cast
• to rely on class which does uchecked cast

Which one you would prefer? Or you see, there's more ways?

[victornoel]

@rocket-3 We should be able to propagate as much as possible in our classes, and then normally, no cast is needed. If some casts are needed, let's put them at the last moment possible (usually inside a method implementation).

So let's go with this and see what happens with the two classes you started modifying!

[rocket-3]

@victornoel, no other way for NoNulls, cause we have an add and addAll impls.
We can't add items to a Collection<? extends X> instance by JDK design, only extract, see PECS trilemma.

By the way, changed Immutable, see in the recent commit.

[victornoel]

@rocket-3 ok, in that case, for NoNulls, because of its nature (being mutable), it's not possible to apply variance like this and thus the constructor should only take a X. If we cast, this will create errors at runtime if incorrect elements are added to such a collection. Let's also add a comment in the javadoc of the constructor so that we remember why it's like this.

[rocket-3]

@victornoel, wrote a test for this behaviour. Seems like no runtime exceptions, WDYT?

[victornoel]

@rocket-3 here is a failing test:

    @Test
    void fails() {
        Collection<Double> a = new ArrayList<Double>();
        Collection<Number> b = new NoNulls<>(a);
        b.add(Long.valueOf(4));
        Double oupsNotADouble = a.iterator().next();
    }

This will throw java.lang.ClassCastException: java.lang.Long cannot be cast to java.lang.Double even though it compiles correctly.

[rocket-3]

Okay, I've removed uchecked cast, added impl. note and removed test created recently.

[victornoel]

@rocket-3 nice use of @implNote, I didn't know about this!

[codecov-commenter]

Codecov Report

Merging #1624 (31eb51a) into master (445cf16) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1624      +/-   ##
============================================
- Coverage     90.12%   90.10%   -0.02%     
- Complexity     1602     1603       +1     
============================================
  Files           298      298              
  Lines          3745     3749       +4     
  Branches        122      122              
============================================
+ Hits           3375     3378       +3     
  Misses          337      337              
- Partials         33       34       +1     

Impacted Files	Coverage Δ
src/main/java/org/cactoos/collection/NoNulls.java	100.00% <ø> (ø)
...rc/main/java/org/cactoos/collection/Immutable.java	100.00% <100.00%> (ø)
src/main/java/org/cactoos/scalar/Solid.java	90.00% <0.00%> (-10.00%)	⬇️
src/main/java/org/cactoos/map/MapOf.java	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 445cf16...31eb51a. Read the comment docs.

[victornoel]

@rocket-3 the linters CI job failed, it must be fixed before we can merge this PR :)

[rocket-3]

@rocket-3 nice use of @implNote, I didn't know about this!

Also because of that linters CI job failed...

[victornoel]

@rultor merge

[victornoel]

@rocket-3 thx

[rultor]

@rultor merge

@victornoel OK, I'll try to merge now. You can check the progress of the merge here

[rultor]

@rultor merge

@victornoel Done! FYI, the full log is here (took me 13min)