Emit feature warnings for implicit conversions.

We now emit feature warnings when encountering an implicit conversion def,
just like Scala 2 does.

In addition, we also emit a feature warning when an implicit conversion is used,
unless the conversion is an implicit class, or otherwise co-defined with the type
to which it converts, or the conversion is predefined.

Predefined are

  - all members of `Predef`,
  - the `scala.reflect.Selectable.reflectiveSelect` conversion.

We might need to extend this to more conversions.

The motivation for going down this route is the following: Implicit conversions are easily the
most mis-used feature in Scala. If I should venture a guess I'd say that more than 90% of the
uses are mis-guided. The danger of implicit conversions is that they seem simple. It's easy
to grok what they are and in particular new users see the possibilities before realizing the
downsides. So far we put definitions of implicit conversions under a feature flag but this is
not enough. Library writers tend not to be deterred by it, they just add the language import.

(Or, worse, IntelliJ does it for them. Dear IntelliJ Scala team: please change things so that
language imports are not inserted automatically, They are required for a reason, and automatic
insertion does not help!)

Either way, it's the users of a library with implicit conversions that need to be warned
by the compiler, since it is them who will run into troubles if implicit conversions have
surprising behavior.

In what concerns the precise rules, I believe we will have to try this out in the wild for a
while to see how it shapes up and whether we should tweak the rules.

Author: odersky

compiler/src/dotty/tools/dotc/core/StdNames.scala

@@ -436,6 +436,7 @@ object StdNames {
     val head: N                 = "head"
     val higherKinds: N          = "higherKinds"
     val identity: N             = "identity"
+    val implicitConversions: N  = "implicitConversions"
     val implicitly: N           = "implicitly"
     val in: N                   = "in"
     val info: N                 = "info"
@@ -488,6 +489,7 @@ object StdNames {
     val productPrefix: N        = "productPrefix"
     val readResolve: N          = "readResolve"
     val reflect : N             = "reflect"
+    val reflectiveSelectable: N = "reflectiveSelectable"
     val reify : N               = "reify"
     val rootMirror : N          = "rootMirror"
     val run: N                  = "run"

compiler/src/dotty/tools/dotc/reporting/Reporter.scala

@@ -8,7 +8,7 @@ import core.Decorators.PhaseListDecorator
 import collection.mutable
 import java.lang.System.currentTimeMillis
 import core.Mode
-import dotty.tools.dotc.core.Symbols.Symbol
+import dotty.tools.dotc.core.Symbols.{Symbol, NoSymbol}
 import diagnostic.messages._
 import diagnostic._
 import Message._
@@ -39,7 +39,7 @@ trait Reporting { this: Context =>
   def reportWarning(warning: Warning): Unit =
     if (!this.settings.silentWarnings.value) {
       if (this.settings.XfatalWarnings.value) reporter.report(warning.toError)
-      else  reporter.report(warning)
+      else reporter.report(warning)
     }
 
   def deprecationWarning(msg: => Message, pos: SourcePosition = NoSourcePosition): Unit =
@@ -170,7 +170,10 @@ abstract class Reporter extends interfaces.ReporterResult {
   def errorsReported = hasErrors
 
   private[this] var reportedFeaturesUseSites = Set[Symbol]()
-  def isReportedFeatureUseSite(featureTrait: Symbol): Boolean = reportedFeaturesUseSites.contains(featureTrait)
+
+  def isReportedFeatureUseSite(featureTrait: Symbol): Boolean =
+    featureTrait.ne(NoSymbol) && reportedFeaturesUseSites.contains(featureTrait)
+
   def reportNewFeatureUseSite(featureTrait: Symbol): Unit = reportedFeaturesUseSites += featureTrait
 
   val unreportedWarnings = new mutable.HashMap[String, Int] {
@@ -227,7 +230,7 @@ abstract class Reporter extends interfaces.ReporterResult {
     ctx.mode.is(Mode.Printing)
 
   /** Does this reporter contain not yet reported errors or warnings? */
-  def hasPending: Boolean = false
+  def hasPending(implicit ctx: Context): Boolean = false
 
   /** If this reporter buffers messages, remove and return all buffered messages. */
   def removeBufferedMessages(implicit ctx: Context): List[MessageContainer] = Nil

compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala

@@ -30,9 +30,10 @@ class StoreReporter(outer: Reporter) extends Reporter {
     infos += m
   }
 
-  override def hasPending: Boolean = infos != null && {
+  override def hasPending(implicit ctx: Context): Boolean = infos != null && {
     infos exists {
       case _: Error => true
+      case m: ConditionalWarning => m.enablingOption.value
       case _: Warning => true
       case _ => false
     }

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

@@ -576,6 +576,49 @@ trait Checking {
     case _ =>
   }
 
+  /** If `sym` is an implicit conversion, check that implicit conversions are enabled.
+   *  @pre  sym.is(Implicit)
+   */
+  def checkImplicitConversionDefOK(sym: Symbol)(implicit ctx: Context): Unit = sym.info.stripPoly match {
+    case mt @ MethodType(_ :: Nil)
+    if !mt.isImplicitMethod && !sym.is(Synthetic) => // it's a conversion
+      checkFeature(
+        defn.LanguageModuleClass, nme.implicitConversions,
+        i"Definition of implicit conversion $sym",
+        ctx.owner.topLevelClass,
+        sym.pos)
+    case _ =>
+  }
+
+  /** If `sym` is an implicit conversion, check that that implicit conversions are enabled, unless
+   *    - it is synthetic
+   *    - it is has the same owner as one of the classes it converts to (modulo companions)
+   *    - it is defined in Predef
+   *    - it is the scala.reflect.Selectable.reflectiveSelectable conversion
+   */
+  def checkImplicitConversionUseOK(sym: Symbol, pos: Position)(implicit ctx: Context): Unit = {
+    val conversionOK =
+      !sym.exists ||
+      sym.is(Synthetic) ||
+      sym.info.finalResultType.classSymbols.exists(_.owner.isLinkedWith(sym.owner)) ||
+      defn.isPredefClass(sym.owner) ||
+      sym.name == nme.reflectiveSelectable && sym.maybeOwner.maybeOwner.maybeOwner == defn.ScalaPackageClass
+    if (!conversionOK)
+      checkFeature(defn.LanguageModuleClass, nme.implicitConversions,
+        i"Use of implicit conversion ${sym.showLocated}", NoSymbol, pos)
+  }
+
+  /** Issue a feature warning if feature is not enabled */
+  def checkFeature(base: ClassSymbol,
+                   name: TermName,
+                   description: => String,
+                   featureUseSite: Symbol,
+                   pos: Position)(implicit ctx: Context): Unit =
+    if (!ctx.featureEnabled(base, name))
+      ctx.featureWarning(name.toString, description,
+        isScala2Feature = base.isContainedIn(defn.LanguageModuleClass),
+        featureUseSite, required = false, pos)
+
   /** Check that `tp` is a class type and that any top-level type arguments in this type
    *  are feasible, i.e. that their lower bound conforms to their upper bound. If a type
    *  argument is infeasible, issue and error and continue with upper bound.
@@ -866,6 +909,8 @@ trait NoChecking extends ReChecking {
   override def checkStable(tp: Type, pos: Position)(implicit ctx: Context): Unit = ()
   override def checkClassType(tp: Type, pos: Position, traitReq: Boolean, stablePrefixReq: Boolean)(implicit ctx: Context): Type = tp
   override def checkImplicitParamsNotSingletons(vparamss: List[List[ValDef]])(implicit ctx: Context): Unit = ()
+  override def checkImplicitConversionDefOK(sym: Symbol)(implicit ctx: Context): Unit = ()
+  override def checkImplicitConversionUseOK(sym: Symbol, pos: Position)(implicit ctx: Context): Unit = ()
   override def checkFeasibleParent(tp: Type, pos: Position, where: => String = "")(implicit ctx: Context): Type = tp
   override def checkInlineConformant(tree: Tree, what: => String)(implicit ctx: Context) = ()
   override def checkNoDoubleDeclaration(cls: Symbol)(implicit ctx: Context): Unit = ()
@@ -877,4 +922,5 @@ trait NoChecking extends ReChecking {
   override def checkCaseInheritance(parentSym: Symbol, caseCls: ClassSymbol, pos: Position)(implicit ctx: Context) = ()
   override def checkNoForwardDependencies(vparams: List[ValDef])(implicit ctx: Context): Unit = ()
   override def checkMembersOK(tp: Type, pos: Position)(implicit ctx: Context): Type = tp
+  override def checkFeature(base: ClassSymbol, name: TermName, description: => String, featureUseSite: Symbol, pos: Position)(implicit ctx: Context): Unit = ()
 }

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

@@ -250,6 +250,12 @@ object ProtoTypes {
         else {
           targ = typerFn(arg)
           if (!ctx.reporter.hasPending) {
+            // There is something fishy going on. Run pos/t1756.scala with -feature.
+            // You will get an orphan type parameter for CI when pickling.
+            // The difference is that with -feature an `implicitConversions` warning
+            // is issued, which means the next two statements are not executed.
+            // It seems we are missing then some constraint instantiations because `evalState`
+            // is not updated.
             myTypedArg = myTypedArg.updated(arg, targ)
             evalState = evalState.updated(arg, (ctx.typerState, ctx.typerState.constraint))
           }

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

@@ -1413,7 +1413,10 @@ class Typer extends Namer
     val tparams1 = tparams mapconserve (typed(_).asInstanceOf[TypeDef])
     val vparamss1 = vparamss nestedMapconserve (typed(_).asInstanceOf[ValDef])
     vparamss1.foreach(checkNoForwardDependencies)
-    if (sym is Implicit) checkImplicitParamsNotSingletons(vparamss1)
+    if (sym is Implicit) {
+      checkImplicitParamsNotSingletons(vparamss1)
+      checkImplicitConversionDefOK(sym)
+    }
     val tpt1 = checkSimpleKinded(typedType(tpt))
 
     var rhsCtx = ctx
@@ -2450,6 +2453,7 @@ class Typer extends Namer
       if (ctx.mode.is(Mode.ImplicitsEnabled))
         inferView(tree, pt) match {
           case SearchSuccess(inferred, _, _) =>
+            checkImplicitConversionUseOK(inferred.symbol, tree.pos)
             readapt(inferred)(ctx.retractMode(Mode.ImplicitsEnabled))
           case failure: SearchFailure =>
             if (pt.isInstanceOf[ProtoType] && !failure.isAmbiguous)

compiler/test/dotty/tools/dotc/CompilationTests.scala

@@ -181,6 +181,7 @@ class CompilationTests extends ParallelTesting {
     compileFilesInDir("tests/neg-kind-polymorphism", defaultOptions and "-Ykind-polymorphism") +
     compileFilesInDir("tests/neg-custom-args/fatal-warnings", defaultOptions.and("-Xfatal-warnings")) +
     compileFilesInDir("tests/neg-custom-args/allow-double-bindings", allowDoubleBindings) +
+    compileDir("tests/neg-custom-args/impl-conv", defaultOptions.and("-Xfatal-warnings", "-feature")) +
     compileFile("tests/neg-custom-args/i3246.scala", scala2Mode) +
     compileFile("tests/neg-custom-args/overrideClass.scala", scala2Mode) +
     compileFile("tests/neg-custom-args/autoTuplingTest.scala", defaultOptions.and("-language:noAutoTupling")) +

tests/neg-custom-args/impl-conv/A.scala

@@ -0,0 +1,11 @@
+package implConv
+
+object A {
+
+  implicit def s2i(x: String): Int = Integer.parseInt(x) // error: feature
+
+  implicit class Foo(x: String) {
+    def foo = x.length
+  }
+
+}

tests/neg-custom-args/impl-conv/B.scala

@@ -0,0 +1,10 @@
+package implConv
+
+object B {
+  import A._
+
+  "".foo
+
+  val x: Int = ""  // error: feature
+
+}