-
Notifications
You must be signed in to change notification settings - Fork 86
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
Use correct type signatures for FFI decls (#257) #378
Conversation
When compiling a pattern binding, instead of assuming the type signature directly before it is the associated one, Fay now looks through all the type signatures in the module and chooses the correct one based on the name. Still todo: Currently calls `error` if there is more than one type signature for a given name. How should this be handled?
Awesome! Is it even legal to have two type signatures for the same name? You can check, but I think it isn't and then you can just use Did you test this with:
? Would be nice to support that as well. Other than that it looks good to merge. I got thinking about other ways to solve this (#376), would this abstract for where clauses? There are a few other ways of solving this that I can think of
I think i like 3 the most since it generalizes all three cases (top level decl, where decl, expression) and the ffi code could be simplified to only handle one case. Are you up for tackling that as well? :) I can merge this PR either way. Thanks! |
Re having type signatures with the same name -- this is ok:
but this is not:
Does that mean I can use Re a type signature having two or more names -- I hadn't, but I just did, and it does seem to work. Re abstracting for where clauses; just to check I've understood: we want to handle all three of the following:
I think I could tackle the desugaring approach, yeah :) |
Oh: to answer your other question: currently it doesn't work inside a |
Ah, makes sense that you can do This is a bit far fetched, but this would be an issue and typechecks:
since the generated code would be different.
So adding support for ScopedTypeVariables would complicate things a bit, we can just add a note in that ticket for now. Other than that ScopedTypeVariables for the ffi should just be another desugaring. |
Cool. I'll have a go at the desugaring approach, then. |
* Add a desugar step which copies the type signature for toplevel FFI declarations to an explicit one in the RHS expression. * Remove (now unnecessary) code from compileDecls and compileDecl Still todo: * Adding FFI type signatures in let/where bindings * Find out why StrictWrapper test is broken. Seems to be ignoring the last statement in the do block.
@bergmark here's a first stab. So far only toplevel declarations are implemented (although I expect to be able to reuse most/all of the code from the toplevel FFI desugar step in the not-yet-written let/where FFI desugar step). Unfortunately it's broken one of the tests:
It looks like the last line is not being executed; the result is ok except that the line "123" is missing. |
Ok, I think it's a separate bug. I've opened an issue: #379 |
Ok, thanks! The strictwrapper can be quite tricky, I'll take a look later. |
refs faylang#257 faylang#376 * add type signatures to FFI expressions in let/where bindings during desugar step * Fix DesugarFFI test (and uncomment FFI calls in let/where binds)
As far as I can tell, this now works! :) The only thing is that the strict wrapper issue has caused |
It was only used for FFI type signatures, which are now added to expressions explicitly in a desugar step.
Ok, now it's ready. |
I rebased your commits on top of my fix for #379 (pushed that to 1fd14b5), there's now a new issue with a missing thunk. With your commits:
But it should be this (which it is on master):
It might be that the new case I added to compilePatBind doesn't match what you desugar ffi declarations to, can you check? I match on
|
Hmm I have an idea, let me try it out... |
I introduced another bug while fixing the strictwrapper bug, so I fixed that one too and refactored the FFI module a bit so it only deals with expressions. This turned out great, thanks a lot! |
You're welcome. :) |
refs #257
When compiling a pattern binding, instead of assuming the type signature
directly before it is the associated one, Fay now looks through all the
type signatures in the module and chooses the correct one based on the
name.
Still todo: Currently calls
error
if there is more than one typesignature for a given name. How should this be handled?