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

Merged
merged 6 commits into from Dec 3, 2016

Conversation

Projects
None yet
4 participants
@odersky
Contributor

odersky commented Nov 30, 2016

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 ?

odersky added some commits Nov 29, 2016

Make clear where symbols are entered or not.
In definitions some of the new... methods entered the created
symbol while others did not. We now make that distrinction clear
in the name.
Create FunctionN types on demand
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.
Drop limit 30 of generated function classes
Function classes beyond 22 are now generated on demand,
with no upper limit.
Drop function 22 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.
@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 30, 2016

Contributor

The PR number #1758 is interesting: 17 + 5 is 22 and 17+5+8 = 30, our previous limit of the arity of temporary functions 😉

Contributor

odersky commented Nov 30, 2016

The PR number #1758 is interesting: 17 + 5 is 22 and 17+5+8 = 30, our previous limit of the arity of temporary functions 😉

@sledorze

This comment has been minimized.

Show comment
Hide comment
@sledorze

sledorze Nov 30, 2016

fun all way down :)

sledorze commented Nov 30, 2016

fun all way down :)

@OlivierBlanvillain

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...

Show outdated Hide outdated compiler/src/dotty/tools/dotc/core/Definitions.scala
@@ -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

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Should it be strict equality instead?

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Should it be strict equality instead?

This comment has been minimized.

@odersky

odersky Dec 1, 2016

Contributor

yes, indeed.

@odersky

odersky Dec 1, 2016

Contributor

yes, indeed.

val sym = tp.symbol
sym.isClass &&
sym != defn.AnyClass && sym != defn.ArrayClass &&
!defn.isUnimplementedFunctionClass(sym)

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Should we implement isImplementedFunctionClass instead to avoid the double negation?

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Should we implement isImplementedFunctionClass instead to avoid the double negation?

This comment has been minimized.

@odersky

odersky Dec 1, 2016

Contributor

It's just one occurrence, so probably not.

@odersky

odersky Dec 1, 2016

Contributor

It's just one occurrence, so probably not.

Show outdated Hide outdated library/src/scala/FunctionXXL.scala
** /____/\___/_/ |_/____/_/ | | **
** |/ **
\* */
// GENERATED CODE: DO NOT EDIT. See scala.Function0 for timestamp.

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Copy-past leftover?

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Copy-past leftover?

Show outdated Hide outdated compiler/src/dotty/tools/dotc/transform/Erasure.scala
@@ -423,6 +443,9 @@ object Erasure extends TypeTestsCasts{
}
}
/** Besides notmal typing, this method collects all arguments

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Indentation is off

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

Indentation is off

/** A function with all parameters grouped in an array. */
trait FunctionXXL {
def apply(xs: Array[Object]): Object

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

What's the motivation for Object instead of Any?

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

What's the motivation for Object instead of Any?

This comment has been minimized.

@odersky

odersky Dec 1, 2016

Contributor

FunctionXXL is only used after erasure, when Any is mapped to Object anyway. So I prefer to be explicit.

@odersky

odersky Dec 1, 2016

Contributor

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

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

s/to ise/into/?

@OlivierBlanvillain

OlivierBlanvillain Nov 30, 2016

Contributor

s/to ise/into/?

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Nov 30, 2016

Contributor

Looks great! I've tried running the full test suite with MaxImplementedFunctionArity = 3 which revealed are quite a few failing test cases, for instance:

I should have mentioned: Only the implemented function traits have tupled and curried methods. The synthetic ones lack them.

Contributor

odersky commented Nov 30, 2016

Looks great! I've tried running the full test suite with MaxImplementedFunctionArity = 3 which revealed are quite a few failing test cases, for instance:

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)

This comment has been minimized.

@OlivierBlanvillain

OlivierBlanvillain Dec 1, 2016

Contributor

functionArity(tp) could be replaced with arity

@OlivierBlanvillain

OlivierBlanvillain Dec 1, 2016

Contributor

functionArity(tp) could be replaced with arity

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Dec 3, 2016

Contributor

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 curried and tupled functions into decorators. Then we should be able to reduce MaxImplementedFunctionArity at will.

Contributor

odersky commented Dec 3, 2016

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 curried and tupled functions into decorators. Then we should be able to reduce MaxImplementedFunctionArity at will.

@odersky odersky merged commit 8288a34 into lampepfl:master Dec 3, 2016

6 checks passed

cla @odersky signed the Scala CLA. Thanks!
Details
continuous-integration/drone/pr the build was successful
Details
validate-junit [2686] SUCCESS. Took 22 min.
Details
validate-main [2697] SUCCESS. Took 22 min.
Details
validate-partest [2691] SUCCESS. Took 20 min.
Details
validate-partest-bootstrapped [1778] SUCCESS. Took 21 min.
Details

@allanrenucci allanrenucci deleted the dotty-staging:change-functions branch Dec 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment