Skip to content

Regression in eta-expansion while overloading as of 3.3.4 #21727

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

Closed
majk-p opened this issue Oct 8, 2024 · 7 comments
Closed

Regression in eta-expansion while overloading as of 3.3.4 #21727

majk-p opened this issue Oct 8, 2024 · 7 comments
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:wontfix

Comments

@majk-p
Copy link

majk-p commented Oct 8, 2024

Compiler version

The attached code compiled well up to 3.3.3 and stopped compiling with 3.3.4. Compilation also fails on latest 3.5.1.

Minimized code

Also available as a gist https://gist.github.com/majk-p/b510b46cb38ecbb9bc7dd29c30a1055f

//> using dep "org.typelevel::cats-effect:3.5.4"
//> using scala "3.3.3"

import cats.Functor
import cats.effect.std.UUIDGen
import cats.syntax.all.*

import java.util.UUID

final class MyId private (val id: String)

object MyId {

  def fromUUID[F[_]: Functor: UUIDGen]: F[MyId] =
    UUIDGen[F].randomUUID.map(fromUUID)
    // UUIDGen[F].randomUUID.map(MyId.fromUUID)     // this also doesn't compile
    // UUIDGen[F].randomUUID.map(fromUUID(_))       // this on the other hand compiles
    // UUIDGen[F].randomUUID.map(MyId.fromUUID(_))  // this also compiles

  private def fromUUID(id: UUID): MyId =
    MyId(id.toString())
}

Notice how this only affects the short syntax without explicit parentheses. Adding (_) solves the issue. Changing the second method name to anything non-ambiguous also fixes the example, this means following example also works:

object MyId {

  def fromUUID[F[_]: Functor: UUIDGen]: F[MyId] =
    UUIDGen[F].randomUUID.map(otherMethodName)

  private def otherMethodName(id: UUID): MyId =
    MyId(id.toString())
}

Output

[error] ./main.scala:15:39
[error] Ambiguous given instances: both method catsFlatMapForSortedMap in object Invariant and method catsFlatMapForMap in object Invariant match type cats.Functor[F] of a context parameter of method fromUUID in object MyId
[error]     UUIDGen[F].randomUUID.map(fromUUID)
[error]    

Expectation

Compiles with no issues

Additional research

I found the great bisect tool in compiler sources and run it against my example code. Here's the output:

6f33c6128df2168f4e9fb3911491f466f6fe9f90 is the first bad commit
commit 6f33c6128df2168f4e9fb3911491f466f6fe9f90
Author: Wojciech Mazur <wmazur@virtuslab.com>
Date:   Fri Jun 28 18:20:54 2024 +0200

    error when reading class file with unknown newer jdk version
    
    
    [Cherry-picked f430e449869d9d6b6cf05373086f3d52b0a11805][modified]

 .../dotc/core/classfile/ClassfileConstants.scala   |  1 +
 .../dotc/core/classfile/ClassfileParser.scala      | 34 +++++++++++++---------
 2 files changed, 21 insertions(+), 14 deletions(-)
bisect found first bad commit
Previous HEAD position was 6f33c6128d error when reading class file with unknown newer jdk version
HEAD is now at e76de95ff8 Release 3.3.4

This is how I configured the bisect tool:

scala-cli project/scripts/bisect.scala -- --releases 3.3.2-RC1-bin-20230724-ce1ce99-NIGHTLY...3.3.5-RC1-bin-20240712-4eb7668-NIGHTLY compile https://gist.github.com/majk-p/b510b46cb38ecbb9bc7dd29c30a1055f

It looks like #20862 is the PR that introduced the regression. CC: @WojciechMazur @bishabosha

@majk-p majk-p added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 8, 2024
@WojciechMazur
Copy link
Contributor

Thank you for the bug report!

Bisect on main points to 0337efd
Last good release: 3.4.1-RC1-bin-20240210-6efcdba-NIGHTLY
First bad release: 3.4.1-RC1-bin-20240212-c529a48-NIGHTLY

I'm preparing a self-contained minimization

@WojciechMazur
Copy link
Contributor

Self contained minimization with the same error:

//> using options -Ykind-projector:underscores

import java.util.UUID

object MyId:
  def fromUUID[F[_]: Functor: UUIDGen]: F[String] =
    toFunctorOps(UUIDGen[F].randomUUID).map(fromUUID) // error
  private def fromUUID(id: UUID): String = ???

object UUIDGen:
  def apply[F[_]](implicit ev: UUIDGen[F]): UUIDGen[F] = ev
trait UUIDGen[F[_]]:
  def randomUUID: F[UUID]

trait FlatMap[F[_]] extends Functor[F]
trait Functor[F[_]] extends Invariant[F]
trait Invariant[F[_]]
object Invariant:
  implicit def catsFlatMapForMap[K]: FlatMap[Map[K, _]] = ???
  implicit def catsFlatMapForSortedMap[K]: FlatMap[scala.collection.SortedMap[K, _]] = ???

implicit def toFunctorOps[F[_], A](target: F[A])(implicit tc: Functor[F]): Ops[F, A] { type TypeClassType = Functor[F]} = 
  new Ops[F, A] { type TypeClassType = Functor[F] }

trait Ops[F[_], A] {
  type TypeClassType <: Functor[F]
  def map[B](f: A => B): F[B] = ???
}

The clue of the issue seems to be fact that Functor context bound parameter is being ignored, thus the issue can be further minimized to

type UUID = String
object MyId:
  def fromUUID[F[_]: Functor: UUIDGen]: F[String] =
    toFunctorOps(UUIDGen[F].randomUUID).map(fromUUID) // error
  private def fromUUID(id: UUID): String = ???

object UUIDGen:
  def apply[F[_]](implicit ev: UUIDGen[F]): UUIDGen[F] = ev
trait UUIDGen[F[_]]:
  def randomUUID: F[UUID]

trait Functor[F[_]]
implicit def toFunctorOps[F[_], A](target: F[A])(implicit tc: Functor[F]): Ops[F, A] { type TypeClassType = Functor[F]} = 
  new Ops[F, A] { type TypeClassType = Functor[F] }

trait Ops[F[_], A] {
  type TypeClassType <: Functor[F]
  def map[B](f: A => B): F[B] = ???
}

Failing with

[error] ./test_selfcontained.scala:4:53
[error] No given instance of type Functor[F] was found for a context parameter of method fromUUID in object MyId
[error] 
[error] where:    F is a type variable with constraint <: [R] =>> String => R
[error]     toFunctorOps(UUIDGen[F].randomUUID).map(fromUUID)
[error]                                                     ^

@WojciechMazur WojciechMazur added area:typer regression This worked in a previous version but doesn't anymore and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 8, 2024
@Gedochao
Copy link
Contributor

@WojciechMazur note: Ykind-projector is deprecated syntax, let's keep to:

//> using options -Xkind-projector:underscores

@Gedochao
Copy link
Contributor

also, cc @odersky

@odersky
Copy link
Contributor

odersky commented Oct 17, 2024

Yes, I think we hit the limits of SAM expansion here. I would recommend one of the workarounds that were already listed.

@dwijnand dwijnand changed the title Regresssion in short syntax method invocation introduced in 3.3.4 Regression in short syntax method invocation introduced in 3.3.4 Oct 17, 2024
@dwijnand dwijnand changed the title Regression in short syntax method invocation introduced in 3.3.4 Regression in eta-expansion as of 3.3.4 Oct 25, 2024
@dwijnand dwijnand changed the title Regression in eta-expansion as of 3.3.4 Regression in eta-expansion while overloading as of 3.3.4 Oct 25, 2024
@odersky
Copy link
Contributor

odersky commented Oct 27, 2024

I dug some more why this happens. The critical code that was deleted in 0337efd was this:

      val compat0 = pt.dealias match
          case defn.FunctionNOf(args, resType, _) =>
            narrowByTypes(alts, args, resType)
          case _ =>
            Nil
      if compat0.isEmpty then ...
      else compat0

In other words, if the expected type was a function type, we do a narrowByTypes, which will end up preferring method alternatives over normal alternatives. I don't know why the code was added or what it was supposed to solve. It's a bit weird, since everywhere else we prever values over methods. If I add this clause back in, the example compiles because with its help we decide that the method type

private def fromUUID(id: UUID): String = ???

looks more like the expected function type than the alternative. But a full comparison would pick the alternative and then fail with a subsequent implicit search.

The dubious clause had to be deleted because it causes other failures and has no logic justification. So in the interest of sanity it will stay deleted.

@Gedochao
Copy link
Contributor

It looks like we won't be fixing this. Closing it.

@Gedochao Gedochao closed this as not planned Won't fix, can't repro, duplicate, stale Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:typer itype:bug regression This worked in a previous version but doesn't anymore stat:wontfix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants