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

Streamline translation of for expressions #16703

Closed
wants to merge 1 commit into from

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants