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

SIP-62 - For comprehension improvements #20522

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

KacperFKorban
Copy link
Member

@KacperFKorban KacperFKorban commented Jun 4, 2024

Implementation for SIP-62.

Summary of the changes

For more details read the committed markdown file here: scala/improvement-proposals#79

This introduces improvements to for comprehensions in Scala to improve usability and simplify desugaring. The changes are hidden behind a language import scala.language.experimental.betterFors.
The main changes are:

  1. Starting for comprehensions with aliases:

    • Current Syntax:
      val a = 1
      for {
        b <- Some(2)
        c <- doSth(a)
      } yield b + c
    • New Syntax:
      for {
        a = 1
        b <- Some(2)
        c <- doSth(a)
      } yield b + c
  2. Simpler Desugaring for Pure Aliases:

    • Current Desugaring:
      for {
        a <- doSth(arg)
        b = a
      } yield a + b
      Desugars to:
      doSth(arg).map { a =>
        val b = a
        (a, b)
      }.map { case (a, b) =>
        a + b
      }
    • New Desugaring: (where possible)
      doSth(arg).map { a =>
        val b = a
        a + b
      }
  3. Avoiding Redundant map Calls:

    • Current Desugaring:
      for {
        a <- List(1, 2, 3)
      } yield a
      Desugars to:
      List(1, 2, 3).map(a => a)
    • New Desugaring:
      List(1, 2, 3)

@lihaoyi
Copy link
Contributor

lihaoyi commented Jun 12, 2024

@KacperFKorban Looks reasonable to me. @odersky could you recommend someone to review the code from the dotty side of things?

@nafg
Copy link

nafg commented Jun 21, 2024

Can I now write a for comprehension with only aliases, no generators?

@KacperFKorban
Copy link
Member Author

Can I now write a for comprehension with only aliases, no generators?

No, the for-comprehension will still have to contain at least one generator to be valid.

@nafg
Copy link

nafg commented Jun 24, 2024

Can I now write a for comprehension with only aliases, no generators?

No, the for-comprehension will still have to contain at least one generator to be valid.

Maybe it's out of scope, but I think there could be value in lifting that restriction. That would allow for to be used similar to let...in in Haskell, i.e. a shorter notation for an expression broken out into many vals.

aaa x y = let r = 3 
              s = 6
              in  r*x + s*y

Current Scala:

def aaa(x: Double, y: Double) = {
  val r = 3
  val s = 6
  r*x + s*y
}

With generator-less for:

def aaa(x: Double, y: Double) =
  for {
    r = 3
    s = 6
  }
  yield r*x + s*y

Perhaps it would be more similar to let...in (and perhaps more worthwhile) with braceless syntax, than those snippets.

@KacperFKorban
Copy link
Member Author

@nafg I think that it is out of scope for this SIP. I also think that reusing the for-comprehension syntax for other usages will add confusion, but that's just an opinion. Either way, you might want to create a separate pre-SIP for that on https://contributors.scala-lang.org/

compiler/src/dotty/tools/dotc/ast/untpd.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/StdNames.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/ast/Desugar.scala Outdated Show resolved Hide resolved
@odersky
Copy link
Contributor

odersky commented Jul 19, 2024

Sorry it took so long to review. I think we are close, let's just refactor to keep it DRY.

@odersky odersky assigned KacperFKorban and unassigned odersky Jul 19, 2024
@KacperFKorban KacperFKorban force-pushed the improved-fors branch 2 times, most recently from 4b67eb0 to d51b7c0 Compare July 22, 2024 18:10
odersky and others added 4 commits July 22, 2024 20:10
 - [] 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.
- Allow `for`-comprehensions to start with aliases desugaring them into
  valdefs in a new block
- Desugar aliases into simple valdefs, instead of patterns when they are
  not followed by a guard
- Add an experimental language flag that enables the new desugaring
  method
@KacperFKorban
Copy link
Member Author

@odersky Thanks for the review, it should be closer to DRY now. See if there is anything else I should correct.

@KacperFKorban KacperFKorban requested a review from odersky July 23, 2024 11:16
@KacperFKorban KacperFKorban assigned odersky and unassigned smarter Jul 23, 2024
@KacperFKorban KacperFKorban removed their assignment Jul 23, 2024
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

Looks all good now.

@odersky odersky merged commit e261fa2 into scala:main Aug 1, 2024
28 checks passed
@odersky odersky deleted the improved-fors branch August 1, 2024 17:41
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
@WojciechMazur WojciechMazur added the release-notes Should be mentioned in the release notes label Oct 8, 2024
@bishabosha
Copy link
Member

Are there any docs for this feature?

@KacperFKorban
Copy link
Member Author

Hmmm, I don't think so. Should I write some?

@bishabosha
Copy link
Member

bishabosha commented Oct 15, 2024

Every language level change is meant to come with a spec in the Scala 3 reference

@odersky
Copy link
Contributor

odersky commented Oct 15, 2024

Yes, it would be good if you could write a spec page for https://docs.scala-lang.org/scala3/reference/. These pages are in
docs/_docs/reference. Probably under "Changed features".

@bishabosha
Copy link
Member

right now it still has the experimental flag, was it approved for stabilisation?

@lihaoyi
Copy link
Contributor

lihaoyi commented Oct 15, 2024

It wasn't approved yet, IIRC one of the blockers was that we wanted to run the community build with it turned on by default to see if anything breaks. @WojciechMazur is that something you can help with?

@WojciechMazur
Copy link
Contributor

I've started the build, I'll be back later with the results

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Oct 17, 2024

There is a plenty of projects that start to fail with this experimental feature enabled. Most of issues seems to be related to type inference.

@KacperFKorban Can I leave it here and let you create a dedicated issues/prs with proper minimization?

Project Version Build URL Notes
apimorphism/telegramium 9.710.0 Open CB logs
argonaut-io/argonaut 6.3.10 Open CB logs
dispatch/reboot 2.0.0 Open CB logs
jd557/minart 0.6.1 Open CB logs
thoughtworksinc/dsl.scala 2.0.0 Open CB logs
virtuslab/avocado 0.2.0 Open CB logs
xuwei-k/optparse-applicative 0.9.4 Open CB logs
zio/zio 2.1.9 Open CB logs
disneystreaming/smithy4s 0.18.24 -> 0.18.25 Open CB logs

I've also found 4 projects that fail to build with only -experimental flag enabled without using any experimental features. I'll try to reproduce these and create dedicated issues.

@ghostdogpr
Copy link
Contributor

@WojciechMazur I have that issue with the -experimental flag at work, discussed here: #20730

@KacperFKorban
Copy link
Member Author

@WojciechMazur Thanks!
I'll take a look and try to minimize them into separate issues.

Regarding the -experimental issue, it might be related to this (scala3#20730) issue about compiling stdlib with -experimental.

@KacperFKorban
Copy link
Member Author

Summary of my findings:

Project Version Build URL Notes
apimorphism/telegramium 9.710.0 Open CB logs inference bug because of trailing map elimination
argonaut-io/argonaut 6.3.10 Open CB logs inference bug because of trailing map elimination
dispatch/reboot 2.0.0 Open CB logs inference bug because of trailing map elimination
jd557/minart 0.6.1 Open CB logs inference bug because of trailing map elimination
thoughtworksinc/dsl.scala 2.0.0 Open CB logs inference bug because of trailing map elimination
virtuslab/avocado 0.2.0 Open CB logs this is my library for rewriting for-comprehensions, so it was obvious that this one was going to fail, since the desugaring is different now 😅
xuwei-k/optparse-applicative 0.9.4 Open CB logs inference bug because of trailing map elimination
zio/zio 2.1.9 Open CB logs some parser crash related to rewriting, not sure how this can be relevant 🤔 + inference bug because of trailing map elimination
disneystreaming/smithy4s 0.18.24 -> 0.18.25 Open CB logs -experimental fail

I think that all these issues are one and the same, I created an issue for it: #21804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants