Skip to content

Change handling of tvars in implicits #752

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

Closed
wants to merge 15 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 9, 2015

Fixes #739 by adding the following rule:

Before typing an implicit parameter list of a method m, instantiate all type parameters of m that occur in the type of some preceding value parameter of m.

Review by @smarter, @retronym

odersky added 9 commits August 4, 2015 18:34
Non-local returns are now implemented.
Disable the check because if fails for desugar.scala and also in some dotty files.
This test failed before the addition of NonLocalReturns.
After erasure, if required conformance is between value and non-value types,
one should perform boxing and unboxing operations automatically, instead of
just issuing a cast, which would be illegal at that point.

Also: make isNonLocalReturn available as part of a global object, because
we'll need it in LiftTry.
Phase lifts tries that would be illegal because they execute on non-empty
expression stacks.
by adding the following rule:

Before typing an implicit parameter list of a method m, instantiate all type parameters of m that occur in the type of some preceding value parameter of m.
@odersky
Copy link
Contributor Author

odersky commented Aug 9, 2015

Only the last two commits are new. Everything else is merged by now.

odersky added 2 commits August 9, 2015 15:22
(scalac and dotty both produce an error here)
@smarter
Copy link
Member

smarter commented Aug 10, 2015

This new scheme seems OK to me but it does not address the issues we have regarding when a type parameter should be minimized or maximized, specifically #738 and #718. It'd be great to see at least #738 addressed to get a better idea of the semantics we're trying to achieve. (Note that both of these issues should be fixed in my prototype)

We now also consider type variables in a selection prefix of the
application. The test case was augmented to include a snippet which
only succeeds under the generalization.
Fixes scala#738.

The previous scheme interpolated type variables according to the
full variance of the result type. This means that the interpolation direction
is downwards if a type varaible occurs like this:

   (implicit x: T)T

but it is upwards if it occurs like this

   (implicit x: T)Unit

For a normal method this makes sense, since the upper bound of T gives
you the most general function. But if the method is implicit, it makes
no sense, since upward instantiation means that we likely get ambiguity
errors.

Interestingly the fix causes a different problem (exemplified by failures
in run/stream-stack-overflow.scala, and also in dotc itself), which will
be fixed in the next commit.
…nwards.

With the previous commit, we had an interesting failure in relation to CanBuildFrom.

Say we have

  expr: (implicit cbf: CanBuildFrom[Repr, Elem, That]): That

for some type vars Repr >: R, Elem >: R, That. CanBuildFrom is defined as

  trait CanBuildFrom[-Repr, -Elem, +That]

Since Repr and Elem appear doubly contravariantly - that is covariantly - in

   (implicit cbf: CanBuildFrom[Repr, Elem, That]): That

it means they are minimized. With the change of the last commit, implicit
parameters are ignored for interpolation, so Repr, and Elem are not interpolated.
But they will be instantiated in `adaptNoArgs` as type variables occurring in an
implicit type. Only now the instantiation is according to the variance of the
argument, so the type variables are maximized to Any!

The same problem would have occurred earlier, if `Repr` or `Elem` would be part
of the result type of a method, because then they would also have to be maxmimized.
But by soem coincidence that code never was seen in the wild in the collection framework.

The fix minimizes all type variables in formal implicit parameters, independently
of their variance in the parameter type. This follows a suggestion of @extempore
a while ago. There's another half to it that still needs to be implemented: when
comparing multiple implicits for which one is best, we also need to ignore variance.
@smarter
Copy link
Member

smarter commented Aug 10, 2015

I'm still confused by the rules regarding type parameter maximization, for example:

import scala.reflect._

class Contra[-T]

object Test {
  def getParam[T](c: Contra[T])(implicit ct: ClassTag[T]): Unit = {
    println(ct)
  }

  def main(args: Array[String]): Unit = {
    getParam(new Contra[Int])
  }
}

This will print Int when compiled with scalac, but it will print Nothing when compiled with dotty, is that what we want?

@odersky
Copy link
Contributor Author

odersky commented Aug 11, 2015

On Mon, Aug 10, 2015 at 4:44 PM, Guillaume Martres <notifications@github.com

wrote:

I'm still confused by the rules regarding type parameter maximization, for
example:

import scala.reflect._
class Contra[-T]
object Test {
def getParam[T](c: Contra[T])(implicit ct: ClassTag[T]): Unit = {
println(ct)
}

def main(args: Array[String]): Unit = {
getParam(new Contra[Int])
}
}

This will print Int when compiled with scalac, but it will print Nothing
when compiled with dotty, is that what we want?

Yes, I think so. Processing the first parameter clause gives T <: Int. Then
when we instantiate T we do so to the lower bound. By what reasoning would
we infer Int?

Reply to this email directly or view it on GitHub
#752 (comment).

Martin Odersky
EPFL

@smarter
Copy link
Member

smarter commented Aug 11, 2015

Yes, I think so. Processing the first parameter clause gives T <: Int. Then
when we instantiate T we do so to the lower bound. By what reasoning would
we infer Int?

Because in the trees that the user will write (like getParam(new Contra[Int])), T only appears contravariantly, so we only add constraints to its upper boud, and it seems intuitive to instantiate T to the bound where we added constraints.

If we don't do type parameter maximization here, could you provide an example where we do type parameter maximization and it's useful (the program would not compile otherwise or would produce a different result) ?

@retronym
Copy link
Member

The reasoning in scalac:

scala> :paste -raw
// Entering paste mode (ctrl-D to finish)

import scala.reflect._

class Contra[-T]

object Test {
  def getParam[T](c: Contra[T])(implicit ct: ClassTag[T]): Unit = {
    println(ct)
  }
  def getParam1[T](c: Contra[T]) = ()
}


scala> val getParam1 = typeOf[Test.type].member(TermName("getParam1"))
getParam1: $r.intp.global.Symbol = method getParam1

scala> val tparams = getParam1.info.typeParams
tparams: List[$r.intp.global.Symbol] = List(type T)

scala> val tvars = tparams map (sym => TypeVar(sym))
tvars: List[$r.intp.global.TypeVar] = List(?T)

scala> val withTVars = getParam1.info.resultType.instantiateTypeParams(tparams, tvars)
withTVars: $r.intp.global.Type = (c: Contra[?T])Unit

scala> typeOf[Contra[Int]] <:< withTVars.params.head.info
res19: Boolean = true

scala> val formal1 = getParam1.info.params.head.info
formal1: $r.intp.global.Type = Contra[T]

scala> tvars.head.constr
res20: $r.intp.global.TypeConstraint =  <: Int

scala> varianceInType(getParam1.info.params.head.info)(tparams.head)
res27: scala.reflect.internal.Variance = contravariant

scala> varianceInType(formal1)(tparams.head)
res38: scala.reflect.internal.Variance = contravariant

scala> formal1.typeConstructor.typeParams.head.variance
res39: scala.reflect.internal.Variance = contravariant

scala> varianceInType(formal1.typeArgs.head)(tparams.head).flip
res40: scala.reflect.internal.Variance = contravariant

scala> solve(tvars, tparams, varianceInType(formal1)(tparams.head) :: Nil, upper = false, reflect.internal.Depth.AnyDepth)
res46: Boolean = true

scala> tvars.head.inst
res47: $r.intp.global.Type = Int

@odersky
Copy link
Contributor Author

odersky commented Aug 11, 2015

Thanks for the analysis, Jason. It seems to point to a fundamental
difference between scalac's and dotty's type inference. I have to mull this
over.

  • Martin

On Mon, Aug 10, 2015 at 6:16 PM, Jason Zaugg notifications@github.com
wrote:

The reasoning in scalac:

scala> :paste -raw
// Entering paste mode (ctrl-D to finish)

import scala.reflect._

class Contra[-T]

object Test {
def getParam[T](c: Contra[T])(implicit ct: ClassTag[T]): Unit = {
println(ct)
}
def getParam1[T](c: Contra[T]) = ()
}

scala> val getParam1 = typeOf[Test.type].member(TermName("getParam1"))
getParam1: $r.intp.global.Symbol = method getParam1

scala> val tparams = getParam1.info.typeParams
tparams: List[$r.intp.global.Symbol] = List(type T)

scala> val tvars = tparams map (sym => TypeVar(sym))
tvars: List[$r.intp.global.TypeVar] = List(?T)

scala> val withTVars = getParam1.info.resultType.instantiateTypeParams(tparams, tvars)
withTVars: $r.intp.global.Type = (c: Contra[?T])Unit

scala> typeOf[Contra[Int]] <:< withTVars.params.head.info
res19: Boolean = true

scala> val formal1 = getParam1.info.params.head.info
formal1: $r.intp.global.Type = Contra[T]

scala> tvars.head.constr
res20: $r.intp.global.TypeConstraint = <: Int

scala> varianceInType(getParam1.info.params.head.info)(tparams.head)
res27: scala.reflect.internal.Variance = contravariant

scala> varianceInType(formal1)(tparams.head)
res38: scala.reflect.internal.Variance = contravariant

scala> formal1.typeConstructor.typeParams.head.variance
res39: scala.reflect.internal.Variance = contravariant

scala> varianceInType(formal1.typeArgs.head)(tparams.head).flip
res40: scala.reflect.internal.Variance = contravariant

scala> solve(tvars, tparams, varianceInType(formal1)(tparams.head) :: Nil, upper = false, reflect.internal.Depth.AnyDepth)
res46: Boolean = true

scala> tvars.head.inst
res47: $r.intp.global.Type = Int


Reply to this email directly or view it on GitHub
#752 (comment).

Martin Odersky
EPFL

@smarter
Copy link
Member

smarter commented Sep 5, 2015

/rebuild

@odersky
Copy link
Contributor Author

odersky commented Sep 18, 2015

Part of this is now in #799. I'll revive the rest in a separate PR.

@odersky odersky closed this Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants