From faf52136724ada560f13eb0af066f5b0a975fea4 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sat, 9 Feb 2019 21:25:12 +0100 Subject: [PATCH 1/6] Trial: Make implicit resolution block scoped So far, nesting was just one of several criteria for selecting a best implicit. What would happen if nesting was most significant? I.e. inner implicits would always win over outer ones? This has the potential to simplify the rules, gain effiviency, and solve the local consistency problem since we can disambiguate implicits using a local definition (see implicit-disambiguation.scala as a test case). --- .../dotty/tools/dotc/typer/Implicits.scala | 1 + tests/run/implicit-disambiguation.check | 1 + tests/run/implicit-disambiguation.scala | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+) create mode 100644 tests/run/implicit-disambiguation.check create mode 100644 tests/run/implicit-disambiguation.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index f2717963fe47..5a78532fa554 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1148,6 +1148,7 @@ trait Implicits { self: Typer => */ def compareCandidate(prev: SearchSuccess, ref: TermRef, level: Int): Int = if (prev.ref eq ref) 0 + else if (prev.level != level) prev.level - level else nestedContext().test(implicit ctx => compare(prev.ref, ref, prev.level, level)) /** If `alt1` is also a search success, try to disambiguate as follows: diff --git a/tests/run/implicit-disambiguation.check b/tests/run/implicit-disambiguation.check new file mode 100644 index 000000000000..223b7836fb19 --- /dev/null +++ b/tests/run/implicit-disambiguation.check @@ -0,0 +1 @@ +B diff --git a/tests/run/implicit-disambiguation.scala b/tests/run/implicit-disambiguation.scala new file mode 100644 index 000000000000..c1b7de40a0f5 --- /dev/null +++ b/tests/run/implicit-disambiguation.scala @@ -0,0 +1,20 @@ +abstract class A { + def show: String +} +class B extends A { + def show = "B" +} +class C extends A { + def show = "C" +} +object M { + def f given B, C : String = { + implied a for A = infer[B] + infer[A].show + } +} +object Test extends App { + implied b for B + implied c for C + println(M.f) +} From 19256ef362b474210cb01bf70b6a0d0270daf115 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 10 Feb 2019 11:12:56 +0100 Subject: [PATCH 2/6] Drop logic which is now redundant Since we always prefer inner implicits over outer ones, no need to also consider levels when disambiguating. --- .../src/dotty/tools/dotc/typer/Applications.scala | 13 +++---------- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 6 ++---- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/typer/Applications.scala b/compiler/src/dotty/tools/dotc/typer/Applications.scala index 5f3449a89080..465b897538e3 100644 --- a/compiler/src/dotty/tools/dotc/typer/Applications.scala +++ b/compiler/src/dotty/tools/dotc/typer/Applications.scala @@ -1181,9 +1181,6 @@ trait Applications extends Compatibility { self: Typer with Dynamic => /** Compare to alternatives of an overloaded call or an implicit search. * * @param alt1, alt2 Non-overloaded references indicating the two choices - * @param level1, level2 If alternatives come from a comparison of two contextual - * implicit candidates, the nesting levels of the candidates. - * In all other cases the nesting levels are both 0. * @return 1 if 1st alternative is preferred over 2nd * -1 if 2nd alternative is preferred over 1st * 0 if neither alternative is preferred over the other @@ -1191,11 +1188,10 @@ trait Applications extends Compatibility { self: Typer with Dynamic => * An alternative A1 is preferred over an alternative A2 if it wins in a tournament * that awards one point for each of the following: * - * - A1 is nested more deeply than A2 - * - The nesting levels of A1 and A2 are the same, and A1's owner derives from A2's owner + * - A1's owner derives from A2's owner. * - A1's type is more specific than A2's type. */ - def compare(alt1: TermRef, alt2: TermRef, nesting1: Int = 0, nesting2: Int = 0)(implicit ctx: Context): Int = track("compare") { trace(i"compare($alt1, $alt2)", overload) { + def compare(alt1: TermRef, alt2: TermRef)(implicit ctx: Context): Int = track("compare") { trace(i"compare($alt1, $alt2)", overload) { assert(alt1 ne alt2) @@ -1305,10 +1301,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic => val owner1 = if (alt1.symbol.exists) alt1.symbol.owner else NoSymbol val owner2 = if (alt2.symbol.exists) alt2.symbol.owner else NoSymbol - val ownerScore = - if (nesting1 > nesting2) 1 - else if (nesting1 < nesting2) -1 - else compareOwner(owner1, owner2) + val ownerScore = compareOwner(owner1, owner2) val tp1 = stripImplicit(alt1.widen) val tp2 = stripImplicit(alt2.widen) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 5a78532fa554..99d71b68c642 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1034,9 +1034,6 @@ trait Implicits { self: Typer => private def isCoherent = pt.isRef(defn.EqlClass) - private val cmpContext = nestedContext() - private val cmpCandidates = (c1: Candidate, c2: Candidate) => compare(c1.ref, c2.ref, c1.level, c2.level)(cmpContext) - /** The expected type for the searched implicit */ lazy val fullProto: Type = implicitProto(pt, identity) @@ -1143,13 +1140,14 @@ trait Implicits { self: Typer => /** Search a list of eligible implicit references */ def searchImplicits(eligible: List[Candidate], contextual: Boolean): SearchResult = { + /** Compare previous success with reference and level to determine which one would be chosen, if * an implicit starting with the reference was found. */ def compareCandidate(prev: SearchSuccess, ref: TermRef, level: Int): Int = if (prev.ref eq ref) 0 else if (prev.level != level) prev.level - level - else nestedContext().test(implicit ctx => compare(prev.ref, ref, prev.level, level)) + else nestedContext().test(implicit ctx => compare(prev.ref, ref)) /** If `alt1` is also a search success, try to disambiguate as follows: * - If alt2 is preferred over alt1, pick alt2, otherwise return an From b673297790accc679b074303ff70a38c92165944 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 10 Feb 2019 11:35:44 +0100 Subject: [PATCH 3/6] Drop implicit shadowing checks Since implicit selection is now inner to outer, shadowing checks are less important than before. Shadowing is necessary if we treat implicit search as a synthesis for untyped terms. I.e. come up with an untyped term and then check whether that term is typeable and refers to the original implicit. If there's a nested definition with the same name, that fails. This viewpoint is very easy to spec but a bit unnatural. In all other instances of meta programming we deal with typed terms. If we synthesize a typed term directly, then name resolution is already done and shadowing is immaterial. Besides, shadowing tests, if they fail, are almost always much more suprising than enlightening. --- compiler/src/dotty/tools/dotc/typer/Implicits.scala | 3 ++- tests/{neg => pos}/implicit-shadowing.scala | 4 ++-- tests/run/i5224.check | 2 +- tests/run/i5224.scala | 2 ++ 4 files changed, 7 insertions(+), 4 deletions(-) rename tests/{neg => pos}/implicit-shadowing.scala (75%) diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index 99d71b68c642..cfbb24316a11 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -1105,7 +1105,8 @@ trait Implicits { self: Typer => } } } - else if (contextual && !ctx.mode.is(Mode.ImplicitShadowing) && + else if (false && + contextual && !ctx.mode.is(Mode.ImplicitShadowing) && !shadowing.tpe.isError && !refSameAs(shadowing)) { implicits.println(i"SHADOWING $ref in ${ref.termSymbol.maybeOwner} is shadowed by $shadowing in ${shadowing.symbol.maybeOwner}") SearchFailure(generated1.withTypeUnchecked( diff --git a/tests/neg/implicit-shadowing.scala b/tests/pos/implicit-shadowing.scala similarity index 75% rename from tests/neg/implicit-shadowing.scala rename to tests/pos/implicit-shadowing.scala index 0ecba452841f..b5d032dafad6 100644 --- a/tests/neg/implicit-shadowing.scala +++ b/tests/pos/implicit-shadowing.scala @@ -4,8 +4,8 @@ object Test { def outer(implicit c: C) = { - def f(c: C) = implicitly[C] // error: shadowing - def g(c: Int) = implicitly[C] // error: shadowing (even though type is different) + def f(c: C) = implicitly[C] // now ok: shadowing no longer tested + def g(c: Int) = implicitly[C] // now ok: shadowing no longer tested f(new C) } diff --git a/tests/run/i5224.check b/tests/run/i5224.check index 3cb9bd81d1bd..23ea6edf2da1 100644 --- a/tests/run/i5224.check +++ b/tests/run/i5224.check @@ -1,2 +1,2 @@ barInt -bar +barInt diff --git a/tests/run/i5224.scala b/tests/run/i5224.scala index 9ac5ff23e2ab..d81bd63a8ead 100644 --- a/tests/run/i5224.scala +++ b/tests/run/i5224.scala @@ -16,5 +16,7 @@ object Test extends App { def barInt: Unit = ??? implicitly[Bar[Int]] + // used to resolve to bar, but + // resolves to barInt now, since shadowing is no longer tested } } \ No newline at end of file From 9b6becbc4e1e69d4eeae9c9fa7c7e1bfa3a35418 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 10 Feb 2019 11:49:36 +0100 Subject: [PATCH 4/6] Drop logic for shadowed implicits checking --- .../tools/dotc/printing/PlainPrinter.scala | 1 - .../dotty/tools/dotc/typer/Implicits.scala | 69 +++---------------- 2 files changed, 10 insertions(+), 60 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala b/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala index 93b690c5258c..909bf7a81a71 100644 --- a/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala +++ b/compiler/src/dotty/tools/dotc/printing/PlainPrinter.scala @@ -520,7 +520,6 @@ class PlainPrinter(_ctx: Context) extends Printer { result.reason match { case _: NoMatchingImplicits => "No Matching Implicit" case _: DivergingImplicit => "Diverging Implicit" - case _: ShadowedImplicit => "Shadowed Implicit" case result: AmbiguousImplicits => "Ambiguous Implicit: " ~ toText(result.alt1.ref) ~ " and " ~ toText(result.alt2.ref) case _ => diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index cfbb24316a11..d60a5bc42976 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -437,20 +437,6 @@ object Implicits { em"${err.refStr(ref)} does not $qualify" } - class ShadowedImplicit(ref: TermRef, - shadowing: Type, - val expectedType: Type, - val argument: Tree) extends SearchFailureType { - /** same as err.refStr but always prints owner even if it is a term */ - def show(ref: Type)(implicit ctx: Context): String = ref match { - case ref: NamedType if ref.symbol.maybeOwner.isTerm => - i"${ref.symbol} in ${ref.symbol.owner}" - case _ => err.refStr(ref) - } - def explanation(implicit ctx: Context): String = - em"${show(ref)} does $qualify but it is shadowed by ${show(shadowing)}" - } - class DivergingImplicit(ref: TermRef, val expectedType: Type, val argument: Tree) extends SearchFailureType { @@ -836,9 +822,6 @@ trait Implicits { self: Typer => shortForm case _ => arg.tpe match { - case tpe: ShadowedImplicit => - i"""$headline; - |${tpe.explanation}.""" case tpe: SearchFailureType => i"""$headline. |I found: @@ -1047,9 +1030,9 @@ trait Implicits { self: Typer => /** Try to typecheck an implicit reference */ def typedImplicit(cand: Candidate, contextual: Boolean)(implicit ctx: Context): SearchResult = track("typedImplicit") { trace(i"typed implicit ${cand.ref}, pt = $pt, implicitsEnabled == ${ctx.mode is ImplicitsEnabled}", implicits, show = true) { val ref = cand.ref - var generated: Tree = tpd.ref(ref).withSpan(span.startPos) + val generated: Tree = tpd.ref(ref).withSpan(span.startPos) val locked = ctx.typerState.ownedVars - val generated1 = + val adapted = if (argument.isEmpty) adapt(generated, pt, locked) else { @@ -1071,52 +1054,20 @@ trait Implicits { self: Typer => } else tryConversion } - lazy val shadowing = - typedUnadapted(untpd.Ident(cand.implicitRef.implicitName).withSpan(span.toSynthetic))( - nestedContext().addMode(Mode.ImplicitShadowing).setExploreTyperState()) - - /** Is candidate reference the same as the `shadowing` reference? (i.e. - * no actual shadowing occured). This is the case if the - * underlying symbol of the shadowing reference is the same as the - * symbol of the candidate reference, or if they have a common type owner. - * - * The second condition (same owner) is needed because the candidate reference - * and the potential shadowing reference are typechecked with different prototypes. - * so might yield different overloaded symbols. E.g. if the candidate reference - * is to an implicit conversion generated from an implicit class, the shadowing - * reference could go to the companion object of that class instead. - */ - def refSameAs(shadowing: Tree): Boolean = { - def symMatches(sym: Symbol): Boolean = - sym == ref.symbol || sym.owner.isType && sym.owner == ref.symbol.owner - def denotMatches(d: Denotation): Boolean = d match { - case d: SingleDenotation => symMatches(d.symbol) - case d => d.hasAltWith(denotMatches(_)) - } - denotMatches(closureBody(shadowing).denot) - } - if (ctx.reporter.hasErrors) { ctx.reporter.removeBufferedMessages SearchFailure { - generated1.tpe match { - case _: SearchFailureType => generated1 - case _ => generated1.withType(new MismatchedImplicit(ref, pt, argument)) + adapted.tpe match { + case _: SearchFailureType => adapted + case _ => adapted.withType(new MismatchedImplicit(ref, pt, argument)) } } } - else if (false && - contextual && !ctx.mode.is(Mode.ImplicitShadowing) && - !shadowing.tpe.isError && !refSameAs(shadowing)) { - implicits.println(i"SHADOWING $ref in ${ref.termSymbol.maybeOwner} is shadowed by $shadowing in ${shadowing.symbol.maybeOwner}") - SearchFailure(generated1.withTypeUnchecked( - new ShadowedImplicit(ref, methPart(shadowing).tpe, pt, argument))) - } else { - val generated2 = - if (cand.isExtension) Applications.ExtMethodApply(generated1).withType(generated1.tpe) - else generated1 - SearchSuccess(generated2, ref, cand.level)(ctx.typerState, ctx.gadt) + val returned = + if (cand.isExtension) Applications.ExtMethodApply(adapted).withType(adapted.tpe) + else adapted + SearchSuccess(returned, ref, cand.level)(ctx.typerState, ctx.gadt) } }} @@ -1326,7 +1277,7 @@ trait Implicits { self: Typer => case _: AmbiguousImplicits => failure2 case _ => reason match { - case (_: DivergingImplicit) | (_: ShadowedImplicit) => failure + case (_: DivergingImplicit) => failure case _ => List(failure, failure2).maxBy(_.tree.treeSize) } } From cd774b2cc64cfd9acda8a61a7e7b319eba68054f Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 10 Feb 2019 12:10:37 +0100 Subject: [PATCH 5/6] Drop ImplicitShadowing mode --- compiler/src/dotty/tools/dotc/core/Mode.scala | 5 ----- compiler/src/dotty/tools/dotc/typer/Typer.scala | 2 -- 2 files changed, 7 deletions(-) diff --git a/compiler/src/dotty/tools/dotc/core/Mode.scala b/compiler/src/dotty/tools/dotc/core/Mode.scala index 5afd3aa68b7e..1d794486aea1 100644 --- a/compiler/src/dotty/tools/dotc/core/Mode.scala +++ b/compiler/src/dotty/tools/dotc/core/Mode.scala @@ -60,11 +60,6 @@ object Mode { */ val Printing: Mode = newMode(10, "Printing") - /** We are currently typechecking an ident to determine whether some implicit - * is shadowed - don't do any other shadowing tests. - */ - val ImplicitShadowing: Mode = newMode(11, "ImplicitShadowing") - /** We are currently in a `viewExists` check. In that case, ambiguous * implicits checks are disabled and we succeed with the first implicit * found. diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 327c9756bc44..4eb5ceeb8d10 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -2025,7 +2025,6 @@ class Typer extends Namer val result = if (ifpt.exists && xtree.isTerm && !untpd.isContextualClosure(xtree) && - !ctx.mode.is(Mode.ImplicitShadowing) && !ctx.mode.is(Mode.Pattern) && !ctx.isAfterTyper) makeContextualFunction(xtree, ifpt) @@ -2594,7 +2593,6 @@ class Typer extends Namer !untpd.isContextualClosure(tree) && !isApplyProto(pt) && !ctx.mode.is(Mode.Pattern) && - !ctx.mode.is(Mode.ImplicitShadowing) && !ctx.isAfterTyper) { typr.println(i"insert apply on implicit $tree") typed(untpd.Select(untpd.TypedSplice(tree), nme.apply), pt, locked) From 4826b64cbc7e5687c6da9d8a9a3ee0119ec8f1ac Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Sun, 10 Feb 2019 12:57:37 +0100 Subject: [PATCH 6/6] Don't consider implicits in package prefixes Don't consider implicits in package prefixes to be in the implicit scope of a type unless under -language:Scala2. --- .../dotty/tools/dotc/typer/Implicits.scala | 45 ++++++++++--------- .../package-implicit/ActorRef.scala | 0 tests/neg/package-implicit/DataFlow.scala | 7 +++ .../package-implicit/package.scala | 0 tests/new/package-implicit/DataFlow.scala | 7 --- 5 files changed, 32 insertions(+), 27 deletions(-) rename tests/{new => neg}/package-implicit/ActorRef.scala (100%) create mode 100644 tests/neg/package-implicit/DataFlow.scala rename tests/{new => neg}/package-implicit/package.scala (100%) delete mode 100644 tests/new/package-implicit/DataFlow.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Implicits.scala b/compiler/src/dotty/tools/dotc/typer/Implicits.scala index d60a5bc42976..0b717bd60bde 100644 --- a/compiler/src/dotty/tools/dotc/typer/Implicits.scala +++ b/compiler/src/dotty/tools/dotc/typer/Implicits.scala @@ -520,27 +520,32 @@ trait ImplicitRunInfo { self: Run => val comps = new TermRefSet tp match { case tp: NamedType => - val pre = tp.prefix - comps ++= iscopeRefs(pre) - def addRef(companion: TermRef): Unit = { - val compSym = companion.symbol - if (compSym is Package) - addRef(companion.select(nme.PACKAGE)) - else if (compSym.exists) - comps += companion.asSeenFrom(pre, compSym.owner).asInstanceOf[TermRef] - } - def addCompanionOf(sym: Symbol) = { - val companion = sym.companionModule - if (companion.exists) addRef(companion.termRef) - } - def addClassScope(cls: ClassSymbol): Unit = { - addCompanionOf(cls) - for (parent <- cls.classParents; ref <- iscopeRefs(tp.baseType(parent.classSymbol))) - addRef(ref) + if (!tp.symbol.is(Package) || ctx.scala2Mode) { + // Don't consider implicits in package prefixes unless under -language:Scala2 + val pre = tp.prefix + comps ++= iscopeRefs(pre) + def addRef(companion: TermRef): Unit = { + val compSym = companion.symbol + if (compSym is Package) { + assert(ctx.scala2Mode) + addRef(companion.select(nme.PACKAGE)) + } + else if (compSym.exists) + comps += companion.asSeenFrom(pre, compSym.owner).asInstanceOf[TermRef] + } + def addCompanionOf(sym: Symbol) = { + val companion = sym.companionModule + if (companion.exists) addRef(companion.termRef) + } + def addClassScope(cls: ClassSymbol): Unit = { + addCompanionOf(cls) + for (parent <- cls.classParents; ref <- iscopeRefs(tp.baseType(parent.classSymbol))) + addRef(ref) + } + val underlyingTypeSym = tp.widen.typeSymbol + if (underlyingTypeSym.isOpaqueAlias) addCompanionOf(underlyingTypeSym) + else tp.classSymbols(liftingCtx).foreach(addClassScope) } - val underlyingTypeSym = tp.widen.typeSymbol - if (underlyingTypeSym.isOpaqueAlias) addCompanionOf(underlyingTypeSym) - else tp.classSymbols(liftingCtx).foreach(addClassScope) case _ => for (part <- tp.namedPartsWith(_.isType)) comps ++= iscopeRefs(part) } diff --git a/tests/new/package-implicit/ActorRef.scala b/tests/neg/package-implicit/ActorRef.scala similarity index 100% rename from tests/new/package-implicit/ActorRef.scala rename to tests/neg/package-implicit/ActorRef.scala diff --git a/tests/neg/package-implicit/DataFlow.scala b/tests/neg/package-implicit/DataFlow.scala new file mode 100644 index 000000000000..ba94d74e67e7 --- /dev/null +++ b/tests/neg/package-implicit/DataFlow.scala @@ -0,0 +1,7 @@ +package t1000647.bar + +import t1000647.foo.{ScalaActorRef} + +object DataFlow { + def foo(ref: ScalaActorRef) = ref.stop() // error: value stop is not a member of ... +} diff --git a/tests/new/package-implicit/package.scala b/tests/neg/package-implicit/package.scala similarity index 100% rename from tests/new/package-implicit/package.scala rename to tests/neg/package-implicit/package.scala diff --git a/tests/new/package-implicit/DataFlow.scala b/tests/new/package-implicit/DataFlow.scala deleted file mode 100644 index d948280d0d4b..000000000000 --- a/tests/new/package-implicit/DataFlow.scala +++ /dev/null @@ -1,7 +0,0 @@ -package t1000647.bar - -import t1000647.foo.{ScalaActorRef} - -object DataFlow { - def foo(ref: ScalaActorRef) = ref.stop() -}