Skip to content

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

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

Merged
adriaanm merged 2 commits into from
May 10, 2018
Merged

Do not eta-expand 0-arg methods. Improve eta-expansion for method values. #6475

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
33a56a3
Insert () before eta under -Xsource:2.13
adriaanm Apr 5, 2018
99ff973
Use FUNmode and wild pt in typing the `m` in `m _`
adriaanm Apr 11, 2018
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
23 changes: 14 additions & 9 deletions src/compiler/scala/tools/nsc/typechecker/ContextErrors.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,14 +667,14 @@ trait ContextErrors {
def MissingArgsForMethodTpeError(tree: Tree, meth: Symbol) = {
val f = meth.name.decoded
val paf = s"$f(${ meth.asMethod.paramLists map (_ map (_ => "_") mkString ",") mkString ")(" })"
val advice = s"""
|Unapplied methods are only converted to functions when a function type is expected.
|You can make this conversion explicit by writing `$f _` or `$paf` instead of `$f`.""".stripMargin
val advice =
if (meth.isConstructor || meth.info.params.length > definitions.MaxFunctionArity) ""
else s"""
|Unapplied methods are only converted to functions when a function type is expected.
|You can make this conversion explicit by writing `$f _` or `$paf` instead of `$f`.""".stripMargin
val message =
if (meth.isMacro) MacroTooFewArgumentListsMessage
else s"""missing argument list for ${meth.fullLocationString}${
if (!meth.isConstructor) advice else ""
}"""
else s"""missing argument list for ${meth.fullLocationString}$advice"""
issueNormalTypeError(tree, message)
setError(tree)
}
Expand Down Expand Up @@ -730,9 +730,12 @@ trait ContextErrors {
setError(tree)
}

def DependentMethodTpeConversionToFunctionError(tree: Tree, tp: Type): Tree = {
issueNormalTypeError(tree, "method with dependent type " + tp + " cannot be converted to function value")
setError(tree)
}

// cases where we do not necessarily return trees
def DependentMethodTpeConversionToFunctionError(tree: Tree, tp: Type) =
issueNormalTypeError(tree, "method with dependent type "+tp+" cannot be converted to function value")

//checkStarPatOK
def StarPatternWithVarargParametersError(tree: Tree) =
Expand Down Expand Up @@ -1313,9 +1316,11 @@ trait ContextErrors {
)
}

// Note that treeInfo.Applied always matches, it just returns Nil when no application was found...
def treeTypeArgs(annotatedTree: Tree): List[String] = annotatedTree match {
case TypeApply(_, args) => args.map(_.toString)
case Block(_, Function(_, treeInfo.Applied(_, targs, _))) => targs.map(_.toString) // eta expansion, see neg/t9527b.scala
case Function(_, treeInfo.Applied(_, targs, _)) => targs.map(_.toString) // eta expansion, see neg/t9527b.scala
case treeInfo.Applied(_, targs, _) => targs.map(_.toString)
case _ => Nil
}

Expand Down
12 changes: 8 additions & 4 deletions 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 @@ -17,7 +17,7 @@ import symtab.Flags._
trait EtaExpansion { self: Analyzer =>
import global._

/** Expand partial method application `p.f(es_1)...(es_n)`.
/** Expand partial method application `p.f(es_1)...(es_n)`. Does not support dependent method types (yet).
*
* We expand this to the following block, which evaluates
* the target of the application and its supplied arguments if needed (they are not stable),
Expand All @@ -33,8 +33,9 @@ trait EtaExpansion { self: Analyzer =>
* }
* ```
*
* This is called from instantiateToMethodType after type checking `tree`,
* and we realize we have a method type, where a function type (builtin or SAM) is expected.
* This is called from typedEtaExpansion, which itself is called from
* - instantiateToMethodType (for a naked method reference), or
* - typedEta (when type checking a method value, `m _`).
*
**/
def etaExpand(unit: CompilationUnit, tree: Tree, typer: Typer): Tree = {
Expand Down Expand Up @@ -120,6 +121,9 @@ trait EtaExpansion { self: Analyzer =>
}

val tree1 = liftoutPrefix(tree)
atPos(tree.pos)(Block(defs.toList, expand(tree1, tpe)))
val expansion = expand(tree1, tpe)

if (defs.isEmpty) expansion
else atPos(tree.pos)(Block(defs.toList, expansion))
}
}
3 changes: 0 additions & 3 deletions src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,4 @@ trait StdAttachments {

def removeStabilizingDefinitions(tree: Tree): Tree =
tree.removeAttachment[StabilizingDefinitions](StabilizingDefinitionsTag)

/** Added to trees that appear in a method value, e.g., to `f(x)` in `f(x) _` */
case object MethodValueAttachment
}
151 changes: 65 additions & 86 deletions 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 @@ -348,24 +348,6 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
}
}

def checkParamsConvertible(tree: Tree, tpe0: Type): Unit = {
def checkParamsConvertible0(tpe: Type) =
tpe match {
case MethodType(formals, restpe) =>
/*
if (formals.exists(_.typeSymbol == ByNameParamClass) && formals.length != 1)
error(pos, "methods with `=>`-parameter can be converted to function values only if they take no other parameters")
if (formals exists (isRepeatedParamType(_)))
error(pos, "methods with `*`-parameters cannot be converted to function values");
*/
if (tpe.isDependentMethodType)
DependentMethodTpeConversionToFunctionError(tree, tpe)
checkParamsConvertible(tree, restpe)
case _ =>
}
checkParamsConvertible0(tpe0)
}

/** Check that type of given tree does not contain local or private
* components.
*/
Expand Down Expand Up @@ -803,10 +785,10 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
* unless followed by explicit type application.
* (4) Do the following to unapplied methods used as values:
* (4.1) If the method has only implicit parameters pass implicit arguments
* (4.2) otherwise, if `pt` is a function type and method is not a constructor,
* convert to function by eta-expansion,
* (4.3) otherwise, if the method is nullary with a result type compatible to `pt`
* (4.2) otherwise, if the method is nullary with a result type compatible to `pt`
* and it is not a constructor, apply it to ()
* (4.3) otherwise, if `pt` is a function type and method is not a constructor,
* convert to function by eta-expansion,
* otherwise issue an error
* (5) Convert constructors in a pattern as follows:
* (5.1) If constructor refers to a case class factory, set tree's type to the unique
Expand Down Expand Up @@ -900,62 +882,42 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
def instantiateToMethodType(mt: MethodType): Tree = {
val meth = tree match {
// a partial named application is a block (see comment in EtaExpansion)
//
// TODO: document why we need to peel off one layer to begin with, and, if we do, why we don't need to recurse??
// (and why we don't need to look at the stats to make sure it's the block created by eta-expansion)
// I don't see how we call etaExpand on a constructor..
//
// I guess we don't need to recurse because the outer block and a nested block will refer to the same method
// after eta-expansion (if that's what generated these blocks), so we can just look at the outer one.
//
// How about user-written blocks? Can they ever have a MethodType?
case Block(_, tree1) => tree1.symbol
case _ => tree.symbol
}

def cantAdapt =
if (context.implicitsEnabled) MissingArgsForMethodTpeError(tree, meth)
else UnstableTreeError(tree)

def emptyApplication: Tree = adapt(typed(Apply(tree, Nil) setPos tree.pos), mode, pt, original)
if (context.implicitsEnabled) MissingArgsForMethodTpeError(tree, meth) else UnstableTreeError(tree)

// constructors do not eta-expand
if (meth.isConstructor) cantAdapt
// (4.2) eta-expand method value when function or sam type is expected
else if (isFunctionType(pt) || (!mt.params.isEmpty && samOf(pt).exists)) {
// scala/bug#9536 `!mt.params.isEmpty &&`: for backwards compatibility with 2.11,
// we don't adapt a zero-arg method value to a SAM
// In 2.13, we won't do any eta-expansion for zero-arg methods, but we should deprecate first

debuglog(s"eta-expanding $tree: ${tree.tpe} to $pt")
checkParamsConvertible(tree, tree.tpe)

// method values (`m _`) are always eta-expanded (this syntax will disappear once we eta-expand regardless of expected type, at least for arity > 0)
// a "naked" method reference (`m`) may or not be eta expanded -- currently, this depends on the expected type and the arity (the conditions for this are in flux)
def isMethodValue = tree.getAndRemoveAttachment[MethodValueAttachment.type].isDefined
val nakedZeroAryMethod = mt.params.isEmpty && !isMethodValue

// scala/bug#7187 eta-expansion of zero-arg method value is deprecated
// 2.13 will switch order of (4.3) and (4.2), always inserting () before attempting eta expansion
// (This effectively disables implicit eta-expansion of 0-ary methods.)
// See mind-bending stuff like scala/bug#9178
if (nakedZeroAryMethod && settings.isScala213) emptyApplication
else {
// eventually, we will deprecate insertion of `()` (except for java-defined methods) -- this is already the case in dotty
// Once that's done, we can more aggressively eta-expand method references, even if they are 0-arity
// 2.13 will already eta-expand non-zero-arity methods regardless of expected type (whereas 2.12 requires a function-equivalent type)
if (nakedZeroAryMethod && settings.isScala212) {
currentRun.reporting.deprecationWarning(tree.pos, NoSymbol,
s"Eta-expansion of zero-argument methods is deprecated. To avoid this warning, write ${Function(Nil, Apply(tree, Nil))}.", "2.12.0")
}

val tree0 = etaExpand(context.unit, tree, this)
// (4.2) apply to empty argument list
else if (mt.params.isEmpty && (settings.isScala213 || !isFunctionType(pt))) {
// Starting with 2.13, always insert `()`, regardless of expected type.
// On older versions, expected type must not be a FunctionN (can be a SAM)
//
// scala/bug#7187 deprecates eta-expansion of zero-arg method values (since 2.12; was never introduced for SAM types: scala/bug#9536).
// The next step is to also deprecate insertion of `()` (except for java-defined methods), as dotty already requires explicitly writing them.
// Once explicit application to () is required, we can more aggressively eta-expand method references, even if they are 0-arity
adapt(typed(Apply(tree, Nil) setPos tree.pos), mode, pt, original)
}
// (4.3) eta-expand method value when function or sam type is expected (for experimentation, always eta-expand under 2.14 source level)
else if (isFunctionType(pt) || samOf(pt).exists || settings.isScala214) { // TODO: decide on `settings.isScala214`
if (settings.isScala212 && mt.params.isEmpty) // implies isFunctionType(pt)
currentRun.reporting.deprecationWarning(tree.pos, NoSymbol, "Eta-expansion of zero-argument methods is deprecated. "+
s"To avoid this warning, write ${Function(Nil, Apply(tree, Nil))}.", "2.12.0")

// #2624: need to infer type arguments for eta expansion of a polymorphic method
// context.undetparams contains clones of meth.typeParams (fresh ones were generated in etaExpand)
// need to run typer on tree0, since etaExpansion sets the tpe's of its subtrees to null
// can't type with the expected type, as we can't recreate the setup in (3) without calling typed
// (note that (3) does not call typed to do the polymorphic type instantiation --
// it is called after the tree has been typed with a polymorphic expected result type)
if (hasUndets)
instantiate(typed(tree0, mode), mode, pt)
else
typed(tree0, mode, pt)
}
typedEtaExpansion(tree, mode, pt)
}
// (4.3) apply to empty argument list
else if (mt.params.isEmpty) emptyApplication
else cantAdapt
}

Expand Down Expand Up @@ -1645,7 +1607,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
map2(preSuperStats, preSuperVals)((ldef, gdef) => gdef.tpt setType ldef.symbol.tpe)

if (superCall1 == cunit) EmptyTree
else cbody2 match {
else cbody2 match { // ???
case Block(_, expr) => expr
case tree => tree
}
Expand Down Expand Up @@ -3003,7 +2965,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
// This tree is the result from one of:
// - manual eta-expansion with named arguments (x => f(x));
// - wildcard-style eta expansion (`m(_, _,)`);
// - instantiateToMethodType adapting a tree of method type to a function type using etaExpand.
// - (I don't think it can result from etaExpand, because we know the argument types there.)
//
// Note that method values are a separate thing (`m _`): they have the idiosyncratic shape
// of `Typed(expr, Function(Nil, EmptyTree))`
Expand Down Expand Up @@ -3082,6 +3044,23 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
}
}

// #2624: need to infer type arguments for eta expansion of a polymorphic method
// context.undetparams contains clones of meth.typeParams (fresh ones were generated in etaExpand)
// need to run typer on tree0, since etaExpansion sets the tpe's of its subtrees to null
// can't type with the expected type, as we can't recreate the setup in (3) without calling typed
// (note that (3) does not call typed to do the polymorphic type instantiation --
// it is called after the tree has been typed with a polymorphic expected result type)
def typedEtaExpansion(tree: Tree, mode: Mode, pt: Type): Tree = {
debuglog(s"eta-expanding $tree: ${tree.tpe} to $pt")

if (tree.tpe.isDependentMethodType) DependentMethodTpeConversionToFunctionError(tree, tree.tpe) // TODO: support this
else {
val expansion = etaExpand(context.unit, tree, this)
if (context.undetparams.isEmpty) typed(expansion, mode, pt)
else instantiate(typed(expansion, mode), mode, pt)
}
}

def typedRefinement(templ: Template): Unit = {
val stats = templ.body
namer.enterSyms(stats)
Expand Down Expand Up @@ -4590,14 +4569,6 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
treeCopy.New(tree, tpt1).setType(tp)
}

def functionTypeWildcard(arity: Int): Type =
functionType(List.fill(arity)(WildcardType), WildcardType)

def checkArity(tree: Tree)(tp: Type): tp.type = tp match {
case NoType => MaxFunctionArityError(tree); tp
case _ => tp
}


/** Eta expand an expression like `m _`, where `m` denotes a method or a by-name argument
*
Expand All @@ -4610,9 +4581,12 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
*/
def typedEta(methodValue: Tree): Tree = methodValue.tpe match {
case tp@(MethodType(_, _) | PolyType(_, MethodType(_, _))) => // (1)
val formals = tp.params
if (isFunctionType(pt) || samMatchesFunctionBasedOnArity(samOf(pt), formals)) methodValue
else adapt(methodValue, mode, checkArity(methodValue)(functionTypeWildcard(formals.length)))
val etaPt =
if (pt ne WildcardType) pt
else functionType(tp.params.map(_ => WildcardType), WildcardType) orElse WildcardType // arity overflow --> NoType

// We know syntactically methodValue can't refer to a constructor because you can't write `this _` for that (right???)
typedEtaExpansion(methodValue, mode, etaPt)

case TypeRef(_, ByNameParamClass, _) | NullaryMethodType(_) => // (2)
val pos = methodValue.pos
Expand Down Expand Up @@ -5397,14 +5371,19 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
// that typecheck must not trigger macro expansions, so we explicitly prohibit them
// however we cannot do `context.withMacrosDisabled`
// because `expr` might contain nested macro calls (see scala/bug#6673).
// Otherwise, eta-expand, passing the original tree, which is required in adapt
// for trees of the form `f() _`: if the method type takes implicits, the fallback
// strategy will use `f()`; else if not, original is used to distinguish an explicit
// method value from eta-expansion driven by an expected function type.
// Otherwise, (check for dead code, and) eta-expand.
case MethodValue(expr) =>
typed1(suppressMacroExpansion(expr), mode, pt) match {
// Need to type in FUNmode so that we accept a method type (which also means we can't use our pt),
// this does mean no overloading is performed. The main reason to ignore pt and move to FUNmode is that
// the `m` in `m _` could involve an implicit conversion, which will go through adapt after converting,
// which will run afoul of the restriction that a method-typed tree is only allowed when a function type is expected.
// We peeled off the `_` marker for the typed1 call, so we don't know that the user has requested eta-expansion.
// See scala/bug#8299.
val funTyped = typed1(suppressMacroExpansion(expr), mode | FUNmode, WildcardType)
if (funTyped.tpe.isInstanceOf[OverloadedType]) inferExprAlternative(funTyped, pt)
funTyped match {
case macroDef if treeInfo.isMacroApplication(macroDef) => MacroEtaError(macroDef)
case methodValue => typedEta(checkDead(methodValue).updateAttachment(MethodValueAttachment))
case methodValue => typedEta(checkDead(methodValue))
}
case Typed(expr, tpt) =>
val tpt1 = typedType(tpt, mode) // type the ascribed type first
Expand Down
6 changes: 0 additions & 6 deletions test/files/neg/t7187-2.13.check
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion test/files/neg/t7187-2.13.flags
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions test/files/neg/t7187-2.13.scala
View file
Open in desktop

This file was deleted.

11 changes: 11 additions & 0 deletions test/files/neg/t7187-2.14.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
t7187-2.14.scala:6: error: type mismatch;
found : Int
required: () => Any
val t1: () => Any = m1 // error
^
t7187-2.14.scala:7: error: type mismatch;
found : Int
required: () => Any
val t2: () => Any = m2 // error, no eta-expansion of zero-args methods
^
two errors found
1 change: 1 addition & 0 deletions test/files/neg/t7187-2.14.flags
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Xsource:2.14
25 changes: 25 additions & 0 deletions test/files/neg/t7187-2.14.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class EtaExpand214 {
def m1 = 1
def m2() = 1
def m3(x: Int) = x

val t1: () => Any = m1 // error
val t2: () => Any = m2 // error, no eta-expansion of zero-args methods
val t3: Int => Any = m3 // ok

val t4 = m1 // apply
val t5 = m2 // apply, ()-insertion
val t6 = m3 // eta-expansion in 2.14

val t4a: Int = t4 // ok
val t5a: Int = t5 // ok
val t6a: Int => Any = t6 // ok

val t7 = m1 _
val t8 = m2 _
val t9 = m3 _

val t7a: () => Any = t7 // ok
val t8a: () => Any = t8 // ok
val t9a: Int => Any = t9 // ok
}
7 changes: 6 additions & 1 deletion test/files/neg/t7187.check
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,9 @@ t7187.scala:25: error: type mismatch;
required: () => Any
val t4b: () => Any = zap() // ditto
^
8 errors found
t7187.scala:30: error: missing argument list for method zup in class EtaExpandZeroArg
Unapplied methods are only converted to functions when a function type is expected.
You can make this conversion explicit by writing `zup _` or `zup(_)` instead of `zup`.
val t5a = zup // error in 2.13, eta-expansion in 2.14
^
9 errors found
Loading