[Scala 3] MacroConst and MacroReaderT Errors readability improvement

[pomadchin]

I still took a closer look into the comment left here https://github.com/typelevel/cats-tagless/pull/560#discussion_r1675658431

It is true that macro generated code fails to compile & errors produced are OK but a bit cryptic:

[error] 31 |    val safe = Derive.const[NotSimpleAlg, Int](42)
[error]    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |Malformed tree was found while expanding macro with -Xcheck-macros.
[error]    |               |The tree does not conform to the compiler's tree invariants.
[error]    |               |
[error]    |               |Macro was:
[error]    |               |scala.quoted.runtime.Expr.splice[ConstTests.this.NotSimpleAlg[cats.tagless.Const[scala.Int].<none>]](((evidence$1: scala.quoted.Quotes) ?=> cats.tagless.macros.MacroConst.inline$deriveConst[[F >: scala.Nothing <: [_$1 >: scala.Nothing <: scala.Any] => scala.Any] => ConstTests.this.NotSimpleAlg[F], scala.Int](scala.quoted.runtime.Expr.quote[scala.Int](42).apply(using evidence$1))(scala.quoted.Type.of[[F >: scala.Nothing <: [_$1 >: scala.Nothing <: scala.Any] => scala.Any] => ConstTests.this.NotSimpleAlg[F]](evidence$1), scala.quoted.Type.of[scala.Int](evidence$1), evidence$1)))
[error]    |               |
[error]    |               |The macro returned:
[error]    |               |{
[error]    |  class $anon$macro$1 extends java.lang.Object with ConstTests.this.NotSimpleAlg[[T >: scala.Nothing <: scala.Any] => scala.Int] {
[error]    |    override def str(str: scala.Predef.String): scala.Predef.String = 42
[error]    |    override def parseInt(`str₂`: scala.Predef.String): scala.Int = 42
[error]    |    override def divide(dividend: scala.Float, divisor: scala.Float): scala.Int = 42
[error]    |  }
[error]    |  new $anon$macro$1()
[error]    |}
[error]    |               |
[error]    |               |Error:
[error]    |               |assertion failed: Found:    (42 : Int)
[error]    |Required: String
[error]    |found: ??
[error]    |expected: type String in object Predef with type String, flags = <touched>, underlying = String,  = String, String, Object with Serializable with Comparable[String] with CharSequence {...}
[error]    |tree = 42
[error]    |               |
[error]    |stacktrace available when compiling with `-Ydebug`
[error]    |               |
[error]    |----------------------------------------------------------------------------
[error]    |Inline stack trace

This is an attempt to align it a bit with the Scala 2 Macro in terms of error handling:

[error] 44 |    Derive.const[NotSimpleAlg, Int](42)
[error]    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |   Expected method str to return scala.Int but found scala.Predef.String
[error]    |----------------------------------------------------------------------------
[error]    |Inline stack trace
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    |This location contains code that was inlined from ConstTests.scala:44
[error] 45 |  inline def const[Alg[_[_]], A](value: A): Alg[Const[A]#λ] = MacroConst.derive[Alg, A](value)
[error]    |                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]     ----------------------------------------------------------------------------
[error] one error found

[alonsodomin]

same thing could also be added to the MacroReaderT. At the moment the implementation fallsback into returning method.rhs or value.rhs if the expression type doesn't match the expected one to be transformed.

But since MacroReaderT creates a fresh implementation from scratch without possibly delegating execution (as MacroConst does), the rhs value in both cases will be None and therefore ending up in a cryptic error message.

[pomadchin]

@alonsodomin ha, you're right! I can add it there as well 🤔

[pomadchin]

The Scala2 version also catches it to print over there https://github.com/typelevel/cats-tagless/blob/master/macros/src/main/scala-2/cats/tagless/DeriveMacros.scala#L746

[alonsodomin]

yeah, it was one oversight from my part... it could be added into this PR as both cases address a similar issue

[pomadchin]

@alonsodomin actually in case of ReaderT macro it gonna print "Concrete method has no definition: override def $methodName: String", I think it makes sense and not too bad. I can modify it to be more specific though, i.e. 'why' the macros function failed.

[pomadchin]

Added ReaderT error handling here => 4d66cd6

[alonsodomin]

isn't it though the same case as with Const?

I mean, the macro is building an instance of Alg[F] where F is [X] =>> ReaderT[F, Alg[F], X], so the case in which the return type of any method or value in Alg not be ReaderT[F, Alg[F], t] should be because in trait Alg[F[_]] the given method or value was not defined to return F[t].

In other words, the case in which this match would fail:

value.tpt.tpe.asType match
        case '[ReaderT[F, Alg[F], t]] =>
          Some(readerT[t](value.symbol)(_.select(value.symbol)))

would be because the type of value is not F[t], the usage of case '[ReaderT[F, Alg[F], t]] is more to make it handy to extract the type t so it can be used to create the instance of ReaderT ...

Or am I reading it incorrectly?

[pomadchin]

So for the Const case macro was building incorrect trees (that caused these cryptic errors), smth like:

def str: String = 42

The reason is that we were unconditionally appending the same function body to any function signature.

In the ReaderT code it's been left as unimplemented, that's why the error was a bit better.

But regardless it feels like a good idea to handle it.

[alonsodomin]

I think it's easier if I put a full example:

Let's say that the algebra is:

trait MyAlg[F[_]] {
  def effectfulMethod(param: Int): F[Long]
  def pureMethod(param: Int): Long
}

So the an expected ReaderT-based implementation with Try would be something like:

class TryMyAlg extends MyAlg[ReaderT[Try, MyAlg[Try], ?] {
  def effectfulMethod(param: Int): ReaderT[Try, MyAlg[Try], Long] = ???
  def pureMethod(param: Int): Long  // <-- this we can't implement
}

And we can't provide with an implementation because the method was defined to be pure, so I believe it's better to say to the end user that we expected the method to return F[?] instead of cats.data.Kleisi as from the user PoV, no Kleisli is involved in the definition of MyAlg.

A similar case would be Const.

[joroKr21]

I see what you mean. The difference between Scala 2 and Scala 3 is that in Scala 2 we work with the original type of the algebra whereas in Scala 3 we work with the target type. I think it would be fine to adjust the error message.