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

[Scala 2] Semiauto derivation should not auto-derive instances for case class fields #620

Open
mrdziuban opened this issue Nov 17, 2023 · 8 comments

Comments

@mrdziuban
Copy link

mrdziuban commented Nov 17, 2023

I just discovered and fixed a bug in my codebase because this compiles:

import cats.Monoid
import cats.data.NonEmptyList

case class  Foo(ids: NonEmptyList[Int])
object Foo {
  implicit val monoid: Monoid[Foo] = cats.derived.semiauto.monoid[Foo]
}

I'm guessing that this is caused by auto-derivation of case class fields for which there is not already an instance in scope?

I meant to derive a Semigroup, but I was shocked that this compiles as there isn't an instance of Monoid[NonEmptyList[Int]] in scope. The result is a Monoid the performs addition on the head of the NonEmptyList, and concatenation of the tails.

Monoid[Foo].empty // Foo(NonEmptyList(0))
Monoid[Foo].combine(Foo(NonEmptyList.of(1, 3)), Foo(NonEmptyList.of(1, 4))) // Foo(NonEmptyList(2, 3, 4))

I've made this same argument for circe -- in my opinion "semiauto" should mean that compilation should fail if any field does not already have an instance of the relevant typeclass.

Would you consider changing this behavior? It happens in both scala 2 and 3, here's a scastie with the same code as above: https://scastie.scala-lang.org/mrdziuban/IIhfamrNTsqXAza0mc2oRQ/4

@joroKr21
Copy link
Member

joroKr21 commented Nov 18, 2023

That's rather a deliberate design choice. Changing it at this point is not an option as it would be really disruptive. What we can consider is adding one more flavour of derivation with these semantics. However that can be as complex as reimplementing everything.

Also keep in mind that for sealed traits (and enums in Scala 3) usually you do want to to derive recursively for all cases. Otherwise it becomes very tedious to do this for every single case manually.

@mrdziuban
Copy link
Author

A separate type of derivation would work for me, but I really do think the existing behavior is counterintuitive and is likely to bite other users.

Regarding sealed traits, my ideal is that semiauto derivation would handle case objects automatically, without needing to derive for them manually, but that it would not automatically handle case classes. This is how semiauto derivation works in circe for scala 2 and I've always found it to be a reasonable approach that doesn't add a lot of boilerplate.

@joroKr21
Copy link
Member

A separate type of derivation would work for me, but I really do think the existing behavior is counterintuitive and is likely to bite other users.

I think a change of behaviour would be more confusing than a new feature.

BTW, can you come up with a good name for it? 😄 I think that might be the hardest part actually.
I'm thinking about maybe adding an implicit parameter to configure this behaviour which is enabled by default in the current semiauto. But we would need a new import and I'm struggling to come up with a name for it.

@mrdziuban
Copy link
Author

I think a change of behaviour would be more confusing than a new feature.

I get where you're coming from, but it could be a new major release with the change called out. Plus anyone who blindly upgrades would only see new compile errors, rather than transparent changes in runtime behavior, so they would learn about the change and be able to decide what to do. That said, I'm still open to a whole new approach to derivation if that's what you think is best.

I'm thinking about maybe adding an implicit parameter to configure this behaviour which is enabled by default in the current semiauto.

Do you mean that enabling auto-derivation for nested types would require this new implicit/import? Or that disabling it would require the implicit?

Either way, my opinion is that a completely separate entrypoint for each behavior would be a better approach. People (including me 🙂) are bound to forget the import and unintentionally opt in or out of the behavior. Plus it'll be a lot easier to see and understand which style of derivation is being used at each call site -- you just look at which method is being called, rather than having to scroll to the top of the file to inspect the imports.

As far as a name goes, I don't have a great idea either...maybe something explicit like semiauto.derivedNoAutoRecursion (which is what I did in my PR to circe for the similar issue) or something less verbose but more opaque like semiauto.derivedOne or semisemiauto.derived?

@joroKr21
Copy link
Member

I get where you're coming from, but it could be a new major release with the change called out. Plus anyone who blindly upgrades would only see new compile errors, rather than transparent changes in runtime behavior, so they would learn about the change and be able to decide what to do.

That is if we assume that there is one correct behaviour. Which I disagree with, both approaches are valid and ideally we should offer both.

Do you mean that enabling auto-derivation for nested types would require this new implicit/import? Or that disabling it would require the implicit?

Not necessarily a new import. The mechanism to implement it might be different from the API. At least in Scala 3, we can add an inline import without a problem. For Scala 2, I need to think about it more.

Either way, my opinion is that a completely separate entrypoint for each behavior would be a better approach.

Yes, I meant a different entry point but it still needs a name.

Here is how semiauto looks in Scala 3. We need a different import name, not a different method name:

object semiauto:
  inline def eq[A]: Eq[A] = DerivedEq[A]
  inline def hash[A]: Hash[A] = DerivedHash[A]
  inline def empty[A]: Empty[A] = DerivedEmpty[A]
  inline def semigroup[A]: Semigroup[A] = DerivedSemigroup[A]
  inline def monoid[A]: Monoid[A] = DerivedMonoid[A]
  inline def order[A]: Order[A] = DerivedOrder[A]
  inline def commutativeSemigroup[A]: CommutativeSemigroup[A] = DerivedCommutativeSemigroup[A]
  inline def commutativeMonoid[A]: CommutativeMonoid[A] = DerivedCommutativeMonoid[A]
  inline def applicative[F[_]]: Applicative[F] = DerivedApplicative[F]
  inline def apply[F[_]]: Apply[F] = DerivedApply[F]
  inline def emptyK[F[_]]: EmptyK[F] = DerivedEmptyK[F]
  inline def pure[F[_]]: Pure[F] = DerivedPure[F]
  inline def foldable[F[_]]: Foldable[F] = DerivedFoldable[F]
  inline def functor[F[_]]: Functor[F] = DerivedFunctor[F]
  inline def reducible[F[_]]: Reducible[F] = DerivedReducible[F]
  inline def traverse[F[_]]: Traverse[F] = DerivedTraverse[F]
  inline def nonEmptyTraverse[F[_]]: NonEmptyTraverse[F] = DerivedNonEmptyTraverse[F]
  inline def show[A]: Show[A] = DerivedShow[A]
  inline def semigroupK[F[_]]: SemigroupK[F] = DerivedSemigroupK[F]
  inline def monoidK[F[_]]: MonoidK[F] = DerivedMonoidK[F]
  inline def contravariant[F[_]]: Contravariant[F] = DerivedContravariant[F]
  inline def invariant[F[_]]: Invariant[F] = DerivedInvariant[F]
  inline def partialOrder[A]: PartialOrder[A] = DerivedPartialOrder[A]
  inline def showPretty[A]: ShowPretty[A] = DerivedShowPretty[A]

@joroKr21
Copy link
Member

joroKr21 commented Nov 19, 2023

I don't know what I would call it - perhaps strict?
Then we have the derives enrichment which can go in cats.derived.strict and the explicit methods can go in cats.derived.strict.semiauto or cats.derived.semiauto.strict.

@mrdziuban
Copy link
Author

Thanks for explaining, I understand now. strict.semiauto or semiauto.strict sounds reasonable to me, let me know if there's any way I can help!

@joroKr21
Copy link
Member

@mrdziuban I drafted an example for Eq: #626 - Let me know what do you think?

@joroKr21 joroKr21 changed the title Semiauto derivation should not auto-derive instances for case class fields [Scala 2] Semiauto derivation should not auto-derive instances for case class fields Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants