Skip to content

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented May 8, 2025

Fixes #23119

A given pattern in a for comprehension results in a fresh val in the body of the mapping function, but it should have the same (arbitrary) name as in the rest of the expansion.

This commit gives the given its usual given name (in makeIdPat) so that the unused check can check it.

Currently, without -preview, showing that $1$ is called given_Int by typer:

➜  scala-cli repl --server=false -S 3.7.2-RC1 -Vprint:typer,refchecks -Wunused:all
Welcome to Scala 3.7.2-RC1 (17.0.15, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> for given Int <- 1 to 2; j: Int = summon[Int] yield j
1 warning found
[[syntax trees at end of                     typer]] // rs$line$1
package <empty> {
  final lazy module val rs$line$1: rs$line$1 = new rs$line$1()
  final module class rs$line$1() extends Object() { this: rs$line$1.type =>
    val res0: IndexedSeq[Int] =
      intWrapper(1).to(2).map[(Int, Int)]((x$1: Int) =>
        x$1:Int @unchecked match
          {
            case given $1$ @ _:Int =>
              val j: Int = $1$
              Tuple2.apply[Int, Int]($1$, j)
          }
      ).map[Int]((x$1: (Int, Int)) =>
        x$1:(x$1 : (Int, Int)) @unchecked match
          {
            case Tuple2.unapply[Int, Int](given given_Int @ _:Int, j @ _:Int)
               => j:Int
          }
      )
  }
}

Without -preview, this commit:

scala> for given Int <- 1 to 2; j: Int = summon[Int] yield j
[[syntax trees at end of                     typer]] // rs$line$1
package <empty> {
  final lazy module val rs$line$1: rs$line$1 = new rs$line$1()
  final module class rs$line$1() extends Object() { this: rs$line$1.type =>
    val res0: IndexedSeq[Int] =
      intWrapper(1).to(2).map[(Int, Int)]((x$1: Int) =>
        x$1:Int @unchecked match
          {
            case given given_Int @ _:Int =>
              val j: Int = given_Int
              Tuple2.apply[Int, Int](given_Int, j)
          }
      ).map[Int]((x$1: (Int, Int)) =>
        x$1:(x$1 : (Int, Int)) @unchecked match
          {
            case Tuple2.unapply[Int, Int](given given_Int @ _:Int, j @ _:Int)
               => j:Int
          }
      )
  }
}

With -preview, the tupling map is eliminated early:

scala> for given Int <- 1 to 2; j: Int = summon[Int] yield j
[[syntax trees at end of                     typer]] // rs$line$1
package <empty> {
  final lazy module val rs$line$1: rs$line$1 = new rs$line$1()
  final module class rs$line$1() extends Object() { this: rs$line$1.type =>
    val res0: IndexedSeq[Int] =
      intWrapper(1).to(2).map[Int]((x$1: Int) =>
        x$1:Int @unchecked match
          {
            case given given_Int @ _:Int =>
              val j: Int = given_Int
              j:Int
          }
      )
  }
}

@som-snytt som-snytt force-pushed the issue/23119-for-given branch from def4dd7 to 4d0f402 Compare May 9, 2025 06:19
@som-snytt som-snytt changed the title Detect anonymized patvar in given pattern De-anonymize patvar in given pattern May 9, 2025
@som-snytt som-snytt marked this pull request as ready for review May 9, 2025 06:55
@som-snytt som-snytt force-pushed the issue/23119-for-given branch 2 times, most recently from 1df2c1d to c9a9dbc Compare May 9, 2025 07:08
@Gedochao Gedochao requested a review from tgodzik May 9, 2025 08:25
@Gedochao Gedochao requested a review from sjrd May 9, 2025 12:15
@Gedochao Gedochao assigned sjrd and unassigned tgodzik May 9, 2025
@som-snytt som-snytt force-pushed the issue/23119-for-given branch from c9a9dbc to 26578ca Compare June 28, 2025 19:22
@som-snytt som-snytt changed the title De-anonymize patvar in given pattern Invent given pattern name in for comprehension Jun 28, 2025
@som-snytt som-snytt removed the request for review from tgodzik June 28, 2025 20:05
@som-snytt
Copy link
Contributor Author

@sjrd Minimized commit!

@som-snytt som-snytt force-pushed the issue/23119-for-given branch from 26578ca to 71280c8 Compare August 28, 2025 18:17
@som-snytt
Copy link
Contributor Author

I knew I'd fixed the spelling of anonympus somewhere, though I am rather sorry to have fixed it.

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.

I think this will break scenarios where there are two given captures for two types where inventGivenName produces the same name. For example a given foo: List[Option[Int]] and a given bar: List[Option[String]].

We should at the very least add a test for such a situation, make sure that both can still be resolved.

@som-snytt
Copy link
Contributor Author

som-snytt commented Aug 29, 2025

@sjrd I considered that because I just noted elsewhere (#23832 (comment)) that local givens can have clashing names but could just as well have fresh names (modulo code can refer to the invented name).

I'll add tests a test for clashing and for referring.

@som-snytt
Copy link
Contributor Author

Actually, as shown in the -Vprint above, the invented names are already used in map desugaring, so

scala> for x <- 1 to 2; given Option[List[String]] = Some(List(x.toString)); given Option[List[Int]] = Some(List(x)) yie
ld summon[Option[List[String]]]
-- [E005] Naming Error: --------------------------------------------------------
1 |for x <- 1 to 2; given Option[List[String]] = Some(List(x.toString)); given Option[List[Int]] = Some(List(x)) yield summon[Option[List[String]]]
  |                                                                      ^^^^^^^^^^^^^^^^^^^^^^^
  |                             duplicate pattern variable: given_Option_List
  |

because

      ).map[Option[List[String]]]((
        x$1: (Int, Option[List[String]], Option[List[Int]])) =>
        x$1:(x$1 : (Int, Option[List[String]], Option[List[Int]])) @unchecked
           match
          {
            case
              Tuple3.unapply[Int, Option[List[String]], Option[List[Int]]](
                x @ _, given given_Option_List @ _:Option[List[String]],
                given given_Option_List @ _:Option[List[Int]])
             => given_Option_List:Option[List[String]]
          }
      )

It progresses to clashing locals.

  |given_Option_List is already defined as given instance given_Option_List

But it would be best if the additional help from "double definition" were available for these cases. (Because givens are so mysterious? Because anonymous givens are common and fragile?)

@som-snytt som-snytt force-pushed the issue/23119-for-given branch from 71280c8 to 52832f8 Compare August 29, 2025 15:35
@som-snytt som-snytt requested a review from sjrd August 29, 2025 16:21
@mr-git
Copy link

mr-git commented Aug 29, 2025

Maybe there is a way to name givens in for-comprehension? Like:

for given j: Int <- 1 to 2 yield j

or something like this?

I failed to find a way to set dedicated name for a given.

@som-snytt
Copy link
Contributor Author

som-snytt commented Aug 29, 2025

@mr-git See the test, the syntax is name @ given T for a pattern.

In the summary it's called Pattern2.

Edit: don't be fooled by the way it prints under -Vprint.

@mr-git
Copy link

mr-git commented Aug 29, 2025

@som-snytt thank you so much! I couldn't imagine such a way to write it - even after using case implicit0(x: X) <- ??? in many places!

Only place where I found such usage is Pattern-Bound Given Instances, but I didn't imagine that it could be used in for-comprehension too. Sadly there are zero examples for such use-case in docs.

And IDEA doesn't provide hint to change from:

for {
  //...
  given X = getX()
  x = summon[X]
  //..

to

for {
  //...
  x @ given X = getX()
  //..

Honestly, I don't know many language users, who would go through the syntax summary.

@som-snytt
Copy link
Contributor Author

@mr-git thanks, I'd forgotten the name for it, but that was the answer to "what is the replacement for f { implicit p => }?"

That is, f { case p @ given P => }, though the idiomatic answer is "use context functions".

I found an example #12676

My point is that it is a FAQ but maybe not written up in a helpful document. I don't see it in the migration guide (at a glance).

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.

false positive warning
4 participants