Skip to content

Review/5108 #20

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Review/5108 #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e94b4b
Add support for multiple trailing implicit parameter lists.
milessabin Apr 20, 2016
965dfff
Flag usages of MethodType#isImplicit to check for broken assumptions
retronym Apr 23, 2016
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: 1 addition & 1 deletion src/compiler/scala/tools/nsc/ast/parser/Parsers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2234,7 +2234,7 @@ self =>
if (ofCaseClass && in.token != LPAREN)
syntaxError(in.lastOffset, "case classes without a parameter list are not allowed;\n"+
"use either case objects or case classes with an explicit `()' as a parameter list.")
while (implicitmod == 0 && in.token == LPAREN) {
while (in.token == LPAREN) {
in.nextToken()
vds += paramClause()
accept(RPAREN)
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/DestructureTypes.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ trait DestructureTypes {
else node("assocs", nodeList(names.indices.toList, (i: Int) => atom(names(i).toString, args(i))))
}
private def typeTypeName(tp: Type) = tp match {
case mt @ MethodType(_, _) if mt.isImplicit => "ImplicitMethodType"
case mt @ MethodType(_, _) if mt.isImplicit => "ImplicitMethodType" //
case TypeRef(_, sym, _) => typeRefType(sym)
case _ => tp.kind
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/EtaExpansion.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ trait EtaExpansion { self: Analyzer =>

/* Eta-expand lifted tree. */
def expand(tree: Tree, tpe: Type): Tree = tpe match {
case mt @ MethodType(paramSyms, restpe) if !mt.isImplicit =>
case mt @ MethodType(paramSyms, restpe) if !mt.isImplicit => //
Copy link
Owner Author

Choose a reason for hiding this comment

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

looks okay, could do with a test:

scala> {def bar(a: Int)(implicit b: DummyImplicit)(implicit c: DummyImplicit) = 0; bar _} //print

{
  def bar(a: scala.Int)(implicit b: scala.Predef.DummyImplicit)(implicit c: scala.Predef.DummyImplicit) = 0;
  {
    ((a: Int) => bar(a)(Predef.this.DummyImplicit.dummyImplicit)(Predef.this.DummyImplicit.dummyImplicit))
  }
} // : Int => Int

val params: List[(ValDef, Boolean)] = paramSyms.map {
sym =>
val origTpe = sym.tpe
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/Implicits.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ trait Implicits {
private def checkCompatibility(fast: Boolean, tp0: Type, pt0: Type): Boolean = {
@tailrec def loop(tp: Type, pt: Type): Boolean = tp match {
case mt @ MethodType(params, restpe) =>
if (mt.isImplicit)
if (mt.isImplicit) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

The crux: this will now unwrap all implicit method types to get to the result type, so the following works:

scala> {class A; implicit def bar(implicit b: DummyImplicit)(implicit c: DummyImplicit): A = new A; implicitly[A]} //print

{
  class A;
  implicit def bar(implicit b: scala.Predef.DummyImplicit)(implicit c: scala.Predef.DummyImplicit): A = new A();
  scala.Predef.implicitly[A](bar(Predef.this.DummyImplicit.dummyImplicit)(Predef.this.DummyImplicit.dummyImplicit))
} // : A

loop(restpe, pt)
else pt match {
case tr @ TypeRef(pre, sym, args) =>
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/scala/tools/nsc/typechecker/Infer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ trait Infer extends Checkable {
}

def skipImplicit(tp: Type) = tp match {
case mt: MethodType if mt.isImplicit => mt.resultType
case mt: MethodType if mt.isImplicit => mt.resultType //
Copy link
Owner Author

Choose a reason for hiding this comment

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

  • This looks like a potential bug with more than one implicit param list

skipImplicit is called from this error message:

scala> def foo[A](implicit a: DummyImplicit): Array[A] = null
foo: [A](implicit a: DummyImplicit)Array[A]

scala> foo: Seq[_]
<console>:18: error: polymorphic expression cannot be instantiated to expected type;
 found   : [A]Array[A]
 required: Seq[_]
       foo: Seq[_]
       ^

scala> def foo[A](implicit a: DummyImplicit)(implicit b: DummyImplicit): Array[A] = null
foo: [A](implicit a: DummyImplicit)(implicit b: DummyImplicit)Array[A]

scala> foo: Seq[_]
<console>:18: error: polymorphic expression cannot be instantiated to expected type;
 found   : [A](implicit b: DummyImplicit)Array[A]
 required: Seq[_]
       foo: Seq[_]
       ^

It is also called from Infer::inferForApproxPt as follows (in the context of inferring type paramsters for constructor patterns for case classes:

val resTpInst  = skipImplicit(ctorTpInst.finalResultType)

However, AFAICT the call to finalResultType would peel off all MethodTypes rendering skipImplicit a redundant no-op.

Copy link

@milessabin milessabin Apr 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Can you explain the issue here a little more for me? If I try,

def foo[A](a: DummyImplicit)(implicit b: DummyImplicit): Array[A] = null
foo: Seq[_]

(ie. your second example but with the first parameter list not implicit) I see the exact same error as with both parameter lists implicit,

multi-implicits.scala:81: error: polymorphic expression cannot be instantiated to expected type;
 found   : [A](a: DummyImplicit)(implicit b: DummyImplicit)Array[A]
 required: Seq[_]
    foo: Seq[_]
    ^

Are you saying the the call to skipImplicit should just be removed? And instead we should just have,

val resTpInst  = ctorTpInst.finalResultType

Copy link
Owner Author

@retronym retronym Apr 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, my reading of this code is that skipImplicit is a no-op, because finalResultType already strips all MethodTypes.

Looks like finalResultType was introduced in scala@b908232#diff-62ccdfe29e58b8e9f06ed66d4cfc88a5L1086 / scala@b908232#diff-62ccdfe29e58b8e9f06ed66d4cfc88a5R1092, so prior to that change skipImplicit was needed.

case _ => tp
}

Expand All @@ -163,7 +163,7 @@ trait Infer extends Checkable {
logResult(sm"""|Normalizing PolyType in infer:
| was: $restpe
| now""")(normalize(restpe))
case mt @ MethodType(_, restpe) if mt.isImplicit => normalize(restpe)
case mt @ MethodType(_, restpe) if mt.isImplicit => normalize(restpe) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM, will recurse to strip off all implicit param lists.

case mt @ MethodType(_, restpe) if !mt.isDependentMethodType =>
if (phase.erasedTypes) FunctionClass(mt.params.length).tpe
else functionType(mt.paramTypes, normalize(restpe))
Expand Down Expand Up @@ -366,7 +366,7 @@ trait Infer extends Checkable {
// optimize type variables wrt to the implicit formals only; ignore the result type.
// See test pos/jesper.scala
def variance = restpe match {
case mt: MethodType if mt.isImplicit && isFullyDefined(pt) => MethodType(mt.params, AnyTpe)
case mt: MethodType if mt.isImplicit && isFullyDefined(pt) => MethodType(mt.params, AnyTpe) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM, we'll ignore all implicit param lists, rather than the only one, when approximating the type used to calculate the variance (and direction of solving) of the type params

case _ => restpe
}
def solve() = solvedTypes(tvars, tparams, tparams map varianceInType(variance), upper = false, lubDepth(restpe :: pt :: Nil))
Expand Down Expand Up @@ -811,21 +811,21 @@ trait Infer extends Checkable {
def onRight = ftpe2 match {
case OverloadedType(pre, alts) => alts forall (alt => isAsSpecific(ftpe1, pre memberType alt))
case et: ExistentialType => et.withTypeVars(isAsSpecific(ftpe1, _))
case mt @ MethodType(_, restpe) => !mt.isImplicit || isAsSpecific(ftpe1, restpe)
case mt @ MethodType(_, restpe) => !mt.isImplicit || isAsSpecific(ftpe1, restpe) //
case NullaryMethodType(res) => isAsSpecific(ftpe1, res)
case PolyType(tparams, NullaryMethodType(restpe)) => isAsSpecific(ftpe1, PolyType(tparams, restpe))
case PolyType(tparams, mt @ MethodType(_, restpe)) => !mt.isImplicit || isAsSpecific(ftpe1, PolyType(tparams, restpe))
case PolyType(tparams, mt @ MethodType(_, restpe)) => !mt.isImplicit || isAsSpecific(ftpe1, PolyType(tparams, restpe)) //
case _ => isAsSpecificValueType(ftpe1, ftpe2, Nil, Nil)
}
ftpe1 match {
case OverloadedType(pre, alts) => alts exists (alt => isAsSpecific(pre memberType alt, ftpe2))
case et: ExistentialType => isAsSpecific(et.skolemizeExistential, ftpe2)
case NullaryMethodType(restpe) => isAsSpecific(restpe, ftpe2)
case mt @ MethodType(_, restpe) if mt.isImplicit => isAsSpecific(restpe, ftpe2)
case mt @ MethodType(_, restpe) if mt.isImplicit => isAsSpecific(restpe, ftpe2) //
case mt @ MethodType(_, _) if bothAreVarargs => checkIsApplicable(mt.paramTypes mapConserve repeatedToSingle)
case mt @ MethodType(params, _) if params.nonEmpty => checkIsApplicable(mt.paramTypes)
case PolyType(tparams, NullaryMethodType(restpe)) => isAsSpecific(PolyType(tparams, restpe), ftpe2)
case PolyType(tparams, mt @ MethodType(_, restpe)) if mt.isImplicit => isAsSpecific(PolyType(tparams, restpe), ftpe2)
case PolyType(tparams, mt @ MethodType(_, restpe)) if mt.isImplicit => isAsSpecific(PolyType(tparams, restpe), ftpe2) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

  • This looks like it needs to be updated to;
case mt @ MethodType(_, _) if mt.isImplicit                    => isAsSpecific(skipImplicit(mt), ftpe2)
...
case PolyType(tparams, mt @ MethodType(_, _)) if mt.isImplicit => isAsSpecific(PolyType(tparams, skipImplicit(mt)), ftpe2)

With a test to show that overload resolution behaves the same for 1 and N implicit param lists (ie, it ignores the implicit params)

Copy link

Choose a reason for hiding this comment

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

Can you say a little more about the overloading scenario you would expect to distinguish these two implementations? I've tried the following,

bject Overload1 {                                                                                                                    
  def over(i: Int)(implicit d: DummyImplicit): Int = i
  def over(b: Boolean)(implicit d: DummyImplicit): Boolean = b

  (over(23): Int)
  (over(true): Boolean)
}

object Overload2 {
  def over(i: Int)(implicit d: DummyImplicit)(implicit e: DummyImplicit): Int = i
  def over(b: Boolean)(implicit d: DummyImplicit)(implicit e: DummyImplicit): Boolean = b

  (over(23): Int)
  (over(true): Boolean)
}

and both appear to compile the same way with both implementations.

case PolyType(_, mt @ MethodType(params, _)) if params.nonEmpty => checkIsApplicable(mt.paramTypes)
case ErrorType => true
case _ => onRight
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/scala/tools/nsc/typechecker/Typers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
notifyUndetparamsAdded(tparams1)
adapt(tree1 setType restpe.substSym(tparams, tparams1), mode, pt, original)

case mt: MethodType if mode.typingExprNotFunNotLhs && mt.isImplicit => // (4.1)
case mt: MethodType if mode.typingExprNotFunNotLhs && mt.isImplicit => // (4.1) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM, adaptation should happen one param lists at a time.

adaptToImplicitMethod(mt)
case mt: MethodType if mode.typingExprNotFunNotLhs && !hasUndetsInMonoMode && !treeInfo.isMacroApplicationOrBlock(tree) =>
instantiateToMethodType(mt)
Expand Down
2 changes: 1 addition & 1 deletion src/interactive/scala/tools/nsc/interactive/Global.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1079,7 +1079,7 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "")
val context = doLocateContext(pos)
val shouldTypeQualifier = tree0.tpe match {
case null => true
case mt: MethodType => mt.isImplicit
case mt: MethodType => mt.isImplicit //
Copy link
Owner Author

Choose a reason for hiding this comment

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

Not sure, probably okay.

case _ => false
}

Expand Down
2 changes: 1 addition & 1 deletion src/reflect/scala/reflect/internal/TreeInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ abstract class TreeInfo {
def mayBeVarGetter(sym: Symbol): Boolean = sym.info match {
case NullaryMethodType(_) => sym.owner.isClass && !sym.isStable
case PolyType(_, NullaryMethodType(_)) => sym.owner.isClass && !sym.isStable
case mt @ MethodType(_, _) => mt.isImplicit && sym.owner.isClass && !sym.isStable
case mt @ MethodType(_, _) => mt.isImplicit && sym.owner.isClass && !sym.isStable //
case _ => false
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Okay, here's a test case:

scala> object O { def a(implicit I: DummyImplicit) = 42; def a_=(a: Any) = () }; O.a = 0
defined object O
O.a: Int = 42

scala> object O { def a(implicit I: DummyImplicit)(implicit J: DummyImplicit) = 42; def a_=(a: Any) = () }; O.a = 0
defined object O
O.a: Int = 42


Expand Down
4 changes: 2 additions & 2 deletions src/reflect/scala/reflect/internal/Types.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4025,7 +4025,7 @@ trait Types
}

def isImplicitMethodType(tp: Type) = tp match {
case mt: MethodType => mt.isImplicit
case mt: MethodType => mt.isImplicit //
case _ => false
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM


Expand Down Expand Up @@ -4219,7 +4219,7 @@ trait Types
tp2 match {
case mt2 @ MethodType(params2, res2) =>
// sameLength(params1, params2) was used directly as pre-screening optimization (now done by matchesQuantified -- is that ok, performancewise?)
mt1.isImplicit == mt2.isImplicit &&
mt1.isImplicit == mt2.isImplicit && //
Copy link
Owner Author

Choose a reason for hiding this comment

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

The existing implementation looks suspicious to me, it allows one to override a method without the override modifier:

scala> class C[T >: Null] { def a(implicit i: T = null): Any = "C"}; class S extends C[String] { def a(i: String) = "S" }
defined class C
defined class S

scala> (new S() : C[String]).a()
res8: Any = S

My intuition is that matchesType should ignore implicit modifers when determining overriding relationships, and a separate check in refchecks should enforce that the overriding method has the same implicit modiifier (or modifiers!) as the overriden one.

matchingParams(params1, params2, mt1.isJava, mt2.isJava) &&
matchesQuantified(params1, params2, res1, res2)
case NullaryMethodType(res2) =>
Expand Down
4 changes: 2 additions & 2 deletions src/reflect/scala/reflect/internal/tpe/TypeComparers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ trait TypeComparers {
private def isSameMethodType(mt1: MethodType, mt2: MethodType) = (
isSameTypes(mt1.paramTypes, mt2.paramTypes)
&& (mt1.resultType =:= mt2.resultType.substSym(mt2.params, mt1.params))
&& (mt1.isImplicit == mt2.isImplicit)
&& (mt1.isImplicit == mt2.isImplicit) //
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM, this will recurse as needed.

)

private def equalTypeParamsAndResult(tparams1: List[Symbol], res1: Type, tparams2: List[Symbol], res2: Type) = {
Expand Down Expand Up @@ -489,7 +489,7 @@ trait TypeComparers {
val params2 = mt2.params
val res2 = mt2.resultType
(sameLength(params1, params2) &&
mt1.isImplicit == mt2.isImplicit &&
mt1.isImplicit == mt2.isImplicit && //
Copy link
Owner Author

Choose a reason for hiding this comment

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

LGTM

scala> object O { def a(implicit I: DummyImplicit)(implicit J: DummyImplicit) = 42};
defined object O

scala> object P { def a(implicit I: DummyImplicit)(implicit J: DummyImplicit) = 42};
defined object P

scala> object Q { def a(implicit I: DummyImplicit)(implicit J: Int) = 42};
defined object Q

scala> val List(mt1, mt2, mt3) = List(typeOf[O.type], typeOf[P.type], typeOf[Q.type]).map(_.member(TermName("a")).info)
mt1: $r.intp.global.Type = (implicit I: DummyImplicit)(implicit J: DummyImplicit)Int
mt2: $r.intp.global.Type = (implicit I: DummyImplicit)(implicit J: DummyImplicit)Int
mt3: $r.intp.global.Type = (implicit I: DummyImplicit)(implicit J: Int)Int

scala> mt2 =:= mt1
res16: Boolean = true

scala> mt2 =:= mt3
res17: Boolean = false

scala> mt2 <:< mt1
res18: Boolean = true

scala> mt2 <:< mt3
res19: Boolean = false

matchingParams(params1, params2, mt1.isJava, mt2.isJava) &&
isSubType(res1.substSym(params1, params2), res2, depth))
// TODO: if mt1.params.isEmpty, consider NullaryMethodType?
Expand Down
29 changes: 29 additions & 0 deletions test/files/pos/multi-implicits.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package test

object Test {
// A couple of type classes with type members ...
trait Foo[T] {
type A
}

object Foo {
implicit val fooIS = new Foo[Int] { type A = String }
}

trait Bar[T] {
type B
val value: B
}

object Bar {
implicit val barSB = new Bar[String] {
type B = Boolean
val value = true
}
}

def run[T](t: T)(implicit foo: Foo[T])(bar: Bar[foo.A]): bar.B = bar.value

val value = run(23)
assert(value: Boolean)
}