search

200sc / bebop

bebop wire format in Go
Apache License 2.0
69 stars 5 forks source link

Import support #8

Closed msackman closed 3 years ago

msackman commented 3 years ago

I note in the README the statement about imports being not supported. It looks like upstream they were added a few months ago. I'm just wodering whether you're already planning on supporting this, or whether it's stalled for now? You mention the need for careful design and thought on this feature - I quite understand. The main thing to me seems the need to figure out the module import path from relative imports (assuming that you essentially go down the route of one bop file == one go file). I'm keen to discuss your thoughts around this - I may very well be keen to contribute shortly.

200sc commented 3 years ago

I must have missed when this feature was added to the upstream implementation-- Yes we absolutely want to support it, I'll need to review what they implemented before coming up with ideas for how it should work over here.

200sc commented 3 years ago

It looks like they also snuck in support for constant declarations in that same push (https://github.com/RainwayApp/bebop/wiki/Writing-Bops%3A-The-Bebop-Schema-Language#const), I'll open an issue for that.

msackman commented 3 years ago

Cool. Very happy to leave you to it if you already know how you'd want to do it. But happy to help if you'd like.

200sc commented 3 years ago

It looks like only relative imports are supported, like you said. My thoughts so far are:

  1. Just combine everything into one file, unfortunately, because relative imports aren't supported in Go or
  2. Accept meaningful comments that would be ignored by another compiler but would be used to determine the module path in Go-- or a predetermined constant name, like go_package. This is similar to how protobuf works in Go e.g. option go_package = "example.com/project/protos/fizz"; (link). The issue with using a constant would be that there's a naming collision possibility-- someone names their const go_package = "go!" and now we can't compile that file, but maybe that's unlikely enough that it doesn't matter.
  3. Some combination of the two, offer either mode, toggled via a compilation flag.
msackman commented 3 years ago

(1) seems initially attractive because it's simple. But, down the road it causes all manner of problems. You've probably split up your protocol into different .bop files because you have some common definitions and then you're reusing them for different audiences. But with (1), if you decide to compile for those different audiences then you'll end up with multiple types-in-go for the same type-in-bop. Provided the opcodes are the same, this might not be a problem, but it would always be odd.

So, I reckon (2) is the right way to go; plus the weight of prior art here (not just protobuf but capnproto does the same too). One bop file == one go file. So then for imports in bop, all you need to do is open the imported file and find the special go_package const and that just tells you how to import that for the current bop file translation. So you just assume that anything imported is already translated (or will be). So then your cli might drop the -o option completely, and maybe drops -i too and you just use all non-flag args to be the files to translate (and maybe have a -r option to recurse through directories).

(3) sounds a little terrifying to convey to the user exactly what the semantics are. I always feel a simple model for the user to understand is better.

Just my 2p :)

msackman commented 3 years ago

oh, one question that I don't think is clear from the upstream: a.bop defines Foo b.bop imports a.bop and defines Bar c.bop imports b.bop. Does c.bop have access to Foo? I.e. does a bop file export only the types it directly defines, or does it also export everything it imported?

200sc commented 3 years ago

I'll do some kind of compatability testing to see if imports add to what a file itself exports, and work on the magic const implementation. That requires that consts are implemented, which I'm working towards (#11).

200sc commented 3 years ago

I can't figure out how to make upstream bebopc produce more than one file, given an import, so to match exactly what they do we would just put everything in one file.

This means that, yes, a imports b imports c means that the generated code will contain all three files and imports do cause everything imported to be exported in that new file.

These decisions seem problematic in a sufficiently complex system. We can support this mode of operation as well as modes that split by file and are more restrictive with import/exports as optional settings, I imagine.

msackman commented 3 years ago

Interesting discoveries. Yeah, it does seem shortsighted to me, but, I can also understand the desire to copy the upstream tool too.