softwaremill / magnolia

Easy, fast, transparent generic derivation of typeclass instances
https://softwaremill.com/open-source/
Apache License 2.0
768 stars 117 forks source link

[scala2] CaseClass[Typeclass, T] derivation for case class T with a private constructor succeeds inside companion object, but fails outside of it #502

Closed jacektomaszek closed 11 months ago

jacektomaszek commented 11 months ago

We have a case class with a private constructor: case class LocalTimeInterval private (startInclusive: LocalTime, endExclusive: LocalTime) and with a smart constructor that parses two LocalTimes to confirm that endExclusive is after the startInclusive.

If combine is defined as def combine[T](caseClass: CaseClass[Typeclass, T]) when deriving outside of companion object there is such error, as expected: constructor LocalTimeInterval in class LocalTimeInterval cannot be accessed in <$anon: magnolia.CaseClass[...]> from <$anon: magnolia.CaseClass[...]>.

However, when deriving in the companion object the compilation succeeds. This is not dangerous if the derived type class only reads the data, but for example for a Decoder it is dangerous because it sort of hides the usage of the constructor and allows to bypass the smart constructor which results in illegal states such as LocalTimeInterval("12:00", "10:00").

Is it as expected, and if so is it possible to somehow prevent that behaviour? Expected behaviour for me would be that it also fails to compile when used in the companion object. Explicitly defining a Decoder in the companion object of course also could use the constructor or the apply and introduce illegal states, but it's more visible than the derivation of such behaviour.

I would imagine that one way to fix this (if it is to be fixed) would be to check if theapply method is private (don't know if reliable enough to determine that a case class has a private constructor) around this place: https://github.com/softwaremill/magnolia/blob/3fc3ae195792d618b6fa8569f521ecb04e3a3854/core/src/main/scala/magnolia1/magnolia.scala#L280

joroKr21 commented 11 months ago

You can use a sealed abstract case class: https://antousias.com/a-better-way-to-constraint-case-class-construction-in-scala/

jacektomaszek commented 11 months ago

Thank you, this indeed makes CaseClass[Typeclass, T] inaccessible. However, I'm using -Xsource:3, so I would like to not use the old, verbose tricks with sealed abstract case class if it's already supported in the current form. Is the meaning in context of magnolia different for sealed abstract case class and case class T private(...)? In case of the language I think it's advised to now use the second form instead of the sealed trick, so perhaps it could be also reflected in the magnolia context?

joroKr21 commented 11 months ago

Magnolia is a macro generating Scala code. So the normal accessibility rules apply. The constructor is accessible inside the companion object and not accessible outside of it. Making it abstract prevents prevents the class from being instantiated. Personally, I would be against doing something different from normal Scala accessibility rules.

adamw commented 11 months ago

As @joroKr21, this seems to follow Scala's visibility rules? A macro, after all, "just" generates code, which is then verified by the type-checker. So I don't think we'd even be allowed to break the visibility rules in the first place

joroKr21 commented 11 months ago

We can't break the rules, but we could throw an error in certain cases. However I would advise against it. Because I think it could get confusing.

adamw commented 11 months ago

@joroKr21 Yes exactly, that's my view as well :)

jacektomaszek commented 11 months ago

Thanks, the arguments seem reasonable.

If anyone comes across this issue with a similar problem I've solved it somehow by using ArchUnit test that fails if it finds a case class with private apply method && it's companion object contains a field with name containing "decoder" and "macro".