betterFors trailing `map` elimination can change the resulting type

[KacperFKorban]

Compiler version

3.6.1-RC1-bin-20241017-59b67fc-NIGHTLY

Minimized code

//>  using scala 3.nightly

import scala.language.experimental.betterFors

case class Container[A](val value: A) {
  def map[B](f: A => B): Container[B] = Container(f(value))
}

sealed trait Animal
case class Dog() extends Animal

def opOnDog(dog: Container[Dog]): Container[Animal] =
  for
    v <- dog
  yield v

Output

-- [E007] Type Mismatch Error: /home/kpi/bugs/better-fors-bug.scala:13:2 -------
13 |  for
   |  ^
   |  Found:    (dog : Container[Dog])
   |  Required: Container[Animal]
14 |    v <- dog
15 |  yield v
   |
   | longer explanation available when compiling with `-explain`

without betterFors enabled, this code compiles successfully.

Expectation

The type of the for-comprehension should not be changed after enabling the betterFors language extension.

[sjrd]

I believe this one must be considered an intended source breakage. That for comprehension collapses to just dog. Under the new rules, if one wants to force a map that "does nothing" except change the result type, they have to write it: dog.map(d => d).

[odersky]

One possibility is to do the reduction from expr.map(x => x) to expr after Typer. We'd have to mark these for-generated maps with an attachment so that we do it just for them and not for all such expressions. The spec would then have to be amended to pick the previous expansion but state that the compiler will eliminate the trailing map after type checking.

It's a bit messier than what we have now. Not sure whether we want to do this anyway in order to avoid the extensive breakages in the open CB.

[bishabosha]

and also you can detect if elimination would change the type of expression, issue a migration warning, and suggest workaround like yield v: Animal?

[KacperFKorban]

@odersky I proposed a similar transformation to what you're describing as an addition to the SIP-62 here: scala/improvement-proposals#79 (comment) (and opened an extremely silent pre-SIP https://contributors.scala-lang.org/t/pre-sip-sip-62-addition-proposal-better-effect-loops-with-for-comprehensions/6759)
That one was for removing yield (), but the principle should be the same.
I have a PoC here: KacperFKorban@31cbd47

[KacperFKorban]

@sjrd @odersky Do you have any strong opinions about changing the implementation of the trailing map elimination? Would that require a new SIP/addendum? Technically this would only change the implementation details.

[sjrd]

It's obviously not an implementation detail, since it yields a different result type. 😉

It's at least a concern that could be brought back to the SIP committee for reconsideration.

[KacperFKorban]

@sjrd Right. If we don't consider this a bug, but simply a behavior change, then you're right.
So what are the appropriate steps now? Should I write down the suggested changes to the betterFors SIP and open a PR with the changes to scala/improvement-proposals? Should I start working on the implementation in parallel?

[odersky]

I think for all intents and purposes, this should be considered a bug. We can't simply break half a dozen projects and then reconsider later.

[sjrd]

If it's a bug, it's a spec bug, because AFAICT the implementation is directly following the SIP. So changing it needs to back to SIP anyway.

This is exactly the point of having two stages to the process: so that the implementation phase can inform back into the design of the approved spec proves to be impractical.

[KacperFKorban]

I drafted the changes to the SIP: scala/improvement-proposals#99