Skip to content

Make SIP-62 - betterFors a standard feature #22652

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

Merged
merged 6 commits into from
Mar 11, 2025

Conversation

KacperFKorban
Copy link
Member

@KacperFKorban KacperFKorban commented Feb 24, 2025

  • Deprecate experimental language import
  • Make betterFors conditional on -source >= 3.7 instead
  • Drop the experimental.betterFors import from tests

@KacperFKorban KacperFKorban added release-notes Should be mentioned in the release notes needs-minor-release This PR cannot be merged until the next minor release labels Feb 24, 2025
@KacperFKorban
Copy link
Member Author

While working on this, I discovered an unhandled/bug case that apparently isn't used anywhere in the open community build – nested given bindings inside of a pattern of an alias. I added a quickfix for it.
e.g.

for
    x <- Option(23 -> "abc")
    (a @ given Int, b @ given String) = x
    _ <- Option(1)
  yield
    assert(summon[Int] == 23)

betterFors generated:

Option.apply[(Int, String)](ArrowAssoc[Int](23).->[String]("abc")).flatMap
  [Nothing]((x: (Int, String)) =>
  {
    val $2$: ((Int, String), Int, String) =
      x:(Int, String) @unchecked match 
        {
          case 
            $1$ @
              Tuple2.unapply[Int, String](given a @ _:Int,
                given b @ _:String)
            => Tuple3.apply[(Int, String), Int, String]($1$, a, b)
        }
    val $1$: (Int, String) = $2$._1
    val a: Int = $2$._2
    val b: String = $2$._3
    Option.apply[Int](1).map[Nothing]((x$1: Int) =>
      x$1:Int @unchecked match 
        {
          case _ => assert(summon[Int](/* missing */summon[Int]).==())
        }
    )
  }
)

After the fix, it generates:

Option.apply[(Int, String)](ArrowAssoc[Int](23).->[String]("abc")).map[
  ((Int, String), (Int, String))]((x: (Int, String)) =>
  {
    val $2$: ((Int, String), Int, String) =
      x:(Int, String) @unchecked match 
        {
          case 
            $1$ @
              Tuple2.unapply[Int, String](given a @ _:Int,
                given b @ _:String)
            => Tuple3.apply[(Int, String), Int, String]($1$, a, b)
        }
    val $1$: (Int, String) = $2$._1
    val a: Int = $2$._2
    val b: String = $2$._3
    Tuple2.apply[(Int, String), (Int, String)](x, $1$)
  }
).flatMap[Unit]((x$1: ((Int, String), (Int, String))) =>
  x$1:(x$1 : ((Int, String), (Int, String))) @unchecked match 
    {
      case 
        Tuple2.unapply[(Int, String), (Int, String)](x @ _,
          Tuple2.unapply[Int, String](given a @ _:Int, given b @ _:String)
          )
        =>
        Option.apply[Int](1).map[Unit]((x$1: Int) =>
          x$1:Int @unchecked match 
            {
              case _ =>
                if a.==(23).unary_! then
                  scala.runtime.Scala3RunTime.assertFailed() else ()
            }
        )
    }
)

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.

Great to have this for 3.7

@odersky
Copy link
Contributor

odersky commented Mar 10, 2025

Needs to resolve a conflict., otherwise ready to go in.

@odersky odersky assigned KacperFKorban and unassigned odersky Mar 10, 2025
@odersky odersky added this to the 3.7.0 milestone Mar 10, 2025
@KacperFKorban KacperFKorban enabled auto-merge (squash) March 10, 2025 12:24
@KacperFKorban KacperFKorban merged commit 87f0078 into scala:main Mar 11, 2025
29 checks passed
@KacperFKorban KacperFKorban deleted the betterFors-standard branch March 11, 2025 15:26
@tgodzik
Copy link
Contributor

tgodzik commented Mar 11, 2025

@KacperFKorban The oficial decision was to have it in preview mode for 3.7 and only stabilize in 3.8 to be on the safe side. Could add it to preview in a follow up? Sorry for raising it just now, I think this wasn't communicated clearly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants