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

Strict equality is too strict in Scala 3.5 #819

Open
mbovel opened this issue Aug 29, 2024 · 7 comments
Open

Strict equality is too strict in Scala 3.5 #819

mbovel opened this issue Aug 29, 2024 · 7 comments

Comments

@mbovel
Copy link

mbovel commented Aug 29, 2024

In Scala 3.5, assertEquals doesn't enable to compareSet of different types:

//> using dep org.scalameta::munit::1.0.1

class FooTest extends munit.FunSuite {
  test("foo") {
    assertEquals(Set(2: Any), Set(2))
  }
}
Compiling project (test, Scala 3.5.0, JVM (17))
[error] ./foo.test.scala:5:5
[error] Can't compare these two types:
[error]   First type:  Set[Any]
[error]   Second type: Set[Int]
[error] Possible ways to fix this error:
[error]   Alternative 1: provide an implicit instance for Compare[Set[Any], Set[Int]]
[error]   Alternative 2: upcast either type into `Any` or a shared supertype.
[error] I found:
[error] 
[error]     munit.Compare.compareSubtypeWithSupertype[Set[Any], Set[Int]](
[error]       /* missing */summon[Set[Any] <:< Set[Int]])
[error] 
[error] But no implicit values were found that match type Set[Any] <:< Set[Int].
[error]     assertEquals(Set(2: Any), Set(2))
[error]     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[[syntax trees at end of                     typer]] // /Users/mbovel/munit-bug/foo.test.scala
package <empty> {
  class FooTest() extends munit.FunSuite() {
    this.test("foo")(
      {
        {
          val obtained$1: Set[Any] = Set.apply[Any]([2:Any : Any]*)
          val expected$1: Set[Int] = Set.apply[Int]([2 : Int]*)
          def clue$1: String = this.assertEquals$default$3[Set[Any], Set[Int]]
          this.assertEquals[Set[Any], Set[Int]](obtained$1, expected$1, clue$1)(
            loc = munit.Location.generate,
            munit.Compare.compareSubtypeWithSupertype[Set[Any], Set[Int]](
              /* missing */summon[Set[Any] <:< Set[Int]])
          )
        }
      }
    )
  }
}

Error compiling project (test, Scala 3.5.0, JVM (17))
Compilation failed

This is because in 3.5, the B type parameter of assertEquals is inferred to be Set[Int] instead of Set[Any] as in 3.3

assertEquals's signature:

def assertEquals[A, B](
obtained: A,
expected: B,
clue: => Any = "values are not the same"
)(implicit loc: Location, compare: Compare[A, B]): Unit = {

Instances of Compare are defined as:

trait ComparePriority1 extends ComparePriority2 {
implicit def compareSubtypeWithSupertype[A, B](implicit
ev: A <:< B
): Compare[A, B] = Compare.defaultCompare
}

trait ComparePriority2 {
implicit def compareSupertypeWithSubtype[A, B](implicit
ev: A <:< B
): Compare[B, A] = Compare.defaultCompare
}

@mbovel
Copy link
Author

mbovel commented Aug 29, 2024

@EugeneFlesselle notes that in Scala 3, we could move the implicit parameters list to solve the issue in this specific use case:

def assertEquals2[A, B](using loc: Location, compare: Compare[A, B])(
      obtained: A,
      expected: B,
      clue: => Any = "values are not the same"
  ): Unit = assertEquals(obtained, expected, clue)

Note sure this would be a general fix though.

@tgodzik
Copy link
Contributor

tgodzik commented Aug 30, 2024

Thanks for reporting! Isn't this expected though? I don't the exact background behind strictEquality, but since Set is invariant I would assume sets of different types are indeed not comparable.

If needed we can include the proposed fix, but I am not sure about what cases the current behaviour would be problematic in.

@EugeneFlesselle
Copy link

Isn't this expected though?

That depends on your expectations of type inference.

  • Before we would infer the type parameter of the second set to also be Any since that is the only instantiation for which an implicit value of type Compare could be found.
  • Now we first instantiate the type variable of the second set to be Int (as we would if it was val binded) and then proceed with implicit search. In the above example, the change in inference causes a new error. In other cases, the earlier instantiation can in contrary reduce the possible ambiguities in the implicit search.

@tgodzik
Copy link
Contributor

tgodzik commented Aug 30, 2024

That's a tough one, I imagine both bahaviours to be expected at times:

  1. let's say we compare Set[Int] and Set[Any] -> we actually didn't want that, we did something wrong with the second param to get Any
  2. on the other hand let's say Set[Animal] and Set[Bird] -> I might have some birds in the animal set so we would be all good

Wouldn't the current behaviour in 3.5 be actually safer? If you want to be explicit you can cast the set of birds to animals.

On (third?) hand we will make it work with Lists, but they are covariant, so this is expected.

@mbovel
Copy link
Author

mbovel commented Aug 30, 2024

on the other hand let's say Set[Animal] and Set[Bird] -> I might have some birds in the animal set so we would be all good

Yeah, that's similar to our use-case. I find it reasonable to want to test the return value of method whose return type is defined in terms of an interface (as foo in the following example) by comparing the return value with a concrete instance.

That said, it's worth noting that the current MUnit behavior is in line with Scala's strict equality, so that might be okay.

//> using dep org.scalameta::munit::1.0.1

trait Interface
case class Impl() extends Interface

def foo(): Set[Interface] = Set(Impl())

class FooTest extends munit.FunSuite:
  val s = Set(2)

  test("foo (normal equality)"):
    assert(foo() == Set(Impl())) // Works in Scala 3.3 and 3.5

  test("foo (strict equality)"):
    import scala.language.strictEquality
    assert(foo() == Set(Impl())) // Fails in Scala 3.3 and 3.5
  
  test("foo (assertEquals)"):
    assertEquals(foo(), Set(Impl())) // Works in Scala 3.3, fails in Scala 3.5

@mbovel
Copy link
Author

mbovel commented Aug 30, 2024

Wait. Seeing that there is a CanEqual.canEqualSet defined in the standard library, that might actually be bug! Arguably, the reason CanEqual.canEqualSet has been defined is to mean that sets of different types can be equal if the said types can be equal 🤔

[error] ./foo.test.scala:16:12
[error] Values of types Set[Interface] and Set[Impl] cannot be compared with == or !=.
[error] I found:
[error] 
[error]     CanEqual.canEqualSet[Interface, U](/* missing */summon[CanEqual[Interface, U]])
[error] 
[error] But no implicit values were found that match type CanEqual[Interface, U].
[error]     assert(foo() == Set(Impl())) // Fails in Scala 3.3 and 3.5
[error]            ^^^^^^^^^^^^^^^^^^^^
[error] ./foo.test.scala:19:5

@tgodzik
Copy link
Contributor

tgodzik commented Aug 30, 2024

In case there is a special type class for Scala itself, we could do the same, which would allow us not to change interfaces 🤔

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

No branches or pull requests

3 participants