-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Drop function 22 limit #1758
Drop function 22 limit #1758
Conversation
In definitions some of the new... methods entered the created symbol while others did not. We now make that distrinction clear in the name.
We know create FunctionN types on demand whenever their name is looked up in the scope of package `scala`. This obviates the need to predefine function traits 23 to 30.
Function classes beyond 22 are now generated on demand, with no upper limit.
Functions with more than 22 parameters are now automatically converted to functions taking a single object array parameter. This has been achieved by tweaking erasure. Other things I have tried that did ot work out well: - Use a single function type in typer. The problem with this one which could not be circumvented was that existing higher-kinded code with e.g. Funcor assumes that Functon1 is a binary type constructor. - Have a late phase that converts to FunctonXXL instead of doing it in erasure. The problem with that one was that potentially every type could be affected, which was ill-suited to the architecture of a miniphase.
The PR number #1758 is interesting: 17 + 5 is 22 and 17+5+8 = 30, our previous limit of the arity of temporary functions 😉 |
fun all way down :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I've tried running the full test suite with MaxImplementedFunctionArity = 3
which revealed are quite a few failing test cases, for instance:
def f0(f: (Int, Int, Int) => Int): (Int, Int) => Int =
f(0, _, _)
I wonder if we could setup the infrastructure to re-run some tests with custom MaxImplementedFunctionArity
values, because expending these test cases by hand to arity 23 would be quite tedious...
@@ -646,6 +684,8 @@ class Definitions { | |||
tp.derivesFrom(NothingClass) || tp.derivesFrom(NullClass) | |||
|
|||
def isFunctionClass(cls: Symbol) = isVarArityClass(cls, tpnme.Function) | |||
def isUnimplementedFunctionClass(cls: Symbol) = | |||
isFunctionClass(cls) && cls.name.functionArity >= MaxImplementedFunctionArity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be strict equality instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, indeed.
val sym = tp.symbol | ||
sym.isClass && | ||
sym != defn.AnyClass && sym != defn.ArrayClass && | ||
!defn.isUnimplementedFunctionClass(sym) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we implement isImplementedFunctionClass
instead to avoid the double negation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just one occurrence, so probably not.
** /____/\___/_/ |_/____/_/ | | ** | ||
** |/ ** | ||
\* */ | ||
// GENERATED CODE: DO NOT EDIT. See scala.Function0 for timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-past leftover?
@@ -423,6 +443,9 @@ object Erasure extends TypeTestsCasts{ | |||
} | |||
} | |||
|
|||
/** Besides notmal typing, this method collects all arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off
/** A function with all parameters grouped in an array. */ | ||
trait FunctionXXL { | ||
|
||
def apply(xs: Array[Object]): Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for Object
instead of Any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionXXL is only used after erasure, when Any is mapped to Object anyway. So I prefer to be explicit.
@@ -470,18 +499,36 @@ object Erasure extends TypeTestsCasts{ | |||
super.typedValDef(untpd.cpy.ValDef(vdef)( | |||
tpt = untpd.TypedSplice(TypeTree(sym.info).withPos(vdef.tpt.pos))), sym) | |||
|
|||
/** Besides normal typing, this function also compacts anonymous functions | |||
* with more than `MaxImplementedFunctionArity` parameters to ise a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/to ise/into/
?
I should have mentioned: Only the implemented function traits have tupled and curried methods. The synthetic ones lack them. |
def isFunctionType(tp: Type)(implicit ctx: Context) = | ||
isFunctionClass(tp.dealias.typeSymbol) && { | ||
val arity = functionArity(tp) | ||
arity >= 0 && tp.isRef(FunctionType(functionArity(tp)).typeSymbol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
functionArity(tp)
could be replaced with arity
I think we should add the testing setup in another PR (if we get around to it). To recall: We'd need to move all |
Functions with more than 22 parameters are now automatically converted to functions taking
a single object array parameter.
This has been achieved using two tricks:
Create function types of arbitrary arities on demand.
This was achieved by tweaking the scope of the scala package.
Eliminate function types of large arity by mapping them to a new
FunctionXXL type that takes all parameters in a single object array.
This was achieved by tweaking erasure.
Other things I have tried that did not work out well:
Use a single function type in typer. The problem with this
one which could not be circumvented was that existing higher-kinded
code with e.g. Functor assumes that Functon1 is a binary type constructor.
Have a late phase that converts to FunctonXXL instead of
doing it in erasure. The problem with that one was that
potentially every type could be affected, which was ill-suited
to the architecture of a miniphase.
Review by @OlivierBlanvillain ?