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

Introduce "strict equality" mode for assertEquals() and friends. #225

Closed
wants to merge 9 commits into from

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Oct 19, 2020

Previously, MUnit had a subtyping constraint on assertEquals(a, b) so
that it would fail to compile if a was not a subtype of b. This was
a suboptimal solution because the compile error messages could become
cryptic in some cases. Additionally, this API didn't integrate with
other libaries like Cats that has its own cats.Eq[A,B] type-class.

Now, MUnit uses a new munit.Compare[A,B] type-class for comparing
values of different types. By default, MUnit provides a "universal"
instance that permits comparison between all types and uses the built-in
== method. Users can optionally enable "strict equality" by adding the
compiler option "-Xmacro-settings.munit.strictEquality" in Scala 2.
In Scala 3, we use the Eql[A, B] type-classes instead to determine
type equality (per http://dotty.epfl.ch/docs/reference/contextual/multiversal-equality.html)

Note: the first few commits are unrelated to this feature, I cleaned up
a few small quirks I noticed while working on this feature.

Fixes #189

@olafurpg
Copy link
Member Author

Big thanks to @gabro who helped a lot with getting this PR into shape 🙏

Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! 😍 Just left some comments here and there

munit/shared/src/main/scala/munit/Assertions.scala Outdated Show resolved Hide resolved
munit/shared/src/main/scala/munit/Assertions.scala Outdated Show resolved Hide resolved
munit/shared/src/main/scala/munit/Compare.scala Outdated Show resolved Hide resolved
Copy link
Member Author

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review @gabro !

munit/shared/src/main/scala/munit/Assertions.scala Outdated Show resolved Hide resolved
munit/shared/src/main/scala/munit/Compare.scala Outdated Show resolved Hide resolved
munit/shared/src/main/scala/munit/Compare.scala Outdated Show resolved Hide resolved
@olafurpg
Copy link
Member Author

Copy-pasting the last commit message explaining the latest approach, which I think comes out quite nicely. Kudos to @gabro for spotting this approach was possible!


Drop strict equality, allow comparison between supertypes/subtypes

This is a fourth attempt at improving strict equality in MUnit
assertEquals() assertions.

  • First attempt (current release version): require second argument to be
    a supertype of the first argument. This has the flaw that the compile
    error message is cryptic and that the ordering of the arguments affects
    compilation.
  • Second attempt: use Eql[A, B] in Scala 3 and allow comparing any
    types in Scala 2. This has the flaw that it's a regression in some
    cases for Scala 2 users and that Eql[A, B] is not really usable
    in its current form, see related discussion
    https://contributors.scala-lang.org/t/should-multiversal-equality-provide-default-eql-instances/4574
  • Third attempt: implement "strict equality" for Scala 2 with a macro
    and Eql[T, T] in Scala. This improves the situation for Scala 2,
    but would mean relying on a feature that we can't easily port to Scala 3.
  • Fourth attempt (this commit): improve the first attempt (current
    release) by allowing Compare[A, B] as long
    as A <:< B OR B <:< A. This is possible thanks to an observation
    by Gabriele Petronella that it's possible to layer the implicits to
    avoid diverging implicit search.

The benefit of the fourth approach is that it works the same way for
Scala 3 and Scala 3. It's very nice that we can avoid macros as well.

Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice! 💣

/**
* Allows comparison between A and B when B is a subtype of A.
*
* This implicit is defined separately from ComparePriority2 in order to avoid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This implicit is defined separately from ComparePriority2 in order to avoid
* This implicit is defined separately from ComparePriority1 in order to avoid


test("chat-int-nok") {
assertNoDiff(
compileErrors("assertEquals('a', 'a'.toInt)"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@olafurpg
Copy link
Member Author

Summarizing the contributors thread in https://contributors.scala-lang.org/t/should-multiversal-equality-provide-default-eql-instances/4574/26?u=olafurpg

The Compare[A, B] type-class that's introduced in this PR does not satisfy compile-time transitivity. For example

class A
B extends A
C extends A

B == A // OK
A == C // OK
B == C // Error

While I agree it's necessary to have transitivity for runtime equality, I am not convinced it's necessary for compile-time equality. The worst-case scenario is that users get a compile-error and need to upcast one of the arguments into the LUB. This is only necessary for classes with custom override def equals() semantics, because it's otherwise always an application bug when you compare two sibling subtypes.

Olafur Pall Geirsson added 9 commits November 1, 2020 10:01
Previously, FailException had some custom nice-to-have features that
ComparisonFailException didn't have.
This makes the error message clearer in some cases.
Previously, MUnit had a subtyping constraint on `assertEquals(a, b)` so
that it would fail to compile if `a` was not a subtype of `b`. This was
a suboptimal solution because the compile error messages could become
cryptic in some cases. Additionally, this API didn't integrate with
other libaries like Cats that has its own `cats.Eq[A,B]` type-class.

Now, MUnit uses a new `munit.Compare[A,B]` type-class for comparing
values of different types. By default, MUnit provides a "universal"
instance that permits comparison between all types and uses the built-in
`==` method. Users can optionally enable "strict equality" by adding the
compiler option `"-Xmacro-settings.munit.strictEquality"` in Scala 2.
In Scala 3, we use the `Eql[A, B]` type-classes instead to determine
type equality.
This is a fourth attempt at improving strict equality in MUnit
`assertEquals()` assertions.

* First attempt (current release version): require second argument to be
  a supertype of the first argument. This has the flaw that the compile
  error message is cryptic and that the ordering of the arguments affects
  compilation.
* Second attempt: use `Eql[A, B]` in Scala 3 and allow comparing any
  types in Scala 2. This has the flaw that it's a regression in some
  cases for Scala 2 users and that `Eql[A, B]` is not really usable
  in its current form, see related discussion
  https://contributors.scala-lang.org/t/should-multiversal-equality-provide-default-eql-instances/4574
* Third attempt: implement "strict equality" for Scala 2 with a macro
  and `Eql[T, T]` in Scala. This improves the situation for Scala 2,
  but would mean relying on a feature that we can't easily port to Scala 3.
* Fourth attempt (this commit): improve the first attempt (current
  release) by allowing `Compare[A, B]` as long
  as `A <:< B` OR `B <:< A`. This is possible thanks to an observation
  by Gabriele Petronella that it's possible to layer the implicits to
  avoid diverging implicit search.

The benefit of the fourth approach is that it works the same way for
Scala 3 and Scala 3. It's very nice that we can avoid macros as well.
@olafurpg olafurpg mentioned this pull request Nov 1, 2020
Base automatically changed from master to main January 20, 2021 08:13
valencik added a commit that referenced this pull request Apr 10, 2022
Originally in #225
Co-authored-by: Ólafur Páll Geirsson <olafurpg@gmail.com>
@valencik
Copy link
Collaborator

Closing this as it was revived and merged in #521

@valencik valencik closed this Apr 16, 2022
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.

Wrong order of arguments for assertEquals
3 participants