andyarvanitis / purescript-native

A native compiler backend for PureScript (via C++ or Golang)
http://purescript.org
Other
632 stars 40 forks source link

ffi array type interop with opaque types #64

Open joprice opened 4 years ago

joprice commented 4 years ago

I was trying to return an array from a function and use it with common Foldable and Traversable functions, and hit a wall of silent failures. Luckily, since the build doesn't detect changes in the output directory, I was able to throw some printlns in there and rebuild without it being overwritten. I found that casts in functions like Data.Array.length like

 xs, _ := xs_.([]Any)

return an empty array when the cast to interface fails.

To fix this, the type cannot simply be assigned to interface:

cannot use files (variable of type []os.FileInfo) as []interface{} value in variable declaration

so it needs to be copied, as this go wiki page points out:

https://github.com/golang/go/wiki/InterfaceSlice#what-can-i-do-instead

I'm wondering whether there's any way to get around this. Maybe define Traversable instances for an opaque type? I haven't tried this and not sure how much boilerplate it would require, but if there is some boilerplate on the go ffi side, maybe it could be provided in the small runtime library where Fn1-10 reside.

Below is the ffi function I ended up with that copies the array:

exports["readDir'"] = func(left Any) Any {
      return func(right Any) Any {
        return func(file_ Any) Any {
          file := file_.(string)
          return func() Any {
            files, err := ioutil.ReadDir(file)
            if err == nil {
              var f []interface{} = make([]interface{}, len(files))
              for i, d := range files {
                f[i] = d
              }
              return Apply(right, f)
            } else {
              return Apply(left, err.Error())
            }
          }
        }
      }
    }
andyarvanitis commented 4 years ago

When doing a type assertion (cast) in go, if you don't provide a (discarded) failure bool variable it will panic (with a description) if it fails. In other words, you would do this:

 xs := xs_.([]Any)

There's no current way to do it from spago, but if you do the go build using the debug tag you will more information:

$ spago build
$ go build -tags debug output/Main/Main.go
$ ./Main

(you could also just do go run -tags...)

Let me know if that helps with your debugging.

joprice commented 4 years ago

I see a lot of casts in the ffi definitions using the two-result value variant. There's a note in the readme on removing this once you're confident of the types. Is this something that should be cleaned up, and is there some overhead?

andyarvanitis commented 4 years ago

The idea was to end up with the (allegedly faster) two-result (ignored failure) assertions in ffi implementations. In my readme I intended to convey that an author should use the debug-friendly (one-result) version while testing. I say "allegedly faster" because TBH I don't know if in recent versions of golang the non-panic assertions are still faster.

Providing debug and release versions of the files, like I do in the runtime, would have been nice, but I thought it would be too onerous for contributors.

joprice commented 4 years ago

Ah I misunderstood. I probably read it too fast while frantically debugging.

Writing and maintaining two files is definitely error prone. I guess you could provide a few casting helper functions that are generated in debug or release mode based on a flag and the ffi could target those. Or there could be a preprocessor step. Also, couldn't the runtime be generated, since everything is built from source anyway? Is there a benefit to have it as a separate library?

andyarvanitis commented 4 years ago

Well, if I end up getting a definitive answer that the panics don't cause any overhead I would just get rid of the two-result versions everywhere.

I'd like to avoid any kind of preprocessor and keep things simple and fast-building.

Benefit to separate runtime library is simply cleaner and easier maintenance. It's not set in stone, though.

joprice commented 3 years ago

It looks like you've gone with using the checked versions https://github.com/andyarvanitis/purescript-native-go-ffi/commit/8512b6529f13b551c190e4ac8698808a67d6ecf0, so that answers the silent failure part of my question.

The other part was whether boxing arrays into interface{} is avoidable, and if not whether the helper library should have a helper function to make that easier for writers of ffi code.

Having to re-allocate arrays to box them is not great in my opinion, even if it is only a constant factor difference, since it immediately adds overhead to using purescript-native over writing the code in go. This can lead people to push more logic into the ffi for performance reasons, getting less use our of purescript's abstractions.

andyarvanitis commented 3 years ago

I don't think there's a way around arrays needing to be of type []Any, short of using opaque types in places, as you mention. A helper function for boxing arrays would indeed be nice, but go doesn't have generics.