Make implicit search ordering transitive

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -1350,50 +1350,46 @@ trait Applications extends Compatibility {
     *  Module classes also inherit the relationship from their companions. This means,
     *  if no direct derivation exists between `sym1` and `sym2` also perform the following
     *  tests:
-    *   - If both sym1 and sym1 are module classes that have companion classes, compare the companions
-    *   - If sym1 is a module class with a companion, and sym2 is a normal class or trait, compare
-    *     the companion with sym2.
+    *   - If both sym1 and sym1 are module classes that have companion classes,
+    *     and sym2 does not inherit implicit members from a base class (#),
+    *     compare the companion classes.
+    *   - If sym1 is a module class with a companion, and sym2 is a normal class or trait,
+    *     compare the companion with sym2.
     *
-    *  Note that this makes `compareOwner(_, _) > 0` not transitive! For instance:
+    *  Condition (#) is necessary to make `compareOwner(_, _) > 0` a transitive relation.
+    *  For instance:
     *
     *    class A extends B
-    *    object A
+    *    object A { given a ... }
     *    class B
-    *    object B extends C
+    *    object B extends C { given b ... }
+    *    class C { given c }
     *
-    *  Then compareOwner(A$, B$) = 1 and compareOwner(B$, C) == 1, but
-    *  compareOwner(A$, C) == 0.
+    *  Then without (#), and taking A$ for the module class of A,
+    *  compareOwner(A$, B$) = 1 and compareOwner(B$, C) == 1,
+    *  but compareOwner(A$, C) == 0.
+    *  Violating transitivity in this way is bad, since it makes implicit search
+    *  outcomes compilation order dependent. E.g. if we compare `b` with `c`
+    *  first, we pick `b`. Then, if we compare `a` and `b`, we pick `a` as
+    *  solution of the search. But if we start with comparing `a` with `c`,
+    *  we get an ambiguity.
+    *
+    *  With the added condition (#), compareOwner(A$, B$) == 0.
+    *  This means we get an ambiguity between `a` and `b` in all cases.
     */
   def compareOwner(sym1: Symbol, sym2: Symbol)(using Context): Int =
     if sym1 == sym2 then 0
     else if sym1.isSubClass(sym2) then 1
     else if sym2.isSubClass(sym1) then -1
     else if sym1.is(Module) then
-      compareOwner(sym1.companionClass, if sym2.is(Module) then sym2.companionClass else sym2)
+      val cls1 = sym1.companionClass
+      if sym2.is(Module) then
+        if sym2.thisType.implicitMembers.forall(_.symbol.owner == sym2) then // test for (#)
+          compareOwner(cls1, sym2.companionClass)
+        else 0
+      else compareOwner(cls1, sym2)
     else 0
 
-  /** A version of `compareOwner` that is transitive, to be used in sorting
-   *  It would be nice if we could use this as the general method for comparing
-   *  owners, but unfortunately this does not compile all existsing code.
-   *  An example is `enrich-gentraversable.scala`. Here we have
-   *
-   *    trait BuildFrom...
-   *    object BuildFrom extends BuildFromLowPriority1
-   *
-   *  and we need to pick an implicit in BuildFrom over BuildFromLowPriority1
-   *  the rules in `compareOwner` give us that, but the rules in `isSubOwner`
-   *  don't.
-   *  So we need to stick with `compareOwner` for backwards compatibility, even
-   *  though it is arguably broken. We can still use `isSubOwner` for sorting
-   *  since that is just a performance optimization, so if the two methods
-   *  don't agree sometimes that's OK.
-   */
-  def isSubOwner(sym1: Symbol, sym2: Symbol)(using Context): Boolean =
-    if sym1.is(Module) && sym1.companionClass.exists then
-      isSubOwner(sym1.companionClass, if sym2.is(Module) then sym2.companionClass else sym2)
-    else
-      sym1 != sym2 && sym1.isSubClass(sym2)
-
   /** Compare to alternatives of an overloaded call or an implicit search.
    *
    *  @param  alt1, alt2      Non-overloaded references indicating the two choices

compiler/src/dotty/tools/dotc/typer/Implicits.scala

@@ -1173,7 +1173,7 @@ trait Implicits { self: Typer =>
         val arity2 = sym2.info.firstParamTypes.length
         if (arity1 < arity2) return true
         if (arity1 > arity2) return false
-        isSubOwner(sym1, sym2)
+        compareOwner(sym1, sym2) == 1
       }
 
       /** Sort list of implicit references according to `prefer`.

docs/docs/reference/changed-features/implicit-resolution.md

@@ -7,116 +7,129 @@ Implicit resolution uses a new algorithm which caches implicit results
 more aggressively for performance. There are also some changes that
 affect implicits on the language level.
 
- 1. Types of implicit values and result types of implicit methods
-    must be explicitly declared. Excepted are only values in local blocks
-    where the type may still be inferred:
-    ```scala
-    class C {
-
-      val ctx: Context = ...        // ok
-
-      /*!*/ implicit val x = ...    // error: type must be given explicitly
-
-      /*!*/ implicit def y = ...    // error: type must be given explicitly
-
-      val y = {
-        implicit val ctx = this.ctx // ok
-        ...
-      }
-    ```
- 2. Nesting is now taken into account for selecting an implicit.
-    Consider for instance the following scenario:
-    ```scala
-    def f(implicit i: C) = {
-      def g(implicit j: C) = {
-        implicitly[C]
-      }
+**1.** Types of implicit values and result types of implicit methods
+must be explicitly declared. Excepted are only values in local blocks
+where the type may still be inferred:
+```scala
+   class C {
+
+   val ctx: Context = ...        // ok
+
+   /*!*/ implicit val x = ...    // error: type must be given explicitly
+
+   /*!*/ implicit def y = ...    // error: type must be given explicitly
+
+   val y = {
+      implicit val ctx = this.ctx // ok
+      ...
+   }
+```
+**2.** Nesting is now taken into account for selecting an implicit.Consider for instance the following scenario:
+```scala
+  def f(implicit i: C) = {
+    def g(implicit j: C) = {
+      implicitly[C]
     }
-    ```
-    This will now resolve the `implicitly` call to `j`, because `j` is nested
-    more deeply than `i`. Previously, this would have resulted in an
-    ambiguity error. The previous possibility of an implicit search failure
-    due to _shadowing_ (where an implicit is hidden by a nested definition)
-    no longer applies.
-
- 3. Package prefixes no longer contribute to the implicit search scope of a type.
-    Example:
-    ```scala
-    package p
-    given a as A
-
-    object o {
-      given b as B
-      type C
-    }
-    ```
-    Both `a` and `b` are visible as implicits at the point of the definition
-    of `type C`. However, a reference to `p.o.C` outside of package `p` will
-    have only `b` in its implicit search scope but not `a`.
-
- 4. The treatment of ambiguity errors has changed. If an ambiguity is encountered
-    in some recursive step of an implicit search, the ambiguity is propagated to the caller.
-    Example: Say you have the following definitions:
-    ```scala
-    class A
-    class B extends C
-    class C
-    implicit def a1: A
-    implicit def a2: A
-    implicit def b(implicit a: A): B
-    implicit def c: C
-    ```
-    and the query `implicitly[C]`.
-
-    This query would now be classified as ambiguous. This makes sense, after all
-    there are two possible solutions, `b(a1)` and `b(a2)`, neither of which is better
-    than the other and both of which are better than the third solution, `c`.
-    By contrast, Scala 2 would have rejected the search for `A` as
-    ambiguous, and subsequently have classified the query `b(implicitly[A])` as a normal fail,
-    which means that the alternative `c` would be chosen as solution!
-
-    Scala 2's somewhat puzzling behavior with respect to ambiguity has been exploited to implement
-    the analogue of a "negated" search in implicit resolution, where a query `Q1` fails if some
-    other query `Q2` succeeds and `Q1` succeeds if `Q2` fails. With the new cleaned up behavior
-    these techniques no longer work. But there is now a new special type `scala.implicits.Not`
-    which implements negation directly. For any query type `Q`: `Not[Q]` succeeds if and only if
-    the implicit search for `Q` fails.
-
- 5. The treatment of divergence errors has also changed. A divergent implicit is
-    treated as a normal failure, after which alternatives are still tried. This also makes
-    sense: Encountering a divergent implicit means that we assume that no finite
-    solution can be found on the corresponding path, but another path can still be tried. By contrast
-    most (but not all) divergence errors in Scala 2 would terminate the implicit
-    search as a whole.
-
- 6. Scala-2 gives a lower level of priority to implicit conversions with call-by-name
-    parameters relative to implicit conversions with call-by-value parameters. Dotty
-    drops this distinction. So the following code snippet would be ambiguous in Dotty:
-    ```scala
-    implicit def conv1(x: Int): A = new A(x)
-    implicit def conv2(x: => Int): A = new A(x)
-    def buzz(y: A) = ???
-    buzz(1)   // error: ambiguous
-    ```
- 7. The rule for picking a _most specific_ alternative among a set of overloaded or implicit
-    alternatives is refined to take context parameters into account. All else
-    being equal, an alternative that takes some context parameters is taken to be less specific
-    than an alternative that takes none. If both alternatives take context parameters, we try
-    to choose between them as if they were methods with regular parameters.
-    The following paragraph in the SLS is affected by this change:
-
-    _Original version:_
-
-    > An alternative A is _more specific_ than an alternative B if the relative weight of A over B is greater than the relative weight of B over A.
-
-    _Modified version:_
-
-    An alternative A is _more specific_ than an alternative B if
-
-     - the relative weight of A over B is greater than the relative weight of B over A, or
-     - the relative weights are the same, and A takes no implicit parameters but B does, or
-     - the relative weights are the same, both A and B take implicit parameters, and
-       A is more specific than B if all implicit parameters in either alternative are
-       replaced by regular parameters.
+  }
+```
+This will now resolve the `implicitly` call to `j`, because `j` is nested
+more deeply than `i`. Previously, this would have resulted in an
+ambiguity error. The previous possibility of an implicit search failure
+due to _shadowing_ (where an implicit is hidden by a nested definition)
+no longer applies.
+
+**3.** Package prefixes no longer contribute to the implicit search scope of a type. Example:
+```scala
+  package p
+  given a as A
+
+  object o {
+    given b as B
+    type C
+  }
+```
+Both `a` and `b` are visible as implicits at the point of the definition
+of `type C`. However, a reference to `p.o.C` outside of package `p` will
+have only `b` in its implicit search scope but not `a`.
+
+**4.** The treatment of ambiguity errors has changed. If an ambiguity is encountered in some recursive step of an implicit search, the ambiguity is propagated to the caller.
+
+Example: Say you have the following definitions:
+```scala
+  class A
+  class B extends C
+  class C
+  implicit def a1: A
+  implicit def a2: A
+  implicit def b(implicit a: A): B
+  implicit def c: C
+```
+and the query `implicitly[C]`.
+
+This query would now be classified as ambiguous. This makes sense, after all
+there are two possible solutions, `b(a1)` and `b(a2)`, neither of which is better
+than the other and both of which are better than the third solution, `c`.
+By contrast, Scala 2 would have rejected the search for `A` as
+ambiguous, and subsequently have classified the query `b(implicitly[A])` as a normal fail,
+which means that the alternative `c` would be chosen as solution!
+
+Scala 2's somewhat puzzling behavior with respect to ambiguity has been exploited to implement
+the analogue of a "negated" search in implicit resolution, where a query `Q1` fails if some
+other query `Q2` succeeds and `Q1` succeeds if `Q2` fails. With the new cleaned up behavior
+these techniques no longer work. But there is now a new special type `scala.implicits.Not`
+which implements negation directly. For any query type `Q`: `Not[Q]` succeeds if and only if
+the implicit search for `Q` fails.
+
+**5.** The treatment of divergence errors has also changed. A divergent implicit is treated as a normal failure, after which alternatives are still tried. This also makes sense: Encountering a divergent implicit means that we assume that no finite solution can be found on the corresponding path, but another path can still be tried. By contrast,
+most (but not all) divergence errors in Scala 2 would terminate the implicit search as a whole.
+
+**6.** Scala-2 gives a lower level of priority to implicit conversions with call-by-name parameters relative to implicit conversions with call-by-value parameters. Dotty drops this distinction. So the following code snippet would be ambiguous in Dotty:
+
+```scala
+  implicit def conv1(x: Int): A = new A(x)
+  implicit def conv2(x: => Int): A = new A(x)
+  def buzz(y: A) = ???
+  buzz(1)   // error: ambiguous
+```
+**7.** The rule for picking a _most specific_ alternative among a set of overloaded or implicit alternatives is refined to take context parameters into account. All else being equal, an alternative that takes some context parameters is taken to be less specific than an alternative that takes none. If both alternatives take context parameters, we try to choose between them as if they were methods with regular parameters. The following paragraph in the SLS is affected by this change:
+
+_Original version:_
+
+> An alternative A is _more specific_ than an alternative B if the relative weight of A over B is greater than the relative weight of B over A.
+
+_Modified version:_
+
+An alternative A is _more specific_ than an alternative B if
+
+ - the relative weight of A over B is greater than the relative weight of B over A, or
+ - the relative weights are the same, and A takes no implicit parameters but B does, or
+ - the relative weights are the same, both A and B take implicit parameters, and A is more specific than B if all implicit parameters in either alternative are replaced by regular parameters.
+
+**8.** The previous disambiguation of implicits based on inheritance depth is refined to make it transitive. Transitivity is important to guarantee that search outcomes are compilation-order independent. Here's a scenario where the previous rules violated transitivity:
+```scala
+  class A extends B
+  object A { given a ... }
+  class B
+  object B extends C { given b ... }
+  class C { given c }
+```
+ Here `a` is more specific than `b` since the companion class `A` is a subclass of the companion class `B`. Also, `b` is more specific than `c`
+   since `object B` extends class `C`. But `a` is not more specific than `c`. This means if `a, b, c` are all applicable implicits, it makes
+   a difference in what order they are compared. If we compare `b` and `c`
+   first, we keep `b` and drop `c`. Then, comparing `a` with `b` we keep `a`. But if we compare `a` with `c` first, we fail with an ambiguity error.
+
+The new rules are as follows: An implicit `a` defined in `A` is more specific than an implicit `b` defined in `B` if
+
+ - `A` extends `B`, or
+ - `A` is an object and the companion class of `A` extends `B`, or
+ - `A` and `B` are objects,
+    `B` does not inherit any implicit members from base classes (*),
+    and the companion class of `A` extends the companion class of `B`.
+
+Condition (*) is new. It is necessary to ensure that the defined relation is transitive.
+
+
+
+
 
 [//]: # todo: expand with precise rules