Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/library/scala/collection/Iterable.scala
Original file line number Diff line number Diff line change
Expand Up @@ -819,6 +819,8 @@ trait IterableOps[+A, +CC[_], +C] extends Any with IterableOnce[A] with Iterable
*/
def inits: Iterator[C] = iterateUntilEmpty(_.init)

override def tapEach[U](f: A => U): C = fromSpecific(new View.Map(this, { a: A => f(a); a }))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I get others' opinions on whether or not this should be implemented as a call to map instead of fromSpecific(new View.Map)? I think that the former simplifies/reduces the work for those implementing collections which need to override map, but I am curious to hear others' thoughts.

I don't think the decision should hold up the PR - it's an easy change to make in its own PR if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using map is better, indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, calling map { a => f(a); a } will only return a CC[A], rather than the more specific C.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is interesting and unfortunate. I'm torn on whether or not it would still be better to use map and cast the result. I'm leaning towards not.

Copy link
Member

@SethTisue SethTisue Nov 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked @szeiger to look at this and explain it to me.

Josh's point is that we can't typecast our way out of this. CC and C[A] can be different collection types entirely:

scala 2.13.0-M5> (Map(1 -> "foo"): Iterable[(Int, String)]).map(x => x)
res5: Iterable[(Int, String)] = List((1,foo))

scala 2.13.0-M5> (collection.immutable.BitSet(0, 5, 9): Iterable[Int]).map(_ + 1)
res0: Iterable[Int] = Set(1, 6, 10)

(in an earlier edit, I'd included a String example where I "upcast" it to Iterable[Char], but that's not really the same, because it's an implicit conversion, not an upcast)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SethTisue Wow! Very sneaky! Some downsides (which may have workarounds):

  • filter can (as far as the compiler is concerned) change the length of the Iterable. So the consequences of this are that, perhaps non-strict Seq's like SeqView and LazyList will have to then go through each element in order to fulfill a call to apply(100), whereas before the filter call, they may have been able to independently evaluate/access element 100 immediately.
  • The above point means that IndexedSeqView#filter cannot return an IndexedSeqView

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add on one more point, filter also destroys independent laziness of elements (currently only a thing for LazyList, but could potentially be a thing for some lazy IndexedSeq in the future).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshlemer okay, sounds like it’s better as-is then

@NthPortal I don’t follow?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SethTisue

scala> def list = LazyList.tabulate(10)(i => i)
list: scala.collection.immutable.LazyList[Int]

scala> def f(i: Int) = println(i)
f: (i: Int)Unit

scala> list.map(i => {f(i); i})(5)
5
res0: Int = 5

scala> list.filter(i => {f(i); true})(5)
0
1
2
3
4
5
res1: Int = 5

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I retract all complaints, and I'm happy with fromSpecific(new View.Map(...))


// A helper for tails and inits.
private[this] def iterateUntilEmpty(f: Iterable[A] => Iterable[A]): Iterator[C] = {
val it = Iterator.iterate(toIterable)(f).takeWhile(x => !x.isEmpty)
Expand Down
11 changes: 11 additions & 0 deletions src/library/scala/collection/IterableOnce.scala
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,17 @@ trait IterableOnceOps[+A, +CC[_], +C] extends Any { this: IterableOnce[A] =>
*/
def span(p: A => Boolean): (C, C)

/** Applies a side-effecting function to each element in this collection.
* Strict collections will apply `f` to their elements immediately, while lazy collections
* like Views and LazyLists will only apply `f` on each element if and when that element
* is evaluated, and each time that element is evaluated.
*
* @param f a function to apply to each element in this $coll
* @tparam U the return type of f
* @return The same logical collection as this
*/
def tapEach[U](f: A => U): C

/////////////////////////////////////////////////////////////// Concrete methods based on iterator

def knownSize: Int = -1
Expand Down
10 changes: 10 additions & 0 deletions src/library/scala/collection/Iterator.scala
Original file line number Diff line number Diff line change
Expand Up @@ -863,6 +863,16 @@ trait Iterator[+A] extends IterableOnce[A] with IterableOnceOps[A, Iterator, Ite
}
}

override def tapEach[U](f: A => U): Iterator[A] = new AbstractIterator[A] {
override def knownSize = self.knownSize
override def hasNext = self.hasNext
override def next() = {
val _next = self.next()
f(_next)
_next
}
}

/** Converts this iterator to a string.
*
* @return `"<iterator>"`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,10 @@ trait StrictOptimizedIterableOps[+A, +CC[_], +C]
(l.result(), r.result())
}

// Optimization avoids creation of second collection
override def tapEach[U](f: A => U): C = {
foreach(f)
coll
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't return this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, StrictOptimizedSeqOps[..., C] does not conform to C. I could play around with introducing a self-type to it, self: C => and see if there are any issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, It seems that this solution does solve it, as long as you also enforce the self: C => type on all extending StrictOptimized{Seq,Set,Map,SortedMap, SortedSet}Ops. I'd like to get @julienrf 's opinion though if he forsees any issues with that constraint, maybe it's intended to be extendable by non-C things too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exceptions used to be StringOps and ArrayOps. All regular collection types extend their C parameter. Can we make any further simplifications if we always enforce this? I don't think it's worth making this change specifically for tapEach. There are plenty of other methods that return C. In most cases you abstract over consuming a collection where this is not an issue. If you also need to produce a more specific collection type you need to use the proper abstraction anyway (unless we can change it in a more generic way).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szeiger I can't think of any other improvements or simplifications that could be made if it had { self: C => unfortunately.

}

}
2 changes: 2 additions & 0 deletions src/library/scala/collection/immutable/LazyList.scala
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,8 @@ final class LazyList[+A] private(private[this] var lazyState: () => LazyList.Sta
if (knownIsEmpty) LazyList.empty
else (mapImpl(f): @inline)

override def tapEach[U](f: A => U): LazyList[A] = map { a => f(a); a}

private def mapImpl[B](f: A => B): LazyList[B] =
newLL {
if (isEmpty) State.Empty
Expand Down
36 changes: 34 additions & 2 deletions test/junit/scala/collection/IteratorTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -535,24 +535,33 @@ class IteratorTest {
// Avoid reaching seq1 through test class. Avoid testing Array.iterator.
class C extends Iterable[String] {
val ss = Array("first", "second")

def iterator = new Iterator[String] {
var i = 0

def hasNext = i < ss.length

def next() =
if (hasNext) { val res = ss(i) ; i += 1 ; res }
if (hasNext) {
val res = ss(i); i += 1; res
}
else Iterator.empty.next()
}

def apply(i: Int) = ss(i)
}
val seq1 = new WeakReference(new C)
val seq2 = List("third")
val it0: Iterator[Int] = Iterator(1, 2)
lazy val it: Iterator[String] = it0.flatMap {
case 1 => seq1.get
case _ => check() ; seq2
case _ => check(); seq2
}

def check() = assertNotReachable(seq1.get, it)(())

def checkHasElement() = assertNotReachable(seq1.get.apply(1), it)(())

assert(it.hasNext)
assertEquals("first", it.next())

Expand All @@ -568,4 +577,27 @@ class IteratorTest {
}
assert(!it.hasNext)
}

@Test def tapEach(): Unit = {
locally {
var i = 0
val tapped = Iterator(-1, -1, -1).tapEach(_ => i += 1)
assertEquals(true, tapped.hasNext)
assertEquals(0, i)
}

locally {
var i = 0
val tapped = Iterator(-1, -1, -1).tapEach(_ => i += 1)
assertEquals(-3, tapped.sum)
assertEquals(3, i)
}

locally {
var i = 0
val tapped = Iterator(-1, -1, -1).tapEach(_ => i += 1)
assertEquals(-1, tapped.next())
assertEquals(1, i)
}
}
}
24 changes: 22 additions & 2 deletions test/junit/scala/collection/ViewTest.scala
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package scala.collection

import scala.collection.immutable.List

import org.junit.Assert._
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.JUnit4

import language.postfixOps
import scala.collection.mutable.ListBuffer

@RunWith(classOf[JUnit4])
class ViewTest {
Expand All @@ -17,7 +18,8 @@ class ViewTest {

import scala.language.postfixOps
assertEquals(Iterable.empty[Int], iter.view take Int.MinValue to Iterable)
assertEquals(Iterable.empty[Int], iter.view takeRight Int.MinValue to Iterable)
assertEquals(Iterable.empty[Int],
iter.view takeRight Int.MinValue to Iterable)
assertEquals(iter, iter.view drop Int.MinValue to Iterable)
assertEquals(iter, iter.view dropRight Int.MinValue to Iterable)
}
Expand Down Expand Up @@ -69,4 +71,22 @@ class ViewTest {
check(immutable.Map(1 -> "a", 2 -> "b")) // MapView
}

@Test
def tapEach: Unit = {
val lb = ListBuffer[Int]()

val v =
View(1, 2, 3)
.tapEach(lb += _)
.map(_ => 10)
.tapEach(lb += _)
.tapEach(_ => lb += -1)

assertEquals(ListBuffer[Int](), lb)

val strict = v.to(Seq)
assertEquals(strict, Seq(10, 10, 10))
assertEquals(lb, Seq(1, 10, -1, 2, 10, -1, 3, 10, -1))
}

}
30 changes: 30 additions & 0 deletions test/junit/scala/collection/immutable/LazyListTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import org.junit.Test
import org.junit.Assert._

import scala.collection.Iterator
import scala.collection.mutable.ListBuffer
import scala.ref.WeakReference
import scala.util.Try

Expand Down Expand Up @@ -358,6 +359,15 @@ class LazyListTest {
assertLazyHeadWhenNextHeadEvaluated(op)
}

@Test
def tapEach_properlyLazy(): Unit = {
val op = lazyListOp(_.tapEach(_ + 1))
assertRepeatedlyFullyLazy(op)
assertLazyTailWhenHeadEvaluated(op)
assertLazyHeadWhenTailEvaluated(op)
assertLazyHeadWhenNextHeadEvaluated(op)
}

@Test
def collect_properlyLazy(): Unit = {
val op = lazyListOp(_ collect { case i if i % 2 != 0 => i})
Expand Down Expand Up @@ -1000,4 +1010,24 @@ class LazyListTest {
assertEquals(s"mkString 3 $i", goal(3,i), precyc(3,i).mkString)
}
}
@Test
def tapEach: Unit = {

/** @param makeLL must make a lazylist that evaluates to Seq(1,2,3,4,5) */
def check(makeLL: => LazyList[Int]): Unit = {
val lb = ListBuffer[Int]()
val ll = makeLL.tapEach(lb += _)
assertEquals(ListBuffer[Int](), lb)
assertEquals(Vector(1, 2), ll.take(2).to(Vector))
assertEquals(ListBuffer(1, 2), lb)
assertEquals(4, ll(3))
assertEquals(ListBuffer(1, 2, 4), lb)
assertEquals(Vector(1,2,3,4,5), ll.to(Vector))
assertEquals(ListBuffer(1, 2, 4, 3, 5), lb)
}

check(LazyList.from(Iterator(1, 2, 3, 4, 5)))
check(LazyList.from(Vector(1, 2, 3, 4, 5)))
check(LazyList.tabulate(5)(_ + 1))
}
}
20 changes: 20 additions & 0 deletions test/junit/scala/collection/immutable/VectorTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import org.junit.runner.RunWith
import org.junit.runners.JUnit4
import org.junit.Test

import scala.collection.mutable.ListBuffer

@RunWith(classOf[JUnit4])
class VectorTest {

Expand Down Expand Up @@ -161,4 +163,22 @@ class VectorTest {
assertEquals(-1, test.indexOf(1000))
}

@Test
def tapEach(): Unit = {
val lb = ListBuffer[Int]()

val v =
Vector(1,2,3)
.tapEach(lb += _)
.tapEach(lb += _)

assertEquals(ListBuffer(1,2,3,1,2,3), lb)
assertEquals(Vector(1,2,3), v)


val f: Any => Unit = println

// test that type info is not lost
val x: Vector[Char] = Vector[Char]().tapEach(f)
}
}