Use correct type signatures for FFI decls (#257)

[hdgarrood]

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 type signature for a given name. How should this be handled?

[bergmark]

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 find instead of filter.

Did you test this with:

f, g :: Int
f = ffi "1"
g = ffi "2"

? 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

1. Reorder type declarations in desugaring so they are always above the definition
2. Collect all type signatures for the current module in InitialPass, put them in CompileState and do a lookup
3. Desugar all ffi calls to be of the expression form instead (ffi "x" :: Ty)

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!

[hdgarrood]

Re having type signatures with the same name -- this is ok:

{-# LANGUAGE ScopedTypeVariables #-}
x :: Int
x :: Int = 1

but this is not:

x :: Int
x :: Int
x = 1

Does that mean I can use find instead of filter? (Would it make it easier to add support for ScopedTypeVariables first?)

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:

-- top level
x :: String
x = ffi "\"hello\""

-- where decl
y :: String
y = hello ++ ", world"
  where
    hello :: String
    hello = ffi "\"hello\""

-- expression
z :: String
z = "hello, " ++ (ffi "\"world\"" :: String)

I think I could tackle the desugaring approach, yeah :)

[hdgarrood]

Oh: to answer your other question: currently it doesn't work inside a where clause.

[bergmark]

Ah, makes sense that you can do x :: Int = 1, but had never thought about it :-)

This is a bit far fetched, but this would be an issue and typechecks:

x :: Foo
x :: Ptr Foo = ffi "..."

since the generated code would be different.

x :: Num a => a
x :: Int = 1 -- This is not valid

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.

[hdgarrood]

Cool. I'll have a go at the desugaring approach, then.

[hdgarrood]

@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: Compile.StrictWrapper. The relevant bit looks like this:

main = do
  ffi "console.log(Strict.StrictWrapper.f(1,2))" :: Fay ()
  ffi "console.log(Strict.StrictWrapper.g({instance:'R',i:1}))" :: Fay ()
  ffi "console.log(Strict.StrictWrapper.h({instance:'R',i:1}))" :: Fay ()
  ffi "console.log(Strict.StrictWrapper.r)" :: Fay ()
  ffi "Strict.StrictWrapper.clog(123)" :: Fay ()

It looks like the last line is not being executed; the result is ok except that the line "123" is missing. I'm baffled as to how this has happened -- surely compileExp takes care of the whole RHS, since it's an UnGuardedRhs? -- and I haven't even touched the module that contains that function. Any suggestions?

[hdgarrood]

Ok, I think it's a separate bug. I've opened an issue: #379

[bergmark]

Ok, thanks! The strictwrapper can be quite tricky, I'll take a look later.

[hdgarrood]

As far as I can tell, this now works! :)

The only thing is that the strict wrapper issue has caused tests/Compile/StrictWrapper.hs to start failing, so this might be blocked on that.

[hdgarrood]

Ok, now it's ready.

[bergmark]

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:

Debug.Trace.trace = function($p1){
  return function($p2){
    return console.log(Fay$$fayToJs_string($p1)),$p2;
  };
};

But it should be this (which it is on master):

Debug.Trace.trace = function($p1){
  return function($p2){
    return new Fay$$$(function(){
      return console.log(Fay$$fayToJs_string($p1)),$p2;
    });
  };
};

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

compilePatBind toplevel patDecl = case patDecl of
  PatBind _ (PVar _ ident'@Ident{}) Nothing
    (UnGuardedRhs _
      (ExpTypeSig _
        (App _ (Var _ (UnQual _ (Ident _ "ffi")))
                (Lit _ (String _ formatstr _)))
      sig)) Nothing ->
    compileFFI True ident' formatstr sig

[bergmark]

Hmm I have an idea, let me try it out...

[bergmark]

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!

[hdgarrood]

You're welcome. :)