Skip to content

Conversation

@odersky
Copy link
Contributor

@odersky odersky commented Jan 15, 2023

Avoid redundant map call if the yielded value is the same as the last result. This makes for expressions more efficient and provides more opportunities for tail recursion.

 - [] Avoid redundant map call if the yielded value is the same as the last result. This makes for expressions more efficient and provides more opportunities for tail recursion.
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Isn't this a spec change?

@tailrec
def pair[B](xs: List[Int], ys: List[B], n: Int): List[(Int, B)] =
if n == 0 then xs.zip(ys)
else for (x, y) <- pair(xs.map(_ + 1), ys, n - 1) yield (x, y)
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't seem to be a compelling case, because I would expect it to be written

Suggested change
else for (x, y) <- pair(xs.map(_ + 1), ys, n - 1) yield (x, y)
else pair(xs.map(_ + 1), ys, n - 1)

to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a test case to verify that it is rewritten to a tail recursive function.

@odersky
Copy link
Contributor Author

odersky commented Jan 18, 2023

Yes, it's technically a spec change, so might require a SIP. Let's discuss at the next meeting. /cc @julienrf

@odersky odersky marked this pull request as draft January 18, 2023 16:13
@odersky
Copy link
Contributor Author

odersky commented Jan 20, 2023

This PR changes the spec how for expressions are generated. Where previously, we always ended with map for a for-yield, the map is now dropped if it would be the identity. This change helps performance and makes more for expressions tailcalls. But it is technically a spec change, since for expressions convert to map by name, and on some types a map with the identity function might not be the identity itself.

So there's a tradeoff here. We can make the spec more complicated to support more convenience and more use cases. But there's a price to pay in terms of more complicated spec.

Another more involved area affects for expressions that contain = bindings. Example:

val as = for x <- xs y = x + 1 z <- zs yield x + y + z

This expression is currently translated like this:

val as = xs.map { x => val y = x + 1 (a, y) }.flatMap { case (x, y) => zs.map { z => x + y + z } }

which is quite a mess. These complications are unavoidable if there is a filter after the = binding of y. But in this case there isn't we could generate the following simpler, faster, and clearer code:

val as = xs.flatMap { x => val y = x + 1 zs.map { z => x + y + z } }

But we'd need a spec change again that states these different rules for for expressions that do not contain filters. (and for for-do expressions, where that code is always possible.) So again we are trading complexity of translation rules (i.e. specification) against complexity of translation results.

Finally, if we go with the more complicated translation rules, we could also envisage different syntax.

  • allow a leading =
  • don't require a trailing yield, extend in that case last expression expr to x <- expr yield x

Example:

 for x = a y <- ys x2 = x + y f(x, y, x2)

To move this forward, we'd need someone to develop and champion a SIP. I probably won't have the bandwidth to do it personally.

See also: #2573

@odersky
Copy link
Contributor Author

odersky commented Jan 26, 2023

Going to close this for now, since it needs many more steps to mature. To be re-opened when somebody else picks this up.

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

Labels

None yet

2 participants