Automatic not applied through function with unconstrained argument type

[johncant]

Hi! I think I've found a problem. Using the Automatic type directly in an FFI works fine, making the type automatic, so that it gets converted between the lazy fay list representation and the JS representation correctly. However, if you pass Automatic instances to a function as an argument with an unconstrained type, which then calls ffi, the argument is treated as unknown, and isn't converted to JS properly. It's probably best illustrated with an example:

module AutomaticBug where

import FFI

fooWorking :: Automatic [Bool] -> Fay ()
fooWorking = ffi "console.log(%1)"

fooNotWorking1 :: a -> Fay ()
fooNotWorking1 = ffi "console.log(%1)"

fooNotWorking :: Automatic [Bool] -> Fay ()
fooNotWorking = (fooNotWorking1 )

main :: Fay ()
main = do
  fooWorking [ True ]
  fooNotWorking [ True ]

expected: "[ true ]\n[ true ]\n"
but got: "[ true ]\n{ car: true, cdr: null }\n"

An example of where it's useful is https://github.com/faylang/fay-angular/blob/develop/src/Angular/Ng/RootScope.hs#L44-54 , where you can't just declare all the function arguments to be Automatic. I'm happy to have a go at fixing this if you point me in the right direction.

[johncant]

var FFI = {};
var AutomaticBug = {};
AutomaticBug.fooWorking = function($p1){
  return new Fay$$$(function(){
    return new Fay$$Monad(Fay$$jsToFay(["unknown"],console.log(Fay$$fayToJs(["automatic"],$p1))));
  });
};
AutomaticBug.fooNotWorking1 = function($p1){
  return new Fay$$$(function(){
    return new Fay$$Monad(Fay$$jsToFay(["unknown"],console.log(Fay$$fayToJs(["unknown"],$p1))));
  });
};
AutomaticBug.fooNotWorking = new Fay$$$(function(){
  return AutomaticBug.fooNotWorking1;
});
AutomaticBug.main = new Fay$$$(function(){
  return Fay$$_(Fay$$_(Fay$$then)(Fay$$_(AutomaticBug.fooWorking)(Fay$$list([true]))))(Fay$$_(AutomaticBug.fooNotWorking)(Fay$$list([true])));
});

Am I right in thinking that the type is baked in during compilation of ffi?

[johncant]

I've think I've now read all the issues and threads. Surely it's impossible to sort out this ambiguity without the use of type information?

[bergmark]

I think you're correct.

[ryachza]

What would be the harm in changing the deserialization of "unknown" from "ptr" to "automatic"? It would resolve this case and many others, but I am not sure where else/all the output of "unknown" is generated and what other effects changing it's interpretation would have.

[bergmark]

It would break any foreign object that we wish to keep untouched, e.g.

{-# LANGUAGE EmptyDataDecls #-}
data Vector a
empty :: Vector a
empty = ffi "[]"

We could perhaps tag empty data constructors so that empty still uses ptr since it refers to an EmptyDataDecl, but the example given, fooNotWorking1, would then break for ptr instead of automatic.

For lists in particular there are actually three options

1. deserialize to list
2. deserialize to tuple (edit: but i just remembered that this is actually represented as lists internally as well!)
3. leave as is

I can't tell whether ptr or automatic by default is better though. I'd be open to adding a compiler flag to reverse the behavior, that way it's accessible for more people to test.

[ryachza]

I'm sorry I'm not sure I followed. empty in this case appears to be serialized as "user" which I believe is already deserialized the same as "automatic". Unless you are referring to the third element in ["user","Vector",[["unknown"]]] but I'm not sure yet where that is involved.

Referencing the following test case:

{-# EmptyDataDecls #-}

module Test where

import FFI

data Vector a

empty :: Fay (Vector a)
empty = ffi "[]"

empty2 :: Vector a
empty2 = ffi "[]"

singleton :: a -> Vector a
singleton = ffi "[%1]"

fooGeneral :: a -> Fay ()
fooGeneral = ffi "console.log(%1)"

fooVector :: Vector a -> Fay()
fooVector = ffi "console.log(%1)"

main = do
    empty >>= fooGeneral     -- 1
    fooGeneral empty2        -- 2
    fooGeneral (singleton 5) -- 3

    empty >>= fooVector      -- 4
    fooVector empty2         -- 5
    fooVector (singleton 5)  -- 6

The lines labeled 1, 4, 5, and 6 appear to currently work as I would expect, but lines 2 and 3 result in a Fay$$$ instance. By updating the runtime ~line 238 with:

// else if(base == "ptr" || base == "unknown")
    // return fayObj;
else if(base == "ptr")
    return fayObj;
else if(base == "unknown")
    return Fay$$fayToJs(["automatic"], fayObj);

Lines 2 and 3 result in the same output as 5 and 6.

If you could perhaps elaborate I would be very interested in trying to understand.

[ryachza]

P.S. I also would not be surprised if mapping "unknown" to "automatic" is too liberal. Perhaps there is some way to handle "unknown" somewhere in between "ptr" (literal) and "automatic" in that only obvious/common (knowable by instanceof, built-in) instances (like list/tuple) would be deserialized and everything else falls through.

[bergmark]

Ah , yes, sorry. I just wrote that off the top of my head so it was not very concrete.

Your suggested change breaks the mutableReference test case:

fay-tests --pattern mutableReference
Fay
  Runtime tests
    tests/mutableReference.hs: FAIL (1.03s)
      tests/mutableReference.hs
      expected: "Hai!\n"
       but got: "\n"

I've tried changing these lines in the past but haven't been able to do it without introducing regressions. I had a feeling there was another test case involved too, I'll think some more about it and see if I remember...

[ryachza]

Ah thank you for directing me to that. It seems that the reason that tests breaks is because then jsToFay was no longer an inverse. Updating jsToFay with:

  else if (base == "double" ||
           base == "bool" ||
           base ==  "ptr") {
    return jsObj;
  }
  else if(base == "unknown")
    return Fay$$jsToFay(["automatic"], jsObj);

Appears to pass mutableReference.hs as well as all other tests except for "serialization.hs" which seems to fail because the 'error "do not want"' function is now evaluated, but this actually seems semantically acceptable to me. If a consumer wanted a value to be passed through unevaluated that would be the purpose of Ptr. "serialization.hs" seems to pass if the type of printUnknown is updated to Ptr a -> Fay ().

[bergmark]

Thanks for looking into this!

I thought a bit more about it. I have a hunch that there may be some corner cases with undefined behavior that will change because of this, but I don't think that's reason enough to not change this. The test-suite is the best specification we have, and changing the serialization test seems fine, like you described.

Can you please send a pull request for this?

As for the impact of this change, does this mean users can remove most (or all?) occurrences of Automatic in their code?

[ryachza]

I believe all uses of Automatic should now be unnecessary. My understanding is that it served only as a flag to indicate the method of serialization, so Automatic a should now be equivalent to a.

My thought process regarding this change is that the FFI essentially has an implied (Serializable a) => constraint on the a, and my understanding is that all types should be serializable with the implementation of fayToJs handling the instance resolution. Previously in the absence of Automatic the only types that would be handled properly were those that had the same representation on both sides.

[bergmark]

Kind of fixed by #430, using Automatic or Ptr outside of an FFI declaration still does nothing, but I think we can close this.