From 7ec4feab0b0e6587a32a97fcd97969a6d88e8520 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 Apr 2018 15:55:33 +0200 Subject: [PATCH 1/3] 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. --- .../src/dotty/tools/dotc/core/StdNames.scala | 2 + .../dotty/tools/dotc/reporting/Reporter.scala | 11 +++-- .../tools/dotc/reporting/StoreReporter.scala | 3 +- .../src/dotty/tools/dotc/typer/Checking.scala | 46 +++++++++++++++++++ .../dotty/tools/dotc/typer/ProtoTypes.scala | 6 +++ .../src/dotty/tools/dotc/typer/Typer.scala | 6 ++- .../dotty/tools/dotc/CompilationTests.scala | 1 + tests/neg-custom-args/impl-conv/A.scala | 11 +++++ tests/neg-custom-args/impl-conv/B.scala | 10 ++++ 9 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 tests/neg-custom-args/impl-conv/A.scala create mode 100644 tests/neg-custom-args/impl-conv/B.scala diff --git a/compiler/src/dotty/tools/dotc/core/StdNames.scala b/compiler/src/dotty/tools/dotc/core/StdNames.scala index a6543572b23f..bfad8f6739f6 100644 --- a/compiler/src/dotty/tools/dotc/core/StdNames.scala +++ b/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" diff --git a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala b/compiler/src/dotty/tools/dotc/reporting/Reporter.scala index 73443871c2bb..7c4f9cabfbc5 100644 --- a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala +++ b/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 diff --git a/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala b/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala index acb0d573507d..41d13d555ede 100644 --- a/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala +++ b/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 } diff --git a/compiler/src/dotty/tools/dotc/typer/Checking.scala b/compiler/src/dotty/tools/dotc/typer/Checking.scala index 698fdaff0c15..f817de154bad 100644 --- a/compiler/src/dotty/tools/dotc/typer/Checking.scala +++ b/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 = () } diff --git a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala b/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala index 5aa4a2fbfd31..547bff0272d2 100644 --- a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala +++ b/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)) } diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 431721dc44e6..5be5585cebd1 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/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) diff --git a/compiler/test/dotty/tools/dotc/CompilationTests.scala b/compiler/test/dotty/tools/dotc/CompilationTests.scala index 2a7b78b6c64c..41ac03245b9e 100644 --- a/compiler/test/dotty/tools/dotc/CompilationTests.scala +++ b/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")) + diff --git a/tests/neg-custom-args/impl-conv/A.scala b/tests/neg-custom-args/impl-conv/A.scala new file mode 100644 index 000000000000..717e2378aae7 --- /dev/null +++ b/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 + } + +} diff --git a/tests/neg-custom-args/impl-conv/B.scala b/tests/neg-custom-args/impl-conv/B.scala new file mode 100644 index 000000000000..30a6be36af8a --- /dev/null +++ b/tests/neg-custom-args/impl-conv/B.scala @@ -0,0 +1,10 @@ +package implConv + +object B { + import A._ + + "".foo + + val x: Int = "" // error: feature + +} From b9e1e7c6e05a7a2a986d81f403dec8b205e6e8af Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 2 Apr 2018 23:12:05 +0200 Subject: [PATCH 2/3] Fix test --- .../dotty/tools/dotc/reporting/UserDefinedErrorMessages.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler/test/dotty/tools/dotc/reporting/UserDefinedErrorMessages.scala b/compiler/test/dotty/tools/dotc/reporting/UserDefinedErrorMessages.scala index bb8db2aff35c..864bc98e3f26 100644 --- a/compiler/test/dotty/tools/dotc/reporting/UserDefinedErrorMessages.scala +++ b/compiler/test/dotty/tools/dotc/reporting/UserDefinedErrorMessages.scala @@ -88,6 +88,7 @@ class UserDefinedErrorMessages extends ErrorMessagesTest { checkMessagesAfter("frontend") { """ |class C { + | import scala.language.implicitConversions | @annotation.implicitAmbiguous("msg A=${A}") | implicit def f[A](x: Int): String = "f was here" | implicit def g(x: Int): String = "f was here" From 1f69d70d771b96c2923fee3400fbcd02439e15cf Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 3 Apr 2018 11:57:11 +0200 Subject: [PATCH 3/3] Fix crash when warnings are issued --- .../src/dotty/tools/dotc/reporting/Reporter.scala | 2 +- .../dotty/tools/dotc/reporting/StoreReporter.scala | 10 ++-------- .../src/dotty/tools/dotc/typer/ProtoTypes.scala | 13 ++++++------- 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala b/compiler/src/dotty/tools/dotc/reporting/Reporter.scala index 7c4f9cabfbc5..f97d8b7a246a 100644 --- a/compiler/src/dotty/tools/dotc/reporting/Reporter.scala +++ b/compiler/src/dotty/tools/dotc/reporting/Reporter.scala @@ -230,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(implicit ctx: Context): Boolean = false + def hasPendingErrors: Boolean = false /** If this reporter buffers messages, remove and return all buffered messages. */ def removeBufferedMessages(implicit ctx: Context): List[MessageContainer] = Nil diff --git a/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala b/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala index 41d13d555ede..3caaf256fed7 100644 --- a/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala +++ b/compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala @@ -30,14 +30,8 @@ class StoreReporter(outer: Reporter) extends Reporter { infos += m } - 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 - } - } + override def hasPendingErrors: Boolean = + infos != null && infos.exists(_.isInstanceOf[Error]) override def removeBufferedMessages(implicit ctx: Context): List[MessageContainer] = if (infos != null) try infos.toList finally infos = null diff --git a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala b/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala index 547bff0272d2..0bba31b165f1 100644 --- a/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala +++ b/compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala @@ -249,13 +249,12 @@ object ProtoTypes { targ = arg.withType(WildcardType) 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. + if (!ctx.reporter.hasPendingErrors) { + // FIXME: This can swallow warnings by updating the typerstate from a nested + // context that gets discarded later. But we do have to update the + // typerstate if there are no errors. If we also omitted the next two lines + // when warning were emitted, `pos/t1756.scala` would fail when run with -feature. + // It would produce an orphan type parameter for CI when pickling. myTypedArg = myTypedArg.updated(arg, targ) evalState = evalState.updated(arg, (ctx.typerState, ctx.typerState.constraint)) }