Skip to content
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

Allow closures to use method type parameters #815

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

EliasC
Copy link
Contributor

@EliasC EliasC commented Jun 1, 2017

This commit fixes a bug where closures did not properly capture
type parameters of methods. A test has been added.

Fixes #809.

This commit fixes a bug where closures did not properly capture
type parameters of methods. A test has been added.

Fixes parapluu#809.
@EliasC EliasC requested a review from PhucVH888 June 1, 2017 20:02
@kikofernandez
Copy link
Contributor

There is a test that fails. Let me know when this is fixed and I can review the PR (unless there's someone else)

@EliasC
Copy link
Contributor Author

EliasC commented Jun 2, 2017

Hm, all tests pass on my machine. Is CI running in debug mode? Anyway, I think I found the source of the error and have repushed (ping @kikofernandez).

Copy link
Contributor

@kikofernandez kikofernandez left a comment

Choose a reason for hiding this comment

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

The changes are right but there is a case that's missing and should be fixed (more details in the comment)


read class Foo
def foo[ty]() : () -> unit
fun() => (new Bar[ty]()).bar()
Copy link
Contributor

Choose a reason for hiding this comment

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

This works for this case but doesn't work for the following one (copy-paste of this function plus a local function declaration):

read class Bar[t]
  def bar() : unit
    println("In Bar")
  end
end

read class Foo
  def foo[ty]() : () -> unit
    fun() => (new Bar[ty]()).bar()
  where
    fun foo(): unit
      new Bar[ty]()
    end
  end
end

active class Main
  def main() : unit
    val f = (new Foo()).foo[int]()
    f()
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Looking into it now...

Choose a reason for hiding this comment

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

@EliasC What is the status of this PR? Does it still fail for @kikofernandez's example?

Choose a reason for hiding this comment

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

The bug this example hits has been reported as #809 — it seems to be not caused by this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have not had time to fix it. The discussion below is all there is so far.

Choose a reason for hiding this comment

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

I have this problem too, now.

@EliasC
Copy link
Contributor Author

EliasC commented Jun 2, 2017

The case with local functions turns out to be tricky! Consider the following snippet:

def m[t]() : Foo[t]
  f()
where
  fun f() : Foo[t]
    new Foo[t]()
  end
end

The problem here is that the local function f uses the (runtime representation of) type parameter t of m, but that we have no obvious way of passing it. In this particular case we could pass the type as a parameter to the function call, but if we use f as a value this is not an option:

def m[t]() : () -> Foo[t]
  f
where
  fun f() : Foo[t]
    new Foo[t]()
  end
end

Here instead we could treat f as a closure, give it an environment and store t there, but then the implementation of f needs to to start by unpacking this environment, which it did not need to do in the first case. We could always treat the local functions as closures, but then we would take a performance hit when using local functions (due to allocation of closure meta data and lost inlining opportunities). We could optimize this by only treating local functions as closures when they capture a type variable from their environment, but this would instead make the compiler more complicated.

The easy way out would be to disallow local functions which mention type variables that they do not define themselves, but this seems like a weird restriction (especially in the case of parametric classes, which have the same problems). Ideas for how to do this in a more clever way are welcome!

@kikofernandez
Copy link
Contributor

kikofernandez commented Jun 6, 2017

Maybe this topic deserves a bit of space for the design discussion. For functions, I always had in mind:

fun foo[t](x: t): Bar[t]
    baz(x)
where
    fun baz[t'](x): Bar[t']
        ...
    end
end

The current type inference may even do this already.

For classes, maybe there's a way to attach type variables to a context, i.e. a parametric class defines a type parameter t at the context of the class. A parametric method defines a type variable t' to the method (context). Could there be a way to do this in a generic way? (I may be talking nonsense)

@supercooldave
Copy link

@kikofernandez I'm not sure I follow what you are saying.
Are you saying: Closure environments should also have type variables?

@EliasC
Copy link
Contributor Author

EliasC commented Jun 7, 2017

Maybe this is what @kikofernandez means, but one solution could be to add an implicit parameter to each local function (one for each parameter in the context) and then add these as argument to each use of that function):

class C[a1, ..., ak]
  def m[b1, ..., bm]() : unit
    f[t1, ..., tn]()
  where
    fun f[c1, ..., cn]() : unit

==>

class C[a1, ..., ak]
  def m[b1, ..., bm]() : unit
    f[a1, ..., ak, b1, ..., bm, t1, ..., tn]()
  where
    fun f[a1, ..., ak, b1, ..., bm, c1, ..., cn]() : unit

We already disallow shadowing of type parameter names, so it should be simple to add these parameters after typechecking.

@kikofernandez
Copy link
Contributor

it was a bit in between. I meant a context (closure) in the more generic sense and not as a lambda capturing state. @EliasC proposal, albeit what I had in mind, is more related to what I wanted to express

@supercooldave
Copy link

What @elias proposes is called Closure Conversion. This is typically considered to be an optimisation. If there are many many parameters, it may not be.

@supercooldave
Copy link

@kikofernandez I still don't know what you mean.

@kikofernandez
Copy link
Contributor

kikofernandez commented Jun 7, 2017

A polymorphic class has type variables that can be used in the formal arguments of methods, etc. A parametric method can have type variables that are used in closures and inner functions. A function may define type variables that can be used in the body and in inner functions.

It seems like we are repeating the same pattern over and over. I am not proposing a solution, I am pointing out that there needs to be a way to extract this pattern as in: a class receives type parameters and they can be used anywhere in their enclosing body (methods, method bodies, inner functions, closures, etc), the same thing happens to parametric methods, they may receive type variables from a top thing (a class in this case, although that's an implementation detail) and they may define disjoint type variables, that can further be used inside.

I am just pointing out that this seems to be a pattern that we could optimise everywhere in the compiler, instead of keeping separate implementations for each one. I have not specified the details of how to do this. (I hope this clarifies my previous comment)

@EliasC proposal is a possible solution

@supercooldave
Copy link

@kikofernandez I understand now.

@albertnetymk
Copy link
Contributor

@EliasC The different treatment of functions when called directly and passed around as lambda already exists, as far as I can tell.

active class Main
  def f() : int
    val x = g
    g()
    0
  where
    fun g() : int
      0
    end
  end

  def main() : unit
    0
  end
end

@supercooldave
Copy link

@albertnetymk What you say is true, but your example does not use type parameters.

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.

Incorrect translation of polymorphic type
4 participants