Skip to content

Emit feature warnings for implicit conversions. #4229

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

Merged
merged 3 commits into from
Apr 27, 2018
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
2 changes: 2 additions & 0 deletions compiler/src/dotty/tools/dotc/core/StdNames.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
11 changes: 7 additions & 4 deletions compiler/src/dotty/tools/dotc/reporting/Reporter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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._
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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] {
Expand Down Expand Up @@ -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 hasPendingErrors: Boolean = false

/** If this reporter buffers messages, remove and return all buffered messages. */
def removeBufferedMessages(implicit ctx: Context): List[MessageContainer] = Nil
Expand Down
9 changes: 2 additions & 7 deletions compiler/src/dotty/tools/dotc/reporting/StoreReporter.scala
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,8 @@ class StoreReporter(outer: Reporter) extends Reporter {
infos += m
}

override def hasPending: Boolean = infos != null && {
infos exists {
case _: Error => true
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
Expand Down
46 changes: 46 additions & 0 deletions compiler/src/dotty/tools/dotc/typer/Checking.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not compare symbol here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't want to force loading the symbol.

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.
Expand Down Expand Up @@ -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 = ()
Expand All @@ -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 = ()
}
7 changes: 6 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/ProtoTypes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,12 @@ object ProtoTypes {
targ = arg.withType(WildcardType)
else {
targ = typerFn(arg)
if (!ctx.reporter.hasPending) {
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))
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions compiler/test/dotty/tools/dotc/CompilationTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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")) +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
11 changes: 11 additions & 0 deletions tests/neg-custom-args/impl-conv/A.scala
Original file line number Diff line number Diff line change
@@ -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
}

}
10 changes: 10 additions & 0 deletions tests/neg-custom-args/impl-conv/B.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package implConv

object B {
import A._

"".foo

val x: Int = "" // error: feature

}