From f07697b25294eaafb1c86698c44a699ec1c0d1ba Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 8 Mar 2017 11:35:05 +0100 Subject: [PATCH 1/5] Disallow subtypes of Function1 acting as implicit conversions The new test `falseView.scala` shows the problem. We might create an implicit value of some type that happens to be a subtype of Function1. We might now expect that this gives us an implicit conversion, this is most often unintended and surprising. See the comment in Implicits#discardForView for a discussion why we picked the particular scheme implemented here. --- .../dotty/tools/dotc/core/Definitions.scala | 2 ++ .../dotty/tools/dotc/typer/Implicits.scala | 32 ++++++++++++++++--- tests/neg/falseView.scala | 7 ++++ tests/pos/t2421_delitedsl.scala | 3 ++ tests/run/t8280.check | 3 +- tests/run/t8280.scala | 3 +- 6 files changed, 42 insertions(+), 8 deletions(-) create mode 100644 tests/neg/falseView.scala diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index 3cab75f93125..fcbb2f9741b2 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -299,6 +299,8 @@ class Definitions { lazy val ScalaPredefModuleRef = ctx.requiredModuleRef("scala.Predef") def ScalaPredefModule(implicit ctx: Context) = ScalaPredefModuleRef.symbol + lazy val Predef_ConformsR = ScalaPredefModule.requiredClass("$less$colon$less").typeRef + def Predef_Conforms(implicit ctx: Context) = Predef_ConformsR.symbol lazy val Predef_conformsR = ScalaPredefModule.requiredMethodRef("$conforms") def Predef_conforms(implicit ctx: Context) = Predef_conformsR.symbol lazy val Predef_classOfR = ScalaPredefModule.requiredMethodRef("classOf") diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 759cc62e94a3..933e26564e04 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -82,11 +82,33 @@ object Implicits { case tpw: TermRef => false // can't discard overloaded refs case tpw => - //if (ctx.typer.isApplicable(tp, argType :: Nil, resultType)) - // println(i"??? $tp is applicable to $this / typeSymbol = ${tpw.typeSymbol}") - !tpw.derivesFrom(defn.FunctionClass(1)) || - ref.symbol == defn.Predef_conforms // - // as an implicit conversion, Predef.$conforms is a no-op, so exclude it + // Only direct instances of Function1 and direct or indirect instances of <:< are eligible as views. + // However, Predef.$conforms is not eligible, because it is a no-op. + // + // In principle, it would be cleanest if only implicit methods qualified + // as implicit conversions. The reasons for deviating are as follows: + // Keeping Function1: It's still used quite often (for instance, view + // bounds map to implicits of function types) and there is no feasible workaround. + // One tempting workaround would be to add a global def + // + // implicit def convertIfFuntion1[A, B](x: A)(implicit ev: A => B): B = ev(a) + // + // But that would throw out the baby with the bathwater. Now, every subtype of + // function gives again rise to an implicit conversion. So it's better to just accept + // function types in their dual roles. + // + // The reason for the treatment of <:< and conforms is similar. We could + // avoid the clause by having a standard conversion like this in Predef: + // + // implicit def convertIfConforms[A, B](x: A)(implicit ev: A <:< B): B = ev(a) + // + // But that would slow down implicit search a lot, because this conversion is + // eligible for all pairs of types, and therefore is tried a lot. So we + // emulate the existence of a such a conversion directly in the search. + // The reason for leaving out `Predef_conforms` is that we know it adds + // nothing since it only relates subtype with supertype. + !tpw.isRef(defn.FunctionClass(1)) && + (!tpw.derivesFrom(defn.Predef_Conforms) || ref.symbol == defn.Predef_conforms) } def discardForValueType(tpw: Type): Boolean = tpw match { diff --git a/tests/neg/falseView.scala b/tests/neg/falseView.scala new file mode 100644 index 000000000000..613abe3f1f9c --- /dev/null +++ b/tests/neg/falseView.scala @@ -0,0 +1,7 @@ +object Test { + + private implicit val xs: Map[String, Int] = ??? + + val x: Int = "abc" // error + +} diff --git a/tests/pos/t2421_delitedsl.scala b/tests/pos/t2421_delitedsl.scala index 22f1ecd85e06..bde3593c9ee4 100644 --- a/tests/pos/t2421_delitedsl.scala +++ b/tests/pos/t2421_delitedsl.scala @@ -1,6 +1,9 @@ trait DeliteDSL { abstract class <~<[-From, +To] extends (From => To) + implicit def trivial[A]: A <~< A = new (A <~< A) {def apply(x: A) = x} + implicit def convert_<-<[A, B](x: A)(implicit ev: A <~< B): B = ev(x) + trait Forcible[T] object Forcible { diff --git a/tests/run/t8280.check b/tests/run/t8280.check index 44c51f5aa0e9..b5885df482f9 100644 --- a/tests/run/t8280.check +++ b/tests/run/t8280.check @@ -1,7 +1,6 @@ -Int -Int Long Int Int Int Int +Int diff --git a/tests/run/t8280.scala b/tests/run/t8280.scala index f433dcc32581..1d2c56b85e4b 100644 --- a/tests/run/t8280.scala +++ b/tests/run/t8280.scala @@ -37,7 +37,8 @@ object Moop1 { implicit object f1 extends (Int => String) { def apply(x: Int): String = "Int" } implicit val f2: Long => String = _ => "Long" - println(5: String) + //println(5: String) + // This picked f1 before, but is now disallowed since subtypes of functions are no longer implicit conversions } } From 7b5cdbc77b5b3e91a90c21a1ed69bc51355243c8 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 8 Mar 2017 11:44:45 +0100 Subject: [PATCH 2/5] Keep old behavior under -language:Scala2 --- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 933e26564e04..e1e4a9259365 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -107,8 +107,14 @@ object Implicits { // emulate the existence of a such a conversion directly in the search. // The reason for leaving out `Predef_conforms` is that we know it adds // nothing since it only relates subtype with supertype. - !tpw.isRef(defn.FunctionClass(1)) && - (!tpw.derivesFrom(defn.Predef_Conforms) || ref.symbol == defn.Predef_conforms) + // + // We keep the old behavior under -language:Scala2. + val isFunction = + if (ctx.scala2Mode) tpw.derivesFrom(defn.FunctionClass(1)) + else tpw.isRef(defn.FunctionClass(1)) + val isConforms = + tpw.derivesFrom(defn.Predef_Conforms) || ref.symbol == defn.Predef_conforms + !(isFunction || isConforms) } def discardForValueType(tpw: Type): Boolean = tpw match { From d1115f58e29a4de2f52933ab0f92326c660f5f70 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 8 Mar 2017 11:55:40 +0100 Subject: [PATCH 3/5] Add puzzler 54 as a test --- tests/run/puzzler54.scala | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 tests/run/puzzler54.scala diff --git a/tests/run/puzzler54.scala b/tests/run/puzzler54.scala new file mode 100644 index 000000000000..9dd4cbb47832 --- /dev/null +++ b/tests/run/puzzler54.scala @@ -0,0 +1,13 @@ +// Scala Puzzler 54 +object Test { + case class Card(number: Int, suit: String = "clubs") { + val value = (number % 13) + 1 // ace = 1, king = 13 + def isInDeck(implicit deck: List[Card]) = deck contains this + } + + def main(args: Array[String]) = { + implicit val deck = List(Card(1, "clubs")) + implicit def intToCard(n: Int): Card = Card(n) + assert(1.isInDeck) + } +} From 180dfdc7e81d632e599fb0a545025720e8000573 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 8 Mar 2017 13:51:44 +0100 Subject: [PATCH 4/5] Fix condition logic I introduced an error in the refactoring two commits ago. --- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index e1e4a9259365..6dbb2216cec3 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -113,7 +113,7 @@ object Implicits { if (ctx.scala2Mode) tpw.derivesFrom(defn.FunctionClass(1)) else tpw.isRef(defn.FunctionClass(1)) val isConforms = - tpw.derivesFrom(defn.Predef_Conforms) || ref.symbol == defn.Predef_conforms + tpw.derivesFrom(defn.Predef_Conforms) && ref.symbol != defn.Predef_conforms !(isFunction || isConforms) } From aa2f9078d76a21d828a06b8e324d31a502ee505c Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Wed, 8 Mar 2017 22:06:22 +0100 Subject: [PATCH 5/5] Drop special case around Function1 Now only Scala2 mode treats Function1's as implicit conversions. Instead we introduce a new subclass ImplicitConverter of Function1, instances of which are turned into implicit conversions. --- .../dotty/tools/dotc/core/Definitions.scala | 2 ++ .../dotty/tools/dotc/typer/Implicits.scala | 30 +++++++------------ library/src/dotty/DottyPredef.scala | 20 +++++++++++++ .../typerep-stephane.scala | 0 tests/{pos => pos-scala2}/viewtest1.scala | 0 tests/pos/t0786.scala | 2 +- tests/run/iterator-from.scala | 6 ++-- tests/run/t8280.scala | 6 ++-- 8 files changed, 40 insertions(+), 26 deletions(-) rename tests/{pos => pos-scala2}/typerep-stephane.scala (100%) rename tests/{pos => pos-scala2}/viewtest1.scala (100%) diff --git a/compiler/src/dotty/tools/dotc/core/Definitions.scala b/compiler/src/dotty/tools/dotc/core/Definitions.scala index fcbb2f9741b2..1be47c1daad7 100644 --- a/compiler/src/dotty/tools/dotc/core/Definitions.scala +++ b/compiler/src/dotty/tools/dotc/core/Definitions.scala @@ -338,6 +338,8 @@ class Definitions { def DottyPredefModule(implicit ctx: Context) = DottyPredefModuleRef.symbol def Predef_eqAny(implicit ctx: Context) = DottyPredefModule.requiredMethod(nme.eqAny) + lazy val Predef_ImplicitConverterR = DottyPredefModule.requiredClass("ImplicitConverter").typeRef + def Predef_ImplicitConverter(implicit ctx: Context) = Predef_ImplicitConverterR.symbol lazy val DottyArraysModuleRef = ctx.requiredModuleRef("dotty.runtime.Arrays") def DottyArraysModule(implicit ctx: Context) = DottyArraysModuleRef.symbol diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 6dbb2216cec3..ebbcbcc95f50 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -86,35 +86,25 @@ object Implicits { // However, Predef.$conforms is not eligible, because it is a no-op. // // In principle, it would be cleanest if only implicit methods qualified - // as implicit conversions. The reasons for deviating are as follows: - // Keeping Function1: It's still used quite often (for instance, view - // bounds map to implicits of function types) and there is no feasible workaround. - // One tempting workaround would be to add a global def - // - // implicit def convertIfFuntion1[A, B](x: A)(implicit ev: A => B): B = ev(a) - // - // But that would throw out the baby with the bathwater. Now, every subtype of - // function gives again rise to an implicit conversion. So it's better to just accept - // function types in their dual roles. - // - // The reason for the treatment of <:< and conforms is similar. We could - // avoid the clause by having a standard conversion like this in Predef: + // as implicit conversions. We could achieve that by having standard conversions like + // this in Predef: // // implicit def convertIfConforms[A, B](x: A)(implicit ev: A <:< B): B = ev(a) + // implicit def convertIfConverter[A, B](x: A)(implicit ev: ImplicitConverter[A, B]): B = ev(a) // - // But that would slow down implicit search a lot, because this conversion is - // eligible for all pairs of types, and therefore is tried a lot. So we - // emulate the existence of a such a conversion directly in the search. + // (Once `<:<` inherits from `ImplicitConverter` we only need the 2nd one.) + // But clauses like this currently slow down implicit search a lot, because + // they are eligible for all pairs of types, and therefore are tried too often. + // We emulate instead these conversions directly in the search. // The reason for leaving out `Predef_conforms` is that we know it adds // nothing since it only relates subtype with supertype. // // We keep the old behavior under -language:Scala2. - val isFunction = - if (ctx.scala2Mode) tpw.derivesFrom(defn.FunctionClass(1)) - else tpw.isRef(defn.FunctionClass(1)) + val isFunctionInS2 = ctx.scala2Mode && tpw.derivesFrom(defn.FunctionClass(1)) + val isImplicitConverter = tpw.derivesFrom(defn.Predef_ImplicitConverter) val isConforms = tpw.derivesFrom(defn.Predef_Conforms) && ref.symbol != defn.Predef_conforms - !(isFunction || isConforms) + !(isFunctionInS2 || isImplicitConverter || isConforms) } def discardForValueType(tpw: Type): Boolean = tpw match { diff --git a/library/src/dotty/DottyPredef.scala b/library/src/dotty/DottyPredef.scala index 12040e0f3462..e78fa9239865 100644 --- a/library/src/dotty/DottyPredef.scala +++ b/library/src/dotty/DottyPredef.scala @@ -36,4 +36,24 @@ object DottyPredef { implicit def eqNumFloat : Eq[Number, Float] = Eq implicit def eqDoubleNum: Eq[Double, Number] = Eq implicit def eqNumDouble: Eq[Number, Double] = Eq + + /** A class for implicit values that can serve as implicit conversions + * The implicit resolution algorithm will act as if there existed + * the additional implicit definition: + * + * def $implicitConversion[T, U](x: T)(c: ImplicitConverter[T, U]): U = c(x) + * + * However, the presence of this definition would slow down implicit search since + * its outermost type matches any pair of types. Therefore, implicit search + * contains a special case in `Implicits#discardForView` which emulates the + * conversion in a more efficient way. + * + * Note that this is a SAM class - function literals are automatically converted + * to `ImplicitConverter` values. + * + * Also note that in bootstrapped dotty, `Predef.<:<` should inherit from + * `ImplicitConverter`. This would cut the number of special cases in + * `discardForView` from two to one. + */ + abstract class ImplicitConverter[-T, +U] extends Function1[T, U] } diff --git a/tests/pos/typerep-stephane.scala b/tests/pos-scala2/typerep-stephane.scala similarity index 100% rename from tests/pos/typerep-stephane.scala rename to tests/pos-scala2/typerep-stephane.scala diff --git a/tests/pos/viewtest1.scala b/tests/pos-scala2/viewtest1.scala similarity index 100% rename from tests/pos/viewtest1.scala rename to tests/pos-scala2/viewtest1.scala diff --git a/tests/pos/t0786.scala b/tests/pos/t0786.scala index b320de0ed6c3..9346afdfff8d 100644 --- a/tests/pos/t0786.scala +++ b/tests/pos/t0786.scala @@ -15,7 +15,7 @@ object ImplicitProblem { def eval = f(nullval[T]).eval + 1 } - def depth[T](n: T)(implicit ev: T => Rep[T]) = n.eval + def depth[T](n: T)(implicit ev: T => Rep[T]) = ev(n).eval def main(args: Array[String]): Unit = { println(depth(nullval[M[Int]])) // (1) this works diff --git a/tests/run/iterator-from.scala b/tests/run/iterator-from.scala index 4f403680c128..c7c0f9809cc5 100644 --- a/tests/run/iterator-from.scala +++ b/tests/run/iterator-from.scala @@ -11,7 +11,9 @@ object Test extends dotty.runtime.LegacyApp { val maxKey = 50 val maxValue = 50 - def testSet[A <% Ordered[A]](set: SortedSet[A], list: List[A]): Unit = { + implicit def convertIfView[A](x: A)(implicit view: A => Ordered[A]): Ordered[A] = view(x) + + def testSet[A: Ordering](set: SortedSet[A], list: List[A]): Unit = { val distinctSorted = list.distinct.sorted assertEquals("Set size wasn't the same as list sze", set.size, distinctSorted.size) @@ -24,7 +26,7 @@ object Test extends dotty.runtime.LegacyApp { } } - def testMap[A <% Ordered[A], B](map: SortedMap[A, B], list: List[(A, B)]): Unit = { + def testMap[A: Ordering, B](map: SortedMap[A, B], list: List[(A, B)]): Unit = { val distinctSorted = distinctByKey(list).sortBy(_._1) assertEquals("Map size wasn't the same as list sze", map.size, distinctSorted.size) diff --git a/tests/run/t8280.scala b/tests/run/t8280.scala index 1d2c56b85e4b..5fcbad0a3ab1 100644 --- a/tests/run/t8280.scala +++ b/tests/run/t8280.scala @@ -74,14 +74,14 @@ object Moop3 { // Dotty deviation. This fails for Dotty with ambiguity error for similar reasons as ob1. } object ob2 { - implicit val f1: Int => String = _ => "Int" + implicit val f1: ImplicitConverter[Int, String] = _ => "Int" implicit def f2(x: Long): String = "Long" println(5: String) } object ob3 { - implicit val f1: Int => String = _ => "Int" - implicit val f2: Long => String = _ => "Long" + implicit val f1: ImplicitConverter[Int, String] = _ => "Int" + implicit val f2: ImplicitConverter[Long, String] = _ => "Long" println(5: String) }