Peek operation for sequences and iterables

[ilya-g]

Discussions about the onEach operation for sequences and iterables will be held here.

[ilya-g]

Do we need peek for Iterables at all?
Some counter-arguments:

• can be expressed with apply { forEach { ... } }
  • pro: semantics is clear
  • con: hides this
• intermediate operations on Iterable usually copy the source collection, it may be hard to tell what would be the behavior of peek
• can be confused with Queue.peek().

[JakeWharton]

I like the name onEach as a complement to forEach since it further implies that this is purely used as a side-effect operation in the stream/chain.

[ilya-g]

Should peek delegate to map for Sequences?

We plan to make some optimizations of map operation for sequences, it would be beneficial if peek could exploit those optimizations without having to reimplement them.

To implement peek with map we have to wrap the operation in the following lambda:
{ operation(it); it }

We could either inline the operation in that lambda or not. The performance consequences of that options should be studied for both cases: when the operation is specified as a lambda itself peek { println(it) }, or passed as a functional type peek(operation).

[cbruegg]

Peek for Iterables

can be expressed with apply { forEach { ... } }

Another con: It creates another nested layer of curly braces and thus more indentation in many cases.

intermediate operations on Iterable usually copy the source collection, it may be hard to tell what would be the behavior of peek

I have to agree here - the behavior definitely differs from Sequence.peek and could cause misunderstanding.

sequenceOf(1, 2).peek { println(it * 2) }.forEach { println(it) } would yield

2
1
4
2

while

listOf(1, 2).peek { println(it * 2) }.forEach { println(it) } would yield

2
4
1
2

can be confused with Queue.peek()

That would be sorted out by renaming peek to onEach - perhaps only Iterable.peek -> Iterable.onEach, but not Sequence.peek -> Sequence.onEach, to reflect the differences in behavior. A downside would be decreased discoverability.

Performance

The performance consequences of that options should be studied for both cases

Unfortunately I don't have much experience with this, especially since there may be differences between various VM implementations like ART, Dalvik and the JVM due to the JITs.

[ilya-g]

That would be sorted out by renaming peek to onEach - perhaps only Iterable.peek -> Iterable.onEach, but not Sequence.peek -> Sequence.onEach, to reflect the differences in behavior. A downside would be decreased discoverability.

The difference in evaluation order is not an issue, the same difference is observed if you change peek to map for example.
After some contemplation I think onEach is a good name both for Iterable and Sequence.

[ilya-g]

I believe it's the time to implement a prototype. You can either craft it as a pull request to the kotlin standard library if you're familiar with stdlib code generator or write manually in any other repository.

[ilya-g]

Some implementation notes:

• Iterable<T>.onEach returns the receiver itself, so it would be good to have the return type preserving the type of the receiver.
• onEach could be provided for other types of receivers that already have forEach.

[cbruegg]

If I understood the DSL correctly, something like this should work. Ant kept crashing for me and I'm getting Could not find artifact org.jetbrains.kotlin:kotlin-maven-plugin:pom:1.1-SNAPSHOT when executing the maven goal compile exec:java on the generator, so unfortunately I wasn't able to test it.

[matklad]

peek is a really confusing name imo, because peek as "look at the next element, but don't call next" also makes sense for iterators. Rust uses the name inspect for this function, I think it is a good choice.

[ilya-g]

@cbruegg not quite right, but ok for starting point. I've elaborated those templates a bit, it required to improve the DSL to support such non-trivial constraints on generic receiver.

The result is available in rr/stdlib/oneach branch. Could you check it out and try it with some tests?

[cbruegg]

@ilya-g Sorry for the late response and thanks for fixing the template. I've had a look at the generated code and tried it out with some tests and everything seems to work as expected. I really like how we were able to implement this using map and apply, which makes the generated code easily verifiable for correctness.

Personally, I think this looks pretty much complete.

[ilya-g]

Since we'd like to avoid inflating method count in stdlib, we're going not to provide onEach for arrays. Otherwise, all looks good and we'll merge rr/stdlib/oneach branch soon.

[cbruegg]

Great, thank you for having worked on this! It's always great to see that JetBrains is really listening to feedback.

[elect86]

What about onEachIndexed?

[haug-den-lucas]

Wanted to reask, what is about onEachIndexed?

[ilya-g]

I think that onEachIndexed can be useful.
Could you provide some motivating examples for this function?