Skip to content
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

error building purescript-dotlang #62

Closed
joprice opened this issue Oct 2, 2020 · 16 comments
Closed

error building purescript-dotlang #62

joprice opened this issue Oct 2, 2020 · 16 comments
Labels
bug C++ Applies to the C++11 target Golang Applies to the Go target

Comments

@joprice
Copy link

joprice commented Oct 2, 2020

I'm trying to add https://github.com/csicar/purescript-dotlang as a dependency, and I get the following error:

go: finding module for package project.localhost/purescript-native/output/Color.1
output/Data.DotLang.Attr.Edge/Data_DotLang_Attr_Edge.go:11:5: cannot find module providing package project.localhost/purescript-native/output/Color.1: unrecognized import path "project.localhost/purescript-native/output/Color.1": https fetch: Get "https://project.localhost/purescript-native/output/Color.1?go-get=1": dial tcp: lookup project.localhost: no such host
psgo: callProcess: go "build" "project.localhost/purescript-native/output/Main" (exit 1): failed

Checking the file output/Data.DotLang.Attr.Edge/Data_DotLang_Attr_Edge.go, I see that the "colors" project is referenced as "project.localhost/purescript-native/output/Color.1", but it exists under the folder Color: output/Color/Color.go. I'm not very familiar with go modules. Is there a reason the suffix .1 is being attached to the module name?

@joprice
Copy link
Author

joprice commented Oct 2, 2020

I'm guessing it has something to do with this section

renameImports :: [Ident] -> [(Ann, ModuleName)] -> M.Map ModuleName (Ann, ModuleName)
renameImports = go M.empty
where
go :: M.Map ModuleName (Ann, ModuleName) -> [Ident] -> [(Ann, ModuleName)] -> M.Map ModuleName (Ann, ModuleName)
go acc used ((ann, mn') : mns') =
let mni = Ident $ runModuleName mn'
in if mn' /= mn && mni `elem` used
then let newName = freshModuleName 1 mn' used
in go (M.insert mn' (ann, newName) acc) (Ident (runModuleName newName) : used) mns'
else go (M.insert mn' (ann, mn') acc) used mns'
go acc _ [] = acc
freshModuleName :: Integer -> ModuleName -> [Ident] -> ModuleName
freshModuleName i mn'@(ModuleName pns) used =
let newName = ModuleName $ init pns ++ [ProperName $ runProperName (last pns) <> "_" <> T.pack (show i)]
in if Ident (runModuleName newName) `elem` used
then freshModuleName (i + 1) mn' used
else newName

@joprice
Copy link
Author

joprice commented Oct 2, 2020

There seems to be a related issue with functions with multiple definitions:

# project.localhost/purescript-native/output/Color
output/Color/Color.go:259:25: s redeclared in this block
	previous declaration at output/Color/Color.go:251:25
output/Color/Color.go:469:13: s redeclared in this block
	previous declaration at output/Color/Color.go:454:13

The original function:

hsva :: Number  Number  Number  Number  Color
hsva h s   0.0 a = hsla h (s / (2.0 - s)) 0.0 a
hsva h 0.0 1.0 a = hsla h 0.0 1.0 a
hsva h s'  v'  a = hsla h s l a
  where
    tmp = (2.0 - s') * v'
    s = s' * v' / (if tmp < 1.0 then tmp else 2.0 - tmp)
    l = tmp / 2.0

The generated code with the lines with variable re-declarations marked:

 func PS__hsva() Any {
      return func(h Any) Any {
          return func(v Any) Any {
              return func(v1 Any) Any {
                  return func(a Any) Any {
>>                    var s Any = v
                      if v1 == 0.0 {
                          return Apply(PS__hsla(), h, s.(float64) / (2.0 - s.(float64)), 0.0, a)
                      }
                      if v == 0.0 && v1 == 1.0 {
                          return Apply(PS__hsla(), h, 0.0, 1.0, a)
                      }
                      var tmp Any = (2.0 - v.(float64)) * v1.(float64)
>>                    var s Any = (v.(float64) * v1.(float64)) / Run(func() Any {
                          var ᵗ25 Any = tmp.(float64) < 1.0
                          if ᵗ25 == true {
                              return tmp
                          }
                          return 2.0 - tmp.(float64)
                      }).(float64)
                      var l Any = tmp.(float64) / 2.0
                      return Apply(PS__hsla(), h, s, l, a)
                  }
              }
          }
      }
  }

@andyarvanitis andyarvanitis added bug Golang Applies to the Go target labels Oct 2, 2020
@andyarvanitis
Copy link
Owner

I found the second issue – it's caused by an inliner function provided directly by the main purescript compiler, which is surprising.

For the first issue, which version of dotlang are you using? It's not in the standard package set, and isn't obvious from the releases page which version one would typically use. Any other info you can provide on what you're building would help too.

@joprice
Copy link
Author

joprice commented Oct 2, 2020

I'm using version 2, which the following in my packages.dhall:

  let additions =
      { dotlang =
          { dependencies =
              [ "colors"
              , "console"
              , "effect"
              , "generics-rep"
              , "prelude"
              , "psci-support"
              , "strings"
              , "test-unit"
              ]
          , repo = "https://github.com/csicar/purescript-dotlang.git"
          , version = "v2.0.0"
          }
      }

  in  upstream // overrides // additions

@andyarvanitis
Copy link
Owner

I looked at it a bit more. The two issues are unrelated. The second one comes from the standard javascript transcompilation behavior: javascript tolerates multiple var s declarations but go doesn't. I have a fix that just needs some testing.

The first issue is also related to behavior inherited from the javascript backend, where a collision with the Color identifier is handled by appending "_1". However, the javascript backend is still doing the importing (require) correctly with the unmodified name. So I need to fix the go backend to either do the same, or change the renaming scheme a bit to handle it.

@joprice
Copy link
Author

joprice commented Oct 3, 2020

I ran the same test with the cpp backend and hit the same duplicate definition issue. I see the golang label you added, so wanted to make sure the change could encompass both backends. I'm not sure how much of the code is common between them.

@andyarvanitis
Copy link
Owner

Good point, thanks

@andyarvanitis andyarvanitis added the C++ Applies to the C++11 target label Oct 3, 2020
@andyarvanitis
Copy link
Owner

Out of mostly curiosity, do you plan on using the C++ backend as well as the golang one? Go gets a bit more attention these days, and tbh I'm generally recommending it to people who don't have specific C++ interop needs. Its garbage collector generally works better (faster) than std::shared_ptr reference counting (and has no concerns with retain cycles), and golang compilation is really fast.

@joprice
Copy link
Author

joprice commented Oct 4, 2020

I was experimenting with each based on specific libraries that I wanted to wrap. I could try to use only the go backend, as there are probably go wrappers for most cpp projects that I would intend to wrap with purescript, but I have heard that go's ffi is slow, and having two layers of indirection would make debugging and analyzing performance more difficult.

As for the issues with shared ptr, have you considered using a gc like boehm?

I've also seen a suggestion somewhere to alloc without freeing, in short lived programs, where the assumption is that the os will free the memory on completion and the total memory used is small enough that reclaiming it is not necessary and might even be unnecessary overhead.

@andyarvanitis
Copy link
Owner

Ok, thanks for the info, and for the bug reports.

Yes, I used to officially support Boehm as a build option when it was pure11. I stopped supporting it for a few reasons, but once in a while I look to see if any new GCs come out that would be interesting options.

Yeah, alloc without freeing is also something that could be explored for the C++ backend, and can already be done with golang because they support turning off the gc (though I haven't tried it). I've also played around a bit with alternative allocators like jemalloc and saw some interesting results.

@joprice
Copy link
Author

joprice commented Oct 4, 2020

I tried your branch and a dotlang example to compile! I just had to add one more function to the ffi:

 mathExports := Foreign("Math")
 mathExports["round"] = func(x_ Any) Any {
   x, _ := x_.(float64)
   return math.Round(x)
 }

Should I pr it to that project?

@andyarvanitis
Copy link
Owner

Sure, thanks!

@joprice
Copy link
Author

joprice commented Oct 4, 2020

Pr up here andyarvanitis/purescript-native-go-ffi#14

Interesting, I'll play around with the go flags and jemalloc as well. Do you have any notes on the difference between the cpp and go branches? Would it just be a matter of tedious work to merge the two so changes that apply to both are easier to get merged, or is there some reason they have to be separate?

@andyarvanitis
Copy link
Owner

Merged, thanks.

I don't have any such notes, unfortunately. I had (re)structured the code a while back to be able to start reusing as much as possible between the two, but just didn't have the time to actually get there. I do at least try to maintain as similar a structure between them as possible, to keep the option open.

@joprice
Copy link
Author

joprice commented Oct 4, 2020

I could spend some time on that if you can give me some pointers.

@andyarvanitis
Copy link
Owner

Closing, since I believe this has been fully addressed, but feel free to reopen if you find any problems with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug C++ Applies to the C++11 target Golang Applies to the Go target
Projects
None yet
Development

No branches or pull requests

2 participants