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

Select local implicits over name-imported over wildcard imported #18203

Merged
merged 4 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/Implicits.scala
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ object Implicits:
class ContextualImplicits(
val refs: List[ImplicitRef],
val outerImplicits: ContextualImplicits | Null,
isImport: Boolean)(initctx: Context) extends ImplicitRefs(initctx) {
val isImport: Boolean)(initctx: Context) extends ImplicitRefs(initctx) {
private val eligibleCache = EqHashMap[Type, List[Candidate]]()

/** The level increases if current context has a different owner or scope than
Expand Down Expand Up @@ -330,8 +330,21 @@ object Implicits:
if ownEligible.isEmpty then outerEligible
else if outerEligible.isEmpty then ownEligible
else
val shadowed = ownEligible.map(_.ref.implicitName).toSet
ownEligible ::: outerEligible.filterConserve(cand => !shadowed.contains(cand.ref.implicitName))
def filter(xs: List[Candidate], remove: List[Candidate]) =
val shadowed = remove.map(_.ref.implicitName).toSet
xs.filterConserve(cand => !shadowed.contains(cand.ref.implicitName))

val outer = outerImplicits.uncheckedNN
def isWildcardImport(using Context) = ctx.importInfo.nn.isWildcardImport
def preferDefinitions = isImport && !outer.isImport
def preferNamedImport = isWildcardImport && !isWildcardImport(using outer.irefCtx)

if !migrateTo3(using irefCtx) && level == outer.level && (preferDefinitions || preferNamedImport) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the !migrateTo3 condition? I thought Scala 2 has the same preferences of definitions over imports?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To counter the fact that we're flattening it in "level". Otherwise we end up preferring the candidates in an outer scope, which should have a smaller level, over the ones in an inner scope.

I encountered it in CI, and minimised the problem to tests/pos/i18183.migration.scala. Because it runs under -source:3.0-migration then outer implicit F: Async[F] "shadows" inner implicit F: Async[M], making it impossible to call semiflatMap for lack of a Async[M] in scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Yes that makes sense.

// special cases: definitions beat imports, and named imports beat
// wildcard imports, provided both are in contexts with same scope
filter(ownEligible, outerEligible) ::: outerEligible
else
ownEligible ::: filter(outerEligible, ownEligible)

def uncachedEligible(tp: Type)(using Context): List[Candidate] =
Stats.record("uncached eligible")
Expand Down
38 changes: 38 additions & 0 deletions tests/pos/i18183.migration.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// scalac: -source:3.0-migration

// A not-fully-minimal reproduction of the CI failure in http4s
// While implementing the fix to name "shadowing" in implicit lookup.

import scala.util.control.NoStackTrace

final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
def semiflatMap[D](f: B => F[D])(implicit F: Monad[F]): EitherT[F, A, D] = ???
}

trait Applicative[F[_]] {
def pure[A](x: A): F[A]
}
trait Monad[F[_]] extends Applicative[F]
trait Async[F[_]] extends Monad[F]

final class Request[+F[_]]

final case class RequestCookie(name: String, content: String)

final class CSRF2[F[_], G[_]](implicit F: Async[F]) { self =>
import CSRF2._

def signToken[M[_]](rawToken: String)(implicit F: Async[M]): M[CSRFToken] = ???

def refreshedToken[M[_]](implicit F: Async[M]): EitherT[M, CSRFCheckFailed, CSRFToken] =
EitherT(extractRaw("")).semiflatMap(signToken[M])

def extractRaw[M[_]: Async](rawToken: String): M[Either[CSRFCheckFailed, String]] = ???
}

object CSRF2 {
type CSRFToken

case object CSRFCheckFailed extends Exception("CSRF Check failed") with NoStackTrace
type CSRFCheckFailed = CSRFCheckFailed.type
}
17 changes: 17 additions & 0 deletions tests/run/i18183.findRef.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// A minimised reproduction of how an initial change to combineEligibles broke Typer#findRef
case class Foo(n: Int)

class Test:
import this.toString

val foo1 = Foo(1)
val foo2 = Foo(2)

def foo(using Foo): Foo =
import this.*
def bar(using Foo): Foo = summon[Foo]
bar(using foo2)

object Test extends Test:
def main(args: Array[String]): Unit =
assert(foo(using foo1) eq foo2)
93 changes: 93 additions & 0 deletions tests/run/i18183.given.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
case class Foo(n: Int)

class Bar(n: Int):
given foo: Foo = new Foo(n)

class InMethod:
def wild(bar: Bar): Unit =
import bar.*
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def givenWild(bar: Bar): Unit =
import bar.{ given, * }
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def givn(bar: Bar): Unit =
import bar.given
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def givenFoo(bar: Bar): Unit =
import bar.given Foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def named(bar: Bar): Unit =
import bar.foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def namedGivenWild(bar: Bar, bar2: Bar): Unit =
import bar.foo, bar2.{ given, * }
assert(bar.foo eq summon[Foo])

def givenWildNamed(bar: Bar, bar2: Bar): Unit =
import bar2.{ given, * }, bar.foo
assert(bar.foo eq summon[Foo])

def namedWild(bar: Bar, bar2: Bar): Unit =
import bar.foo, bar2.*
assert(bar.foo eq summon[Foo])

def wildNamed(bar: Bar, bar2: Bar): Unit =
import bar2.*, bar.foo
assert(bar.foo eq summon[Foo])

class InClassWild(bar: Bar):
import bar.*
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class InClassGivenWild(bar: Bar):
import bar.{ given, * }
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class InClassGiven(bar: Bar):
import bar.given
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class InClassGivenFoo(bar: Bar):
import bar.given Foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class InClassNamed(bar: Bar):
import bar.foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

object Test:
def main(args: Array[String]): Unit =
val bar = new Bar(1)
val bar2 = new Bar(2)

new InMethod().wild(bar)
new InMethod().givenWild(bar) // was: error
new InMethod().givn(bar) // was: error
new InMethod().givenFoo(bar) // was: error
new InMethod().named(bar) // was: error

new InMethod().namedWild(bar, bar2)
new InMethod().wildNamed(bar, bar2)
new InMethod().namedGivenWild(bar, bar2) // was: error
new InMethod().givenWildNamed(bar, bar2)

new InClassWild(bar)
new InClassGivenWild(bar) // was: error
new InClassGiven(bar) // was: error
new InClassGivenFoo(bar) // was: error
new InClassNamed(bar) // was: error
141 changes: 141 additions & 0 deletions tests/run/i18183.mixed.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
case class Foo(n: Int)

class OldBar(n: Int):
implicit val foo: Foo = new Foo(n)

class NewBar(n: Int):
given foo: Foo = new Foo(n)

class OldInMethod:
def wild(bar: OldBar): Unit =
import bar.*
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def named(bar: OldBar): Unit =
import bar.foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def namedWild(bar: OldBar, bar2: NewBar): Unit =
import bar.foo, bar2.*
assert(bar.foo eq summon[Foo])

def wildNamed(bar: OldBar, bar2: NewBar): Unit =
import bar2.*, bar.foo
assert(bar.foo eq summon[Foo])

def namedGivenWild(bar: OldBar, bar2: NewBar): Unit =
import bar.foo
import bar2.{ given, * }
assert(bar.foo eq summon[Foo])

def givenWildNamed(bar: OldBar, bar2: NewBar): Unit =
import bar2.{ given, * }, bar.foo
assert(bar.foo eq summon[Foo])

class OldInClassWild(bar: OldBar):
import bar.*
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class OldInClassNamed(bar: OldBar):
import bar.foo
given foo: Foo = new Foo(2)
assert(foo eq summon[Foo])


class NewInMethod:
def givenWild(bar: NewBar): Unit =
import bar.{ given, * }
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def wild(bar: NewBar): Unit =
import bar.*
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def givn(bar: NewBar): Unit =
import bar.given
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def givenFoo(bar: NewBar): Unit =
import bar.given Foo
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def named(bar: NewBar): Unit =
import bar.foo
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

def namedWild(bar: NewBar, bar2: OldBar): Unit =
import bar.foo, bar2.*
assert(bar.foo eq summon[Foo])

def wildNamed(bar: NewBar, bar2: OldBar): Unit =
import bar2.*, bar.foo
assert(bar.foo eq summon[Foo])

class NewInClassGivenWild(bar: NewBar):
import bar.{ given, * }
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class NewInClassWild(bar: NewBar):
import bar.*
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class NewInClassGiven(bar: NewBar):
import bar.given
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class NewInClassGivenFoo(bar: NewBar):
import bar.given Foo
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])

class NewInClassNamed(bar: NewBar):
import bar.foo
implicit val foo: Foo = new Foo(2)
assert(foo eq summon[Foo])


object Test:
def main(args: Array[String]): Unit =
val oldBar = new OldBar(1)
val newBar = new NewBar(1)
val oldBar2 = new OldBar(2)
val newBar2 = new NewBar(2)


new OldInMethod().wild(oldBar) // was: error
new OldInMethod().named(oldBar) // was: error

new OldInMethod().namedWild(oldBar, newBar2)
new OldInMethod().wildNamed(oldBar, newBar2)
new OldInMethod().namedGivenWild(oldBar, newBar2) // was: error
new OldInMethod().givenWildNamed(oldBar, newBar2)

new OldInClassWild(oldBar) // was: error
new OldInClassNamed(oldBar) // was: error


new NewInMethod().wild(newBar)
new NewInMethod().givenWild(newBar) // was: error
new NewInMethod().givn(newBar) // was: error
new NewInMethod().givenFoo(newBar) // was: error
new NewInMethod().named(newBar) // was: error

new NewInMethod().namedWild(newBar, oldBar2) // was: error
new NewInMethod().wildNamed(newBar, oldBar2)

new NewInClassWild(newBar)
new NewInClassGivenWild(newBar) // was: error
new NewInClassGiven(newBar) // was: error
new NewInClassGivenFoo(newBar) // was: error
new NewInClassNamed(newBar) // was: error
49 changes: 49 additions & 0 deletions tests/run/i18183.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
case class Foo(n: Int)

class Bar(n: Int):
implicit val foo: Foo = new Foo(n)

class InMethod:
def wild(bar: Bar): Unit =
import bar._
implicit val foo: Foo = new Foo(2)
assert(foo eq implicitly[Foo])

def named(bar: Bar): Unit =
import bar.foo
implicit val foo: Foo = new Foo(2)
assert(foo eq implicitly[Foo])

def namedWild(bar: Bar, bar2: Bar): Unit =
import bar.foo
import bar2._
assert(bar.foo eq implicitly[Foo])

def wildNamed(bar: Bar, bar2: Bar): Unit =
import bar2._
import bar.foo
assert(bar.foo eq implicitly[Foo])

class InClassWild(bar: Bar):
import bar._
implicit val foo: Foo = new Foo(2)
assert(foo eq implicitly[Foo])

class InClassNamed(bar: Bar):
import bar.foo
implicit val foo: Foo = new Foo(2)
assert(foo eq implicitly[Foo])

object Test:
def main(args: Array[String]): Unit =
val bar = new Bar(1)
val bar2 = new Bar(2)

new InMethod().wild(bar) // was: error
new InMethod().named(bar) // was: error

new InMethod().namedWild(bar, bar2) // was: error
new InMethod().wildNamed(bar, bar2)

new InClassWild(bar) // was: error
new InClassNamed(bar) // was: error