scala3 icon indicating copy to clipboard operation
scala3 copied to clipboard

Inconsistent summonInline behavior

Open yurikpanic opened this issue 3 years ago • 10 comments

We define an inline given type class instance, which may end up with calling scala.compiletime.error at some point. Things work as expected if the error is not called when:

  • we use it via using argument to an inline method (fff1 in the example below)
  • if we call summonInline for the instance from the object for a distinct case class
  • if we call summonInline from within an inline method (fff2 in the example below)

But if the compiletime.error is called the behavior is different in these cases

  • we use it via using argument to an inline method - the custom error message is returned at the compile time - OK
  • if we call summonInline for the instance from the object for a distinct case class - the custom error message is returned at the compile time - OK
  • if we call summonInline from within an inline method - no given instance of type Test[Test2] was found. error is returned. - does not look as correct (or at least expected) behavior to me.

Compiler version

Checked on 3.1.3 and 3.2.1-RC1-bin-SNAPSHOT-git-f157978

Minimized code

import scala.deriving.Mirror
import scala.compiletime.*

trait Test[A] {
  def test: String
}

object Test {

  inline def findA[T <: Tuple]: Unit =
    inline erasedValue[T] match {
      case _: EmptyTuple  => error("Field 'a' not found")
      case _: ("a" *: tl) => ()
      case _: (_ *: tl)   => findA[tl]
    }

  inline given [A <: Product](using mm: Mirror.ProductOf[A]): Test[A] = new {
    override def test: String = {
      findA[mm.MirroredElemLabels]
      "test"
    }
  }
}

final case class Test1(a: String, b: Int)
final case class Test2(q: String, w: Int)

object Main {
  inline def fff1[P <: Product](using ggg: Test[P]): String = {
    ggg.test
  }

  inline def fff2[P <: Product]: String = {
    summonInline[Test[P]].test
  }

  fff1[Test1]
  fff1[Test2]

  summonInline[Test[Test1]].test
  summonInline[Test[Test2]].test

  fff2[Test1]
  fff2[Test2]
}

Output

-- Error: xxx.scala:38:13 ------------------------------------------------------
38 |  fff1[Test2]
   |             ^
   |             Field 'a' not found
-- Error: xxx.scala:41:15 ------------------------------------------------------
41 |  summonInline[Test[Test2]].test
   |               ^
   |               Field 'a' not found
-- Error: xxx.scala:44:6 -------------------------------------------------------
44 |  fff2[Test2]
   |  ^^^^^^^^^^^
   |no given instance of type Test[Test2] was found.
   |I found:
   |
   |    Test.given_Test_A[Test2](
   |      Test2.$asInstanceOf[
   |
   |          (
   |            deriving.Mirror.Product{
   |              MirroredType = Test2; MirroredMonoType = Test2;
   |                MirroredElemTypes <: Tuple
   |            }
   |           &
   |            scala.deriving.Mirror.Product{
   |              MirroredMonoType = Test2; MirroredType = Test2;
   |                MirroredLabel = ("Test2" : String)
   |            }
   |          ){
   |            MirroredElemTypes = (String, Int);
   |              MirroredElemLabels = (("q" : String), ("w" : String))
   |          }
   |
   |      ]
   |    )
   |
   |But given instance given_Test_A in object Test does not match type Test[Test2].
   |----------------------------------------------------------------------------
   |Inline stack trace
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from xxx.scala:34
34 |    summonInline[Test[P]].test
   |    ^^^^^^^^^^^^^^^^^^^^^
    ----------------------------------------------------------------------------

Expectation

All three errors should be Field 'a' not found

yurikpanic avatar Jul 30 '22 11:07 yurikpanic

If you throw transparent inline in the works you may get a different response.

soronpo avatar Jul 30 '22 13:07 soronpo

If you throw transparent inline in the works you may get a different response.

Tanks, changing fff2 signature to transparent inline does solve my problem. The custom error message is returned in this case now as well.

But to me it still seems that this should also work for non transparent inline methods.

yurikpanic avatar Jul 30 '22 14:07 yurikpanic

So, summonInline[Test[Test1]] in object Main is inlined during the typer phase, that is why when Typer.adapt is called from Implicits.typedImplicit for the given_Test_A instance - it is not inlined.

But if we call summonInline[Test[P]] from an inline method (not transparent inline) - we are doing this during the inlining phase, Inlines.needsInlining give us true - the given_Test_A is inlined while trying to do Implicits.inferImplicitArg. If it completes without an error - it works, but if there was an error - Implicits.typedImplicit treats this as SearchFailure. So, the result of this summonInline - is the correct instance, but typed as MismatchedImplicit.

The custom error is still there in ctx.reporter at this stage. But an error message from the error type is rendered.

yurikpanic avatar Jul 31 '22 17:07 yurikpanic

It seems that transparent inline given is also not working as expected. If we add another inline given instance for Test with exactly the same signature, but that would not return error in any case:

object Test {
   ...
  inline given[B <: Product](using mm: Mirror.ProductOf[B]): Test[B] = new {
    override def test: String = "all"
  }
}

Then calls to fff1[Test1] and fff1[Test2] and top level summonInline[Test1] and summonInline[Test2] would give an "Ambiguous given instances" errors in both cases, no matter whether transparent is added to inline given instances or not.

But for fff2[Test1] it would give an "Ambiguous given instances" error - which is definitely correct, since the first given instance does not return error, since Test1 has field "a" and the second - also does not return an error and also fits.

For fff2[Test2] - it picks the second instance. So, it behaves as if the inline given instances were transparent inline given. The implicit search continues after trying inlining the first one ends up with an error. But again - this behaviour is the same, whether given instances are transparent or not. Which also seems to be not how this is described in the documentation.

yurikpanic avatar Aug 01 '22 08:08 yurikpanic

Then calls to fff1[Test1] and fff1[Test2] and top level summonInline[Test1] and summonInline[Test2] would give an "Ambiguous given instances" errors in both cases, no matter whether transparent is added to inline given instances or not.

Because you have ambiguity. That's the correct behavior. Why would you expect it to be something else?

soronpo avatar Aug 01 '22 12:08 soronpo

Then calls to fff1[Test1] and fff1[Test2] and top level summonInline[Test1] and summonInline[Test2] would give an "Ambiguous given instances" errors in both cases, no matter whether transparent is added to inline given instances or not.

Because you have ambiguity. That's the correct behavior. Why would you expect it to be something else?

Perhaps I did not explain it clear enough. According to the docs here - for a transparent inline given - the given search should continue if an error was reported during inlining. So, we not just add a second instance, which never report an error, but also change the first one from just inline given to transparent inline given. In this case - we'll have an ambiguity when we ask for Test[Test1] instance, as both candidates are eligible and no ambiguity when we ask for Test[Test2] instance, since the first one reports an error (because Test2 has no "a" field) and that is why it's not a valid candidate.

But the actual behavior is different. Adding transparent to inline given instances does not change a thing - first two cases (fff1 and plain summonInline) always act as if there is no transparent before inline given. While the last case (with fff2 method) always acts as if there is a transparent before inline given, no matter whether this transparent was specified actually or not. So, it looks like now inline given and transparent inline given makes no difference at all regarding the "continue search on error", haven't checked if transparent inline given gives us more precise type, than just inline given.

Perhaps this can be a separate issue. I mentioned it here, because it's also related to how errors are handled during inlining. So, the solution may be the same. And because we also see that the actual behavior of summonInline is also inconsistent in this aspect (it depends on whether summonInline was called from the top level or from an inlined function). And I believe neither of these behaviors match what's written in the docs.

yurikpanic avatar Aug 01 '22 13:08 yurikpanic

Maybe related to your inquiry https://github.com/lampepfl/dotty/issues/12429

soronpo avatar Aug 01 '22 14:08 soronpo

Maybe related to your inquiry #12429

Agree. Looks like this is the behavior that is described now in the documentation section I referenced.

yurikpanic avatar Aug 01 '22 14:08 yurikpanic

I'll summarise how I see the problem now.

  • the compiler considers all available given candidates when summonInline is encountered using usual precedence rules
  • candidates for inlining are type checked
  • if a candidate fails to type check - it is excluded from the list of available candidates (this is how it' implemented in the code now - we check if there were any error) - indeed we can not inline something that is not well typed.

This is how it's implemented now.

So, we can reformulate the part of documentation about transparent inline given in a more generic way. Like it was described in #12429 - if we fail to type check a candidate - it's removed from the list of potential given instances. This is true for any inline given instance, whether it's transparent or not. And this implies, that a transparent inline given with an explicit type ascription at the end should behave just like inline given, no matter if compiletime.error was called or not.

But do we really fail to type check an inline given (or transparent inline given) instance if it calls compiletime.error in the current context? I believe - no - the compile time code is well typed, just as some runtime code, that throws an exception (e.g. def a: Int = throw new RuntimeException("boo!") - is a well typed runtime code, despite throw here if of type Nothing). Because implicitly we assume, that any return type T is actually T|Nothing.

The same is true for compile time code. compiletime.error serves here the same role, as throw at runtime. So, if an inline given instance called compiletime.error - it does not mean, that type checking failed. The code is well typed, as runtime code that throw at some point. And that is why - my PR is not as "hacky" as I though. It makes error that is emitted by compiletime.error to be handled differently than other errors. So that this error does not mean "type check failed" any more. The intuition about compiletime.error being kind of a throw at compile time just translates to the code.

And it changes nothing from the user point of view.

  • If we have an inline given of some type T, that calls compiletime.error in some code branch - we have no way to know this, we still treat this candidate as valid, because all we know is that the return type is T. Just as for def a: Int = throw ... example for runtime - we'll see the error only once we call it. The same here - the given candidate may be picked and we'll see the error if it was actually inlined. (Since throw or compiletime.error may be called conditionally based on input arguments - we may or may not see the exception/error. Like in my original example - summonInline[Test[Test1]] - works, summonInline[Test[Test2]] - does not compile).
  • For a transparent inline given with explicit type ascription - the behavior is the same. In this case we also can't know that compiletime.error was called, as in non-transparent case.
  • If we have a transparent inline given (without explicit type ascription at the end) which calls compiletime.error at some point - we can actually see the fact that the return value is of type Nothing (maybe this would be just for some cases, based on arguments). In this case - we do the same, as it's described in the documentation. Since Nothing is uninhabited - we would never get an instance of this type (a Tree to inline) - we just remove this from the list of potential candidates. For example, this is the case where we have to candidates for given instance of some T. If one gives us Nothing, we see it because it's transparent and another one gives us T - there is not ambiguity - we just pick the second one. If both give us T (none of them called compiletime.error) - we should say, that there is an ambiguous given, both first and second give us T, they are of same priority - do not know what to pick.

Looks like pretty consistent behaviour to me.

yurikpanic avatar Aug 02 '22 09:08 yurikpanic

@nicolasstucki your thoughts?

soronpo avatar Aug 02 '22 13:08 soronpo