Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MapView.values is not lazy and forcing evaluation #12059

Closed
neontorrent opened this issue Jun 26, 2020 · 10 comments · Fixed by scala/scala#9090
Closed

MapView.values is not lazy and forcing evaluation #12059

neontorrent opened this issue Jun 26, 2020 · 10 comments · Fixed by scala/scala#9090

Comments

@neontorrent
Copy link

reproduction steps

using Scala 2.13.2,

scala> Map(1->1).view.values
val res0: Iterable[Int] = Iterable(1)

problem

Expected behavior:
View operations should be more lazy. values method should return a View (possibly a new type MapView.Values) without forcing the elements. E.g.

val res0: Iterable[Int] = View(<not computed>)

Currently I use the workaround as:

m.view.map(_._2)
@julienrf
Copy link

We could argue that values is an operation that forces the view. At least, the return type Iterable, doesn’t promise that the resulting collection will be lazy. However, your suggestion could also work. We need to specify the behavior that we want.
That being said, the fact that it is currently forcing the view, and that there is a simple workaround to achieve what you want makes me lean towards not changing the existing behavior.

@neontorrent
Copy link
Author

neontorrent commented Jun 26, 2020

It is currently returning an Iterable. Changing it to View which is still an Iterable, so I don't think changing the behavior will impact any existing code (maybe not the code which assume the returned Iterable is a List which to me is unreasonable)

It is confusing that when you expect operations under View to be lazy, but it is not. I don't know officially which methods should force, and in my mind only the to(Factory) (and the deprecated force) should realize the collection. I would like to even push it even more, to make the signature more restricted, to just return a View instead of a plain Iterable.

@NthPortal
Copy link

NthPortal commented Jun 27, 2020

the fact that Map#view.map(_._2) is lazy, but Map#view.values is strict, is highly unintuitive IMO disregard, this is incorrect

@NthPortal
Copy link

it actually is lazy (the implementation just returns an AbstractIterable), it's just that the REPL forces Iterables when rendering them. oops.

scala> Map(1 -> 1).view.mapValues(i => { println(i); i }).values eq ""
                                                                 ^
       warning: Iterable[Int] and String are unrelated: they will most likely never compare equal
val res0: Boolean = false

scala> Map(1 -> 1).view.mapValues(i => { println(i); i }).values
1
val res1: Iterable[Int] = Iterable(1)

@NthPortal
Copy link

technically this is the result of scala.runtime.ScalaRunTime.stringOf, not the REPL, but there's no "runtime" label

@NthPortal
Copy link

this problem is actually slightly more complicated than just blaming it on ScalaRunTime.stringOf. even if we call toString, it still evaluates. thus, there isn't really any way in principle to fix stringOf without also changing how those Iterables are created.

@neontorrent
Copy link
Author

neontorrent commented Jun 27, 2020

@NthPortal You are right in the fact that values is lazy and return just an iterator. My original example is invalid.

But the problem arises after you call the values. Any operation after values will be forced. E.g.

mm.view.values.map(_+1)

This will force and return a List. On the contrary,

mm.view.map(_._2).map(_+1)

returns a lazy View.

Here is a full test (put in a scala file and run to avoid REPL evaluation):

val lazyMapViewWithEffect = Map(0 -> {() => println("Forced")}).view.mapValues(_())

println("Test 1")
lazyMapViewWithEffect.values

println("Test 2")
lazyMapViewWithEffect.values.map(_ => "dummy")

println("Test 3")
lazyMapViewWithEffect.map(_._2).map(_ => "dummy")

This will return:

Test 1
Test 2
Forced
Test 3

As you can see values does not force, but any subsequent operation will do

@julienrf
Copy link

julienrf commented Jun 27, 2020

But the problem arises after you call the values. Any operation after values will be forced. E.g.

mm.view.values.map(_+1)

This will force and return a List. On the contrary,

mm.view.map(_._2).map(_+1)

I definitely agree that this behavior makes it hard to reason about when things get evaluated. I hope not much code relies on the existing behavior and that we can change it to return a View instead of this weird collection which is like a view at first and then not like a view. Unfortunately, we can’t change the static return type to be View because that would break the forward compatibility.

Would someone be interested in submitting a PR with the change (returning a proper view without changing the type signature of the operation)? Then we can see if something breaks in the community build or not.

@NthPortal
Copy link

yeah, I can do that

@neontorrent
Copy link
Author

@NthPortal Thanks for the PR!

@julienrf It is pretty easy to migrate if there is existing code relying on the current behavior. Just replace mm.view.values with mm.values and it returns a realized List. Worst case just add a toList

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants