-
Notifications
You must be signed in to change notification settings - Fork 26
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
base: development
Are you sure you want to change the base?
Conversation
This commit fixes a bug where closures did not properly capture type parameters of methods. A test has been added. Fixes parapluu#809.
There is a test that fails. Let me know when this is fixed and I can review the PR (unless there's someone else) |
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). |
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.
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() |
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.
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
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.
Good catch! Looking into it now...
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.
@EliasC What is the status of this PR? Does it still fail for @kikofernandez's example?
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.
The bug this example hits has been reported as #809 — it seems to be not caused by this PR.
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, I have not had time to fix it. The discussion below is all there is so far.
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.
I have this problem too, now.
The case with local functions turns out to be tricky! Consider the following snippet:
The problem here is that the local function
Here instead we could treat 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! |
Maybe this topic deserves a bit of space for the design discussion. For functions, I always had in mind:
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 |
@kikofernandez I'm not sure I follow what you are saying. |
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):
==>
We already disallow shadowing of type parameter names, so it should be simple to add these parameters after typechecking. |
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 |
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. |
@kikofernandez I still don't know what you mean. |
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 |
@kikofernandez I understand now. |
@EliasC The different treatment of functions when called directly and passed around as lambda already exists, as far as I can tell.
|
@albertnetymk What you say is true, but your example does not use type parameters. |
This commit fixes a bug where closures did not properly capture
type parameters of methods. A test has been added.
Fixes #809.