Collection creation behavior changed from 1.2.2 to 2.0.0

[danieldietrich]

@ggalmazor wrote:

Currently, IntelliJ can't compile this: public List<String> toList(String... someStrings) {
List<String> theList = List.of(someStrings);
return theList;
}
But it works if I change the first line on the method to: List<String> theList = List.<String>of(someStrings); 

My intention is to handle the vararg someStrings as a list inside the method, but expose a nice API for consumers of this methods in which they can invoke it like: toList("a", "b", "c", ...)

This is curious because in 1.2.2 it worked flawlessly
Maybe we could have a List.ofAll(T[] tArray)
I mean, List.ofAll(T[] tArray) => List<T>

[danieldietrich]

Investigating it showed, that I first also got the IntelliJ compile error, but then (after issuing a mvn compile) on the command line, the IDE errors magically disappeared. Then I pressed the 'make project' button - and the errors still did not appear again.

Here is the test code:

public class Test {

    public void toList(String... someStrings) {
        List<String> theList1 = List.<String>of(someStrings);
        List<String> theList2 = List.of(someStrings);
    }
}

Also the extra <String> arg is recognized by IntelliJ as unnecessary. I think there is nothing to do. If IntelliJ says, there is an error while javac compiles fine, it seems to be an IntelliJ bug.

[ggalmazor]

Anyway, I think that the name of the factory method is misleading. IMHO factory methods should be:

• When providing an individual object: List.of(T element)
• When providing a collection or array of objects: List.ofAll(T[] elements) and List.ofAll(Iterable<T> elements)

This way, there would be no possible confussion:

• List.of(T) gives List<T>
• List.of(T, T, T,...) is not supported
• List.of(T[]) gives List<T[]>
• List.of(Iterable<T>) gives List<Iterable<T>>
• List.ofAll(T) gives List<T> supported by next varargs signature
• List.ofAll(T, T, T, ...) gives List<T>
• List.ofAll(T[]) gives List<T> supported by varargs signature
• List.ofAll(Iterable<T>) gives List<T>

I'd indulge the redundancy of List.of(T) == List.ofAll(T) in favor or a more coherent API.

[danieldietrich]

I like the idea to use ofAll(T...) instead of of(T...) because it looks more consistent.

History

The idea behind of(T) and of(T...) was to do it 'the Java way'. I did take a look at the java.util.stream.Stream API. There we have these methods:

• Stream.of(T)
• Stream.of(T...)

I didn't understood, why there is the of(T) method, because it is a special case of of(T...), using just one argument. Here is a diskussion with Lukas Eder: https://github.com/jOOQ/jOOL/issues/89

It seems, that we have Stream.of(T) for the null case, because the null array is/should be not a valid varargs argument.

Next steps

If no one is shouting out loud, I will implement your suggestion and change the current API.

We have also the primitive arrays, e.g. List.ofAll(int[]) gives List<Integer>, but I think it will not collide with the signatures you sketched above.

[ggalmazor]

Alright! :)

Yeah, I didn't specify the cases for primitive arrays as they should behave (imo) just like in List.ofAll(T[])

[danieldietrich]

Lukily we are still in beta. There are quite some (little) changes in the pipeline :-)

[danieldietrich]

Before performing this change we will boost up the test-coverage to ~100% in order to get sure all is still working as expected.

Example:

T[] array = ...;

// = List<T> before the change
List.of(array);

// = List<T[]> after the change
List.of(array);

Right?

It might break existing programs. This is the reason why we have a major release, jumping from 1.x to 2.0.0. The API changes in order to improve.

[danieldietrich]

It would be nice to have a syntax that aligns with JEP 269: Convenience Factory Methods for Collections. Java will most probably get the syntax List.of(T...).

What comes in the way is List.of(T). Maybe it can be renamed to List.single(T) or List.singleton(T).

Then all List.ofAll(...) methods can be renamed to List.of(...). To be checked...

[danieldietrich]

This gist describes the problems of of(T) and of(T...): https://gist.github.com/danieldietrich/45bd2678001444d09a5b

[danieldietrich]

It will look like this

public class Test {{

    // List.single(T)
    List<List<String>> l1 = List.single(List.of("1"));

    // List.of(Iterable)
    List<String> l2 = List.of(List.of("1"));

    // List.of(T...)
    List<List<String>> l3 = List.of(List.of("1"), List.of("2"));

}}

interface List<T> extends Iterable<T> {

    static <T> List<T> single(T element) { ... }

    @SafeVarargs
    static <T> List<T> of(T... elements) { ... }

    static <T> List<T> of(Iterable<? extends T> elements) { ... }

}

[danieldietrich]

After starting to refactor of(T) -> single(T) and ofAll(*) -> of(*) I came to the conclusion, that it is better to stay with of and ofAll and just rename of(T...) to ofAll(T...) for the following reasons:

• there would otherwise raise inconsistencies in the naming scheme, e.g. Stack.pushAll(Iterable) vs Stack.push(T) - naming the latter pushSingle is not what we want
• renaming just of(T...) is minimal intrusive regarding changes of the API. this is important for users which already build upon Javaslang
• factory methods easy to remember - all start with of...

Therefore I will rename of(T...) to ofAll(T...).

[ruslansennov]

:+1:

[ggalmazor]

:+1:

This looks nice!

[danieldietrich]

Great, thx :-)

[danieldietrich]

Here is an interesting case:

Tuple1 has a toSeq method, which creates a List containing the tuple element:

class Tuple1<T1> {

    T1 _1;

    // ...

    public Seq<?> toSeq() {
        return List.ofAll(_1);
    }
}

One could think that at runtime List.ofAll(Iterable) is called if _1 is of type Iterable. But because of type erasure, the generic T1 is erased to Object at compile time. Therefore the method toSeq calls List.ofAll(T...).

The following code works as expected:

Tuple1<List<Integer>> t = Tuple.of(List.of(1));

// = List(List(1))
t.toSeq();

I think it is a little bit less confusing if the Tuple1.toSeq implementation calls List.of(_1) instead of List.ofAll(_1). I will change that.

[danieldietrich]

Just saying: This does not compile:

SortedSet<Integer> set = TreeSet.ofAll((a, b) -> a - b, 1, 2, 3);

It needs to be

// like this
Comparator<Integer> c = (a, b) -> a - b;
SortedSet<Integer> set = TreeSet.ofAll(c, 1, 2, 3);

// or like that
SortedSet<Integer> set = TreeSet.<Integer> ofAll((a, b) -> a - b, 1, 2, 3);

If we rename ofAll to of it works. However, we will stay with the current API. There will always be ambiguities and we know how to solve them.

[danieldietrich]

@ggalmazor Oh my gosh - I think we have to do it like you said in your first comment and that will solve all problems :-)

Maybe we could have a List.ofAll(T[] tArray) I mean, List.ofAll(T[] tArray) => List

That is having these:

of(T)
of(T...)

For SortedSet/SortedMap also versions, which take a Comparator:

of(Comparator<? super T>, T)
of(Comparator<? super T>, T...)

The solves the problem in the previous comment.

To solve your original problem, we will add:

ofAll(T[])

Btw, this is consistent in the way, that all ofAll methods take a single argument that is inherently multi-valued and will be unboxed.

EXTRA POINTS: The of(T) and of(T...) methods will be consistent with the Java standard API!

Have to check, if the above really holds in all cases...

Update: I think I have overseen the one case where we want to create an Object of <T[]>. It is not possible to do it with of. Either we add an additional single(T) for this case - or we live with the fact that we need to provide extra generics for that rare case xxx.<T[]> of(...). Then ofAll(T[]) is unnecessary, right?

[danieldietrich]

What I do not like about it:

• we found just another ambiguity
• again changing the API that was supposed to be final

What is good about it:

• aligning with standard Java is not that bad
• we do not break Javaslang 1.2.2 API here

Note: It is more important that we are able to create a TreeSet with an inline comparator and varargs TreeSet((a, b) -> ..., v1, v2, ...) than wrapping an Array. The latter is considered as rare case:

[danieldietrich]

Before closing this check CharSeq:

public static CharSeq of(CharSequence sequence);

[ggalmazor]

Do we have a test class with all the cases? I have to confess that I'm a little confused right now...

I can work on a test this weekend...

[danieldietrich]

:+1:

[danieldietrich]

We have some tests - but refactoring is hard. IntelliJ changes too much - all comments, the Generator.scala String tempates... Sometime right, somethimes not o_O

I will fix that...

[danieldietrich]

On the other hand some javadoc bugs introduced by our previous refactoring (of -> ofAll) are not fixed again. All will be good :-)

[danieldietrich]

@ggalmazor I'm able to finish the refactoring tomorrow. Until then I will not pull any PR's so that we are able to go back easily if we find any blockers in our tests. With blockers I mean API that does not seem to be right or feels not good.

[danieldietrich]

@ggalmazor finished. I'm in a hurry - continuous delivery ;) I will create a 2.0.0-RC2 now. Changes, if any, will go to 2.0.0-RC3-SNAPSHOT.

[danieldietrich]

Works flawless so far. I will close this ticket. Any experience / additional tests or notes are welcome!

[ggalmazor]

So, I've updated my fork and I've taken a look to the tests... some thoughts:

Currently, it's really difficult to get a grasp about what we can do with Javaslang... There is too much setup code. Take, for example ListTest: there are 160 lines for test preparation for 50 lines of (hard to read) test code...

Also, extending test cases doesn't help to make things easier (otherwise, we wouldn't have those 160 lines, would we?). We should use @RunWith(Parameterized.class) to run same tests on different objects.

We could write test classes centered on specific scenarios: empty lists, exception handling, etc.

Test could be the number 1 go-to reference when trying to understand any of Javaslang's features if we wrote them slightly different.

I've started working on this concept and I've commited into my fork a pair of test reflecting this and I'd like to discuss this before requesting a pull request.

The goal would be to adapt AbstractSeqTest and all its implementations to these concepts, using @RunWith(Parameterized.class) which would produce more readable and concise tests.

I can start working on that if I've convinced you :)

[danieldietrich]

Hi!

I'm completely in it, you already bought me to make the tests more readable! :)

I looked at the fork. I like the idea to slice the tests by business logic (in a technical domain), e.g. having a test for factory methods of List.

Our requirements are:

• reuse as many tests as possible within the class hierarchy (e.g. Value tests, Traversable tests, Seq tests etc.)
• don't loose the ability to overwrite special assertion behavior, currently needed for Maps and CharSeq I think.
• also we distinguish between collections that check equality to be same object vs. same content.

I prefer high cohesion of functionality. E.g. instead of spreading the tests of one method (e.g. head()) over many classes (e.g. EmptyTraversableTest, SomeOther1Test, SomeOther2Test, ...) a developer needs to have it in one place in order to see if something is missing.

Currently we use abstract methods and assertion override to define the behavior of a specific test impl. How would this look like with a @RunWith(Parameterized.class). Could you sketch this, please?

Thx,

Daniel

[ggalmazor]

Ok! Agree on everything you said :)

Currently we use abstract methods and assertion override to define the behavior of a specific test impl. How would this look like with a @RunWith(Parameterized.class). Could you sketch this, please?

That was step 2 of my plan. I'll work on a sketch and get back to you before start working on everything...

[danieldietrich]

Nice!

[ggalmazor]

While working on the tests I've noticed that we can Tree.ofAll(List.of(1, 2, 3)) but there is no Tree.of(1, 2, 3)... is this right?

[danieldietrich]

Yes - but it might be useful. I created an issue: #935