Skip to content

Make sure arguments are evaluated in the correct typer state. #1460

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 5 commits into from
Aug 26, 2016
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
21 changes: 21 additions & 0 deletions src/dotty/tools/dotc/core/TyperState.scala
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@ class TyperState(r: Reporter) extends DotClass with Showable {
/** Commit state so that it gets propagated to enclosing context */
def commit()(implicit ctx: Context): Unit = unsupported("commit")

/** The typer state has already been committed */
def isCommitted: Boolean = false

/** Optionally, if this is a mutable typerstate, it's creator state */
def parent: Option[TyperState] = None

/** The closest ancestor of this typer state (including possibly this typer state itself)
* which is not yet committed, or which does not have a parent.
*/
def uncommittedAncestor: TyperState =
Copy link
Contributor

Choose a reason for hiding this comment

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

proposal: instead of storing the direct parent, lazily-cache uncommittedAncestor and test-update it every time it's updated:

private var lastUncommittedAncestor = parent

def uncommittedAncestor: TyperState = {
  if (lastUncommittedAncestor.isCommited && lastUncommittedAncestor.isDefined) { 
    lastUncommittedAncestor = lastUncommittedAncestor.lastUncommittedAncestor
  }
  lastUncommittedAncestor
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll roll that into the next PR.

if (!isCommitted || !parent.isDefined) this
else parent.get.uncommittedAncestor

/** Make type variable instances permanent by assigning to `inst` field if
* type variable instantiation cannot be retracted anymore. Then, remove
* no-longer needed constraint entries.
Expand Down Expand Up @@ -115,6 +128,7 @@ extends TyperState(r) {
*/
override def commit()(implicit ctx: Context) = {
val targetState = ctx.typerState
assert(targetState eq previous)
assert(isCommittable)
targetState.constraint = constraint
constraint foreachTypeVar { tvar =>
Expand All @@ -124,8 +138,15 @@ extends TyperState(r) {
targetState.ephemeral = ephemeral
targetState.gc()
reporter.flush()
myIsCommitted = true
}

private var myIsCommitted = false

override def isCommitted: Boolean = myIsCommitted

override def parent = Some(previous)

override def gc()(implicit ctx: Context): Unit = {
val toCollect = new mutable.ListBuffer[GenericType]
constraint foreachTypeVar { tvar =>
Expand Down
11 changes: 6 additions & 5 deletions src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ object Types {
}

/** Is some part of this type produced as a repair for an error? */
final def isErroneous(implicit ctx: Context): Boolean = existsPart(_.isError)
final def isErroneous(implicit ctx: Context): Boolean = existsPart(_.isError, forceLazy = false)

/** Does the type carry an annotation that is an instance of `cls`? */
final def hasAnnotation(cls: ClassSymbol)(implicit ctx: Context): Boolean = stripTypeVar match {
Expand Down Expand Up @@ -219,8 +219,8 @@ object Types {

/** Returns true if there is a part of this type that satisfies predicate `p`.
*/
final def existsPart(p: Type => Boolean)(implicit ctx: Context): Boolean =
new ExistsAccumulator(p).apply(false, this)
final def existsPart(p: Type => Boolean, forceLazy: Boolean = true)(implicit ctx: Context): Boolean =
new ExistsAccumulator(p, forceLazy).apply(false, this)

/** Returns true if all parts of this type satisfy predicate `p`.
*/
Expand Down Expand Up @@ -3688,9 +3688,10 @@ object Types {
protected def traverseChildren(tp: Type) = foldOver((), tp)
}

class ExistsAccumulator(p: Type => Boolean)(implicit ctx: Context) extends TypeAccumulator[Boolean] {
class ExistsAccumulator(p: Type => Boolean, forceLazy: Boolean = true)(implicit ctx: Context) extends TypeAccumulator[Boolean] {
override def stopAtStatic = false
def apply(x: Boolean, tp: Type) = x || p(tp) || foldOver(x, tp)
def apply(x: Boolean, tp: Type) =
x || p(tp) || (forceLazy || !tp.isInstanceOf[LazyRef]) && foldOver(x, tp)
}

class ForeachAccumulator(p: Type => Unit)(implicit ctx: Context) extends TypeAccumulator[Unit] {
Expand Down
66 changes: 39 additions & 27 deletions src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -541,24 +541,13 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>

def typedApply(tree: untpd.Apply, pt: Type)(implicit ctx: Context): Tree = {

/** Try same application with an implicit inserted around the qualifier of the function
* part. Return an optional value to indicate success.
*/
def tryWithImplicitOnQualifier(fun1: Tree, proto: FunProto)(implicit ctx: Context): Option[Tree] =
tryInsertImplicitOnQualifier(fun1, proto) flatMap { fun2 =>
tryEither { implicit ctx =>
Some(typedApply(
cpy.Apply(tree)(untpd.TypedSplice(fun2), proto.typedArgs map untpd.TypedSplice),
pt)): Option[Tree]
} { (_, _) => None }
}

def realApply(implicit ctx: Context): Tree = track("realApply") {
val originalProto = new FunProto(tree.args, IgnoredProto(pt), this)(argCtx(tree))
val fun1 = typedExpr(tree.fun, originalProto)

// Warning: The following lines are dirty and fragile. We record that auto-tupling was demanded as
// a side effect in adapt. If it was, we assume the tupled proto-type in the rest of the application.
// a side effect in adapt. If it was, we assume the tupled proto-type in the rest of the application,
// until, possibly, we have to fall back to insert an implicit on the qualifier.
// This crucially relies on he fact that `proto` is used only in a single call of `adapt`,
// otherwise we would get possible cross-talk between different `adapt` calls using the same
// prototype. A cleaner alternative would be to return a modified prototype from `adapt` together with
Expand All @@ -574,6 +563,32 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
if (!constrainResult(fun1.tpe.widen, proto.derivedFunProto(resultType = pt)))
typr.println(i"result failure for $tree with type ${fun1.tpe.widen}, expected = $pt")

/** Type application where arguments come from prototype, and no implicits are inserted */
def simpleApply(fun1: Tree, proto: FunProto)(implicit ctx: Context): Tree =
methPart(fun1).tpe match {
case funRef: TermRef =>
val app =
if (proto.allArgTypesAreCurrent())
new ApplyToTyped(tree, fun1, funRef, proto.typedArgs, pt)
else
new ApplyToUntyped(tree, fun1, funRef, proto, pt)(argCtx(tree))
convertNewGenericArray(ConstFold(app.result))
Copy link
Member

Choose a reason for hiding this comment

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

Some of this code is duplicated from typedApply and this line is especially non-obvious, it would be better to have simpleApply and typedApply call into a common method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, the code was moved out of typedApply, not duplicated.

case _ =>
handleUnexpectedFunType(tree, fun1)
}

/** Try same application with an implicit inserted around the qualifier of the function
* part. Return an optional value to indicate success.
*/
def tryWithImplicitOnQualifier(fun1: Tree, proto: FunProto)(implicit ctx: Context): Option[Tree] =
tryInsertImplicitOnQualifier(fun1, proto) flatMap { fun2 =>
tryEither {
implicit ctx => Some(simpleApply(fun2, proto)): Option[Tree]
} {
(_, _) => None
}
}

fun1.tpe match {
case ErrorType => tree.withType(ErrorType)
case TryDynamicCallType =>
Expand All @@ -583,23 +598,20 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
case _ =>
handleUnexpectedFunType(tree, fun1)
}
case _ => methPart(fun1).tpe match {
case funRef: TermRef =>
tryEither { implicit ctx =>
val app =
if (proto.argsAreTyped) new ApplyToTyped(tree, fun1, funRef, proto.typedArgs, pt)
else new ApplyToUntyped(tree, fun1, funRef, proto, pt)(argCtx(tree))
val result = app.result
convertNewGenericArray(ConstFold(result))
} { (failedVal, failedState) =>
case _ =>
tryEither {
implicit ctx => simpleApply(fun1, proto)
} {
(failedVal, failedState) =>
def fail = { failedState.commit(); failedVal }
// Try once with original prototype and once (if different) with tupled one.
// The reason we need to try both is that the decision whether to use tupled
// or not was already taken but might have to be revised when an implicit
// is inserted on the qualifier.
tryWithImplicitOnQualifier(fun1, originalProto).getOrElse(
if (proto eq originalProto) fail
else tryWithImplicitOnQualifier(fun1, proto).getOrElse(fail))
}
case _ =>
handleUnexpectedFunType(tree, fun1)
}
}
}
}

Expand All @@ -611,7 +623,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
*
* { val xs = es; e' = e' + args }
*/
def typedOpAssign: Tree = track("typedOpAssign") {
def typedOpAssign: Tree = track("typedOpAssign") {
val Apply(Select(lhs, name), rhss) = tree
val lhs1 = typedExpr(lhs)
val liftedDefs = new mutable.ListBuffer[Tree]
Expand Down
47 changes: 35 additions & 12 deletions src/dotty/tools/dotc/typer/ProtoTypes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -172,36 +172,60 @@ object ProtoTypes {
/** A map in which typed arguments can be stored to be later integrated in `typedArgs`. */
private var myTypedArg: SimpleMap[untpd.Tree, Tree] = SimpleMap.Empty

/** A map recording the typer states in which arguments stored in myTypedArg were typed */
private var evalState: SimpleMap[untpd.Tree, TyperState] = SimpleMap.Empty

def isMatchedBy(tp: Type)(implicit ctx: Context) =
typer.isApplicable(tp, Nil, typedArgs, resultType)

def derivedFunProto(args: List[untpd.Tree] = this.args, resultType: Type, typer: Typer = this.typer) =
if ((args eq this.args) && (resultType eq this.resultType) && (typer eq this.typer)) this
else new FunProto(args, resultType, typer)

def argsAreTyped: Boolean = myTypedArgs.size == args.length
/** Forget the types of any arguments that have been typed producing a constraint in a
* typer state that is not yet committed into the one of the current context `ctx`.
* This is necessary to avoid "orphan" PolyParams that are referred to from
* type variables in the typed arguments, but that are not registered in the
* current constraint. A test case is pos/t1756.scala.
* @return True if all arguments have types (in particular, no types were forgotten).
*/
def allArgTypesAreCurrent()(implicit ctx: Context): Boolean = {
evalState foreachBinding { (arg, tstate) =>
if (tstate.uncommittedAncestor.constraint ne ctx.typerState.constraint) {
typr.println(i"need to invalidate $arg / ${myTypedArg(arg)}, ${tstate.constraint}, current = ${ctx.typerState.constraint}")
myTypedArg = myTypedArg.remove(arg)
evalState = evalState.remove(arg)
}
}
myTypedArg.size == args.length
}

private def cacheTypedArg(arg: untpd.Tree, typerFn: untpd.Tree => Tree)(implicit ctx: Context): Tree = {
var targ = myTypedArg(arg)
if (targ == null) {
targ = typerFn(arg)
if (!ctx.reporter.hasPending) {
myTypedArg = myTypedArg.updated(arg, targ)
evalState = evalState.updated(arg, ctx.typerState)
}
}
targ
}

/** The typed arguments. This takes any arguments already typed using
* `typedArg` into account.
*/
def typedArgs: List[Tree] = {
if (!argsAreTyped)
myTypedArgs = args mapconserve { arg =>
val targ = myTypedArg(arg)
if (targ != null) targ else typer.typed(arg)
}
if (myTypedArgs.size != args.length)
myTypedArgs = args.mapconserve(cacheTypedArg(_, typer.typed(_)))
myTypedArgs
}

/** Type single argument and remember the unadapted result in `myTypedArg`.
* used to avoid repeated typings of trees when backtracking.
*/
def typedArg(arg: untpd.Tree, formal: Type)(implicit ctx: Context): Tree = {
var targ = myTypedArg(arg)
if (targ == null) {
targ = typer.typedUnadapted(arg, formal)
if (!ctx.reporter.hasPending) myTypedArg = myTypedArg.updated(arg, targ)
}
val targ = cacheTypedArg(arg, typer.typedUnadapted(_, formal))
typer.adapt(targ, formal, arg)
}

Expand Down Expand Up @@ -237,7 +261,6 @@ object ProtoTypes {
*/
class FunProtoTyped(args: List[tpd.Tree], resultType: Type, typer: Typer)(implicit ctx: Context) extends FunProto(args, resultType, typer)(ctx) {
override def typedArgs = args
override def argsAreTyped = true
}

/** A prototype for implicitly inferred views:
Expand Down
9 changes: 3 additions & 6 deletions src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1445,11 +1445,8 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
val sel = typedSelect(untpd.Select(untpd.TypedSplice(tree), nme.apply), pt)
if (sel.tpe.isError) sel else adapt(sel, pt)
} { (failedTree, failedState) =>
tryInsertImplicitOnQualifier(tree, pt) match {
case Some(tree1) => adapt(tree1, pt)
case none => fallBack(failedTree, failedState)
}
}
tryInsertImplicitOnQualifier(tree, pt).getOrElse(fallBack(failedTree, failedState))
}

/** If this tree is a select node `qual.name`, try to insert an implicit conversion
* `c` around `qual` so that `c(qual).name` conforms to `pt`. If that fails
Expand All @@ -1462,7 +1459,7 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
tryEither { implicit ctx =>
val qual1 = adaptInterpolated(qual, qualProto, EmptyTree)
if ((qual eq qual1) || ctx.reporter.hasErrors) None
else Some(typedSelect(cpy.Select(tree)(untpd.TypedSplice(qual1), name), pt))
else Some(typed(cpy.Select(tree)(untpd.TypedSplice(qual1), name), pt))
} { (_, _) => None
}
case _ => None
Expand Down
59 changes: 0 additions & 59 deletions tests/pending/pos/t1756.scala

This file was deleted.

33 changes: 33 additions & 0 deletions tests/pos/t1756.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
trait Ring[T <: Ring[T]] {
def +(that: T): T
def *(that: T): T
}

class A extends Ring[A] {
def +(that: A) = new A
def *(that: A) = new A
}

class Poly[C <: Ring[C]](val c: C) extends Ring[Poly[C]] {
def +(that: Poly[C]) = new Poly(this.c + that.c)
def *(that: Poly[C]) = new Poly(this.c*that.c)
}

object Test extends App {

implicit def coef2poly[CI <: Ring[CI]](c: CI): Poly[CI] = new Poly(c)

val a = new A
val x = new Poly(new A)

println(x + a) // works
println(a + x) // works

val y = new Poly(new Poly(new A))

println(x + y*x) // works
println(x*y + x) // works
println(y*x + x) // works

println(x + x*y) // failed before, first with type error, after that was fixed with "orphan poly parameter CI".
}