Skip to content

Fix #4419: liftFun should also update method parameter types #4428

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 1 commit into from
May 28, 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
24 changes: 21 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
*/
protected def liftFun(): Unit = ()

/** Whether `liftFun` is needed? It is the case if default arguments are used.
*/
protected def needLiftFun: Boolean =
!isJavaAnnotConstr(methRef.symbol) &&
args.size < reqiredArgNum(funType)

/** A flag signalling that the typechecking the application was so far successful */
private[this] var _ok = true

Expand All @@ -205,12 +211,25 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
/** The function's type after widening and instantiating polytypes
* with TypeParamRefs in constraint set
*/
val methType = funType.widen match {
lazy val methType: Type = liftedFunType.widen match {
case funType: MethodType => funType
case funType: PolyType => constrained(funType).resultType
case tp => tp //was: funType
}

def reqiredArgNum(tp: Type): Int = tp.widen match {
case funType: MethodType => funType.paramInfos.size
case funType: PolyType => reqiredArgNum(funType.resultType)
case tp => args.size
}

lazy val liftedFunType =
if (needLiftFun) {
liftFun()
normalizedFun.tpe
}
else funType

/** The arguments re-ordered so that each named argument matches the
* same-named formal parameter.
*/
Expand All @@ -231,6 +250,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
()
else
fail(err.typeMismatchMsg(methType.resultType, resultType))

// match all arguments with corresponding formal parameters
matchArgs(orderedArgs, methType.paramInfos, 0)
case _ =>
Expand Down Expand Up @@ -425,8 +445,6 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
}

def tryDefault(n: Int, args1: List[Arg]): Unit = {
if (!isJavaAnnotConstr(methRef.symbol))
liftFun()
val getter = findDefaultGetter(n + numArgs(normalizedFun))
if (getter.isEmpty) missingArg(n)
else {
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/EtaExpansion.scala
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ abstract class Lifter {
if (noLift(expr)) expr
else {
val name = UniqueName.fresh(prefix)
var liftedType = fullyDefinedType(expr.tpe.widen, "lifted expression", expr.pos)
// don't instantiate here, as the type params could be further constrained, see tests/pos/pickleinf.scala
Copy link
Member

Choose a reason for hiding this comment

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

This is very likely to break things. Dotty does not have global type inference, once you create a val, its type cannot be further constrained. (In fact, I remember adding the fullyDefinedType call here to fix a problem, git blame should tell you more)

Copy link
Member

Choose a reason for hiding this comment

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

After looking into it this might be OK because we're more careful about how we deal with signatures containing type variables in recent versions of Dotty, I've found one related issue for which I've opened a PR: #4593

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for looking into it @smarter !

var liftedType = expr.tpe.widen
if (liftedFlags.is(Method)) liftedType = ExprType(liftedType)
val lifted = ctx.newSymbol(ctx.owner, name, liftedFlags, liftedType, coord = positionCoord(expr.pos))
defs += liftedDef(lifted, expr).withPos(expr.pos.focus)
Expand Down
12 changes: 12 additions & 0 deletions tests/pos/i4419.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class Foo(config: String) {
case class Bar(val x: Int) {
def doThings: String = config //Do whatever here
}
}


object Test {
def test(foo: Foo)(bar: foo.Bar = foo.Bar(5)) = ???

test(new Foo("port"))()
}