scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
230 stars 21 forks source link

Implicit instance resolves differently when resolved via method and via implicitly #12773

Closed alexklibisz closed 1 year ago

alexklibisz commented 1 year ago

Reproduction steps

Scala version: 2.12.13 and 2.13.10

trait Effect
object Effect {
  trait Read extends Effect
  trait Write extends Effect
}

sealed trait EffectType[E <: Effect] {
  def extendsRead: Boolean
}
object EffectType {
  sealed trait Read[E <: Effect] {
    val extended: Boolean
  }
  sealed trait ReadNotExtended {
    implicit def notExtended[E <: Effect]: Read[E] = new Read[E] {
      val extended = false
    }
  }
  object Read extends ReadNotExtended {
    implicit def extended[E <: Effect.Read]: Read[E] = new Read[E] {
      val extended = true
    }
  }
  implicit def fromEffect[E <: Effect](implicit r: Read[E]): EffectType[E] = 
    new EffectType[E] {
      def extendsRead = r.extended
    }
}

object Test extends App {

  def summonViaImplicitly[E <: Effect: EffectType](): EffectType[E] =
    implicitly[EffectType[E]]

  def summonViaMethod[E <: Effect: EffectType](): EffectType[E] = 
    EffectType.fromEffect[E]

  // All of these should be true.
  assert(implicitly[EffectType[Effect.Read]].extendsRead)   // passes
  assert(EffectType.fromEffect[Effect.Read].extendsRead)  // passes
  assert(summonViaImplicitly[Effect.Read]().extendsRead)  // passes
  assert(summonViaMethod[Effect.Read]().extendsRead)    // FAILS 
}

Problem

When I summon the instance of EffectType using implicitly, it summons the correct (more-specific) instance of Read and therefore produces the correct instance of EffectType. See the lines commented with // passes.

When I summon the instance of EffectType by calling the fromEffect[E] method, it summons the wrong (less-specific) instance of Read and therefore produces the incorrect instance of EffectType. See the lines commented with // FAILS.

alexklibisz commented 1 year ago

Making Read covariant in E (Read[+E <: Effect]) seems to resolve this case, but does not resolve a slightly more complicated case from which I extracted the initial snippet. I'll see if I can replicate the more complicated case in a representative snippet.

joroKr21 commented 1 year ago

implicitly is very different from fromEffect - it has a different type signature:

def implicitly[T](implicit e: T): T
def fromEffect[E <: Effect](implicit r: Read[E]): EffectType[E]

If you enable -Wunused you would get a warning:

evidence parameter evidence$2 of type EffectType[E] in method summonViaMethod is never used

So it's equivalent to this:

def summonViaMethod[E <: Effect](): EffectType[E] = 
    EffectType.fromEffect[E]

Does it make sense to you now?

alexklibisz commented 1 year ago

Hi @joroKr21. Thanks for the feedback. I don't quite follow, though. I'm not comparing implicitly to fromEffect. Both implicitly and fromEffect actually work correctly. See the first and second asserts. Instead, I'm comparing (implicitly, fromEffect, summonViaImplicitly) vs. (summonViaMethod). They should all behave the same way, but instead the first three behave correctly and last one incorrectly.

I'll try to restate the question: I'm calling summonViaMethod[Effect.Read]. Based on the method body, I expect this to result in a call to fromEffect[Effect.Read]. However, it seems to result in a call to fromEffect[Effect]. So, somewhere between the call to summonViaMethod and the method body, E has changed. Why is E changing this way? Is it a bug?

alexklibisz commented 1 year ago

If you enable -Wunused you would get a warning:

evidence parameter evidence$2 of type EffectType[E] in method summonViaMethod is never used

So it's equivalent to this:

def summonViaMethod[E <: Effect](): EffectType[E] = 
    EffectType.fromEffect[E]

I guess I'm understanding a little better after re-reading your response a few more times and responding above. I think you're saying: summonViaMethod takes an implicit EffectType[E]. However, fromEffect does not use the implicit EffectType[E]. So the implicit EffectType[E] remains unused. So fromEffect[E] degrades to the least specific subtype of E, which is Effect.

alexklibisz commented 1 year ago

Setting aside whether or not this is a bug, my main concern with this implementation is that a user of a library might call fromEffect in a way that's equivalent to summonViaMethod. The user thinks they are calling fromEffect[Effect.Read], but really they are calling fromEffect[Effect]. Do you see any way to restructure the code to prevent this?

som-snytt commented 1 year ago

I agree it's tricky. I see that it doesn't use the provided evidence, but I don't see yet why it doesn't prefer Read.extended. I'll have to spend a few minutes looking at it. I guess it's because of the narrow bound.

joroKr21 commented 1 year ago

Yes @alexklibisz I think you understood it on your own. Implicits work with statically known types, so it's wired at compile time, not at runtime.

implicit def extended[E <: Effect.Read]: Read[E] - inside fromEffect however we know only that E <: Effect and because we have implicit def notExtended[E <: Effect]: Read[E] it compiles.

Do you see any way to restructure the code to prevent this?

I'm not sure what's the end goal. Maybe you can change fromEffect to be more like implicitly:

implicit def fromEffect[E <: Effect](implicit e: EffectType[E]): EffectType[E] = e
alexklibisz commented 1 year ago

Right now, fromEffect is what actually makes implicitly work. It's the only way to implicitly resolve an instance of EffectType[E]. If I write fromEffect as @joroKr21 suggested, there is no place to get an instance of EffectType.

alexklibisz commented 1 year ago

Also, here's the "why" of what I'm doing here, in case it's useful: I'm working with a sql library (Slick) which uses a phantom type called Effect to represent the "effect type" of a given sql statement. I'm trying to find a typesafe way to introspect this phantom type, in order to automatically send it to the appropriate database (a read-write primary vs. a read-only replica).

alexklibisz commented 1 year ago

Re-reading this a while later, and with some tips from @bishabosha , I'm in agreement this is not a bug.

I think I can summarize my mistake as follows:

I had written:

def summonViaMethod[E <: Effect: EffectType](): EffectType[E] = EffectType.fromEffect[E]

called it using:

summonViaMethod[Effect.Read]().extendsRead

and expected the call to resolve to the instance of EffectType.Read implemented by:

implicit def extended[E <: Effect.Read]: Read[E] = new Read[E] {
  val extended = true
}

However, the summonViaMethod signature only specifies that E <: Effect. It has no way to know that E is a specific subtype of Effect. So it resolves to the broadest available instead of EffectType.Read[Effect], which is the one implemented by def notExtended.

One way to avoid this possible footgun is to provide explicit negative instances for every known subtype of Effect, rather than providing the single def notExtended[E <: Effect].

som-snytt commented 1 year ago

There is a "not planned" status for non-bugs, as I learned via Seth.