capnpc-go should not generate name collisions #46

zombiezen · 2016-08-22T05:04:25Z

One of the notable examples is in persistent.capnp that the annotation and the type collide. However, there are subtler cases, such as the struct field in schema.capnp should not collide with the Struct field of the wrapper struct.

The text was updated successfully, but these errors were encountered:

zenhack · 2016-09-17T21:06:01Z

Hope it's cool if I think out loud on this thread.

One obvious strategy: we need an injective function from legal capnproto idenfiers to legal go identifiers that don't collide with names that go-capnpc uses for helper functions (e.g. *_ServerToClient).

Facts we can potentially take advantage of:

capnproto has different case rules for different kinds of identifiers, e.g. I think annotations are the only thing that can appear at top-level in a schema file that can start with a lower case letter. This might be be the only case where we have to worry about upper-casing a capnproto name causing a collision with another capnproto name on its own.
underscores are legal in go names, but not in capnproto names. We already seem to use underscores as namespace seperators.
- Idea: if a name would otherwise collide with a go or go-capnpc name, append a single underscore. e.g. Struct becomes Struct_
Filesystems on MacOS and Windows are case-insensitive by default, so if someone tries to name two files e.g. web-session.capnp and webSession.capnp, they're going to hit other problems anyway. So maybe we can just punt on this and say "don't do that." $Go.name still exists if someone gets screwed over by someone else's schema.
There are some conventions that show up in sandstorm and the main repo re: what chars get used in filenames, so we can probably optimize at those, and for stuff that tends not to show up risk generating something ugly.

zenhack · 2016-09-17T21:13:06Z

For characters in filenames that don't constitute legal go package names, we can replace them with strings that start with a double underscore, e.g. c++.capnp generates a package named c__plus__plus. Since dash (-) shows up often in idiomatic filenames, we can maybe special case this to camelCasing, taking advantage of the filesystem point I made above (so websession.capnp generates package websession, and web-session.capnp generates package webSession).

zenhack · 2016-09-17T21:14:47Z

Obviously, a lot of these techniques are going to cause source-level breakage, don't know how you want to handle that.

zenhack · 2016-09-17T21:16:59Z

We could also just make go-capnpc use names with a trailing underscore, that don't otherwise conflict with go keywords. This would let us introduce new ones without worrying about disturbing the names of exising schema.

zenhack · 2016-09-17T21:19:50Z

One other thing that needs thinking about; what do we do about importing different schema with the same filename, and therefore package name? e.g. "/foo/mypackage.capnp" and "/foo/mypackage.capnp". I there's a similar question for dealing with #42, and an ideal solution would cover both.

zenhack · 2017-05-18T19:28:21Z

Maybe add this to the v3 milestone? Allowing it to be a breaking change opens up a lot of options.

This is a small step toward addressing the ideas in capnproto#46 > zombiezen commented on Aug 22, 2016: > > One of the notable examples is in persistent.capnp that the annotation and > the type collide. This is ready to show off because it shows the compiler error: ``` $ cd capnpc-go/test-regen $ capnp compile --no-standard-import -I../../std -o- persistent-simple.capnp | \ capnpc-go -promises=0 -schemas=0 -structstrings=0 $ # generated file: persistent-simple.capnp.go $ go build test_regen # compile the generated file ./persistent-simple.capnp.go:14:6: Persistent redeclared in this block ./persistent-simple.capnp.go:12:7: other declaration of Persistent ./persistent-simple.capnp.go:19:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:19:91: undefined: Persistent_call_Results_Future ./persistent-simple.capnp.go:39:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:47:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:53:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:63:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:69:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:73:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:167:38: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.go:73:9: too many errors ```

This is a small step toward addressing the ideas in capnproto#46 > zombiezen commented on Aug 22, 2016: > > One of the notable examples is in persistent.capnp that the annotation and > the type collide. Right now it fails when the golang compiler errors out with: ``` $ cd $(mktemp -d) $ capnpc-go -promises=0 -schemas=0 -structstrings=0 < testdata/persistent-simple.capnp.out $ go build persistent-simple.capnp.out.go ./persistent-simple.capnp.out.go:14:6: Persistent redeclared in this block ./persistent-simple.capnp.out.go:12:7: other declaration of Persistent ./persistent-simple.capnp.out.go:19:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:19:91: undefined: Persistent_call_Results_Future ./persistent-simple.capnp.out.go:39:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:47:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:53:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:63:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:69:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:73:9: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:167:38: Persistent (constant 10369543450650296259 of type uint64) is not a type ./persistent-simple.capnp.out.go:73:9: too many errors ```

Example: ``` interface Persistent { } annotation persistent(interface, field) :Void; ``` The annotation `persistent` generates the following go code (before this change): `const Persistent = uint64(0x...)` After this change, the generated code for an annotation always appends an underscore: `const Persistent_ = uint64(0x...)` Discussed on capnproto#46, capnproto/capnproto#323, as well as a $Go.name() to rename the annotation in this repo, which is essentially maintaining a fork of upstream capnp's persistent.capnp. This removes the $Go.name(), as a small step toward removing go-capnp's copies of capnp files.

This is a partial fix of capnproto#46 > zombiezen commented on Aug 22, 2016: > > One of the notable examples is in persistent.capnp that the annotation and > the type collide. Also see capnproto/capnproto#323 This commit changes the generated code for all annotations that start with a lowercase letter. Since the generated code in Go must start with an uppercase letter to be valid Go, this change appends an "_" to the name, regardless of whether there is a name collision already. (The thinking is that a name collision can happen in the future, and the renaming must be strictly an injective function, meaning that Go identifiers are always chosen the same regardless of other names. This way the codegen will not cause painful refactors in other code due to a name change later.) The capnp files in this repo under `std` all have $Go.name() annotations due to this bug. This removes one of them (std/capnp/persistent.capnp) and replaces it with a unit test that compiles the generated code. Actually doing a "go build" is one step father than previous unit tests. The thinking is to not add too many calls to "go build," since there is a lot of overhead and slow unit tests are discouraging. This commit appears to add about 250ms to the time it takes to run a "go test" in the capnpc-go subdir.

This is a partial fix of capnproto#46 > zombiezen commented on Aug 22, 2016: > > One of the notable examples is in persistent.capnp that the annotation and > the type collide. Also see capnproto/capnproto#323 This commit changes the generated code for all annotations that start with a lowercase letter. Since the generated code in Go must start with an uppercase letter to be valid Go, this change appends an "_" to the name, regardless of whether there is a name collision already. (The thinking is that a name collision can happen in the future, and the renaming must be strictly an injective function, meaning that Go identifiers are always chosen the same regardless of other names. This way the codegen will not cause painful refactors in other code due to a name change later.) The capnp files in this repo under `std` all have $Go.name() annotations due to this bug. This removes one of them (std/capnp/persistent.capnp) and replaces it with a unit test that compiles the generated code. Actually doing a "go build" is one step father than previous unit tests. The thinking is to not add too many calls to "go build," since there is a lot of overhead and slow unit tests are discouraging. This commit appears to add about 250ms to the time it takes to run a "go test" in the capnpc-go subdir. This also tweaks templates/interfaceClient to fix a problem if an interface is empty. The {{range .Methods -}} block in interfaceClient has a side effect of marking the "context" package for import, and an empty interface skips that. Since Resolve() also depends on the "context" package, this adds a call to {{$.G.Imports.Context}}.Context to ensure it also marks "context" as used.

This is a partial fix of #46 > zombiezen commented on Aug 22, 2016: > > One of the notable examples is in persistent.capnp that the annotation and > the type collide. Also see capnproto/capnproto#323 This commit changes the generated code for all annotations that start with a lowercase letter. Since the generated code in Go must start with an uppercase letter to be valid Go, this change appends an "_" to the name, regardless of whether there is a name collision already. (The thinking is that a name collision can happen in the future, and the renaming must be strictly an injective function, meaning that Go identifiers are always chosen the same regardless of other names. This way the codegen will not cause painful refactors in other code due to a name change later.) The capnp files in this repo under `std` all have $Go.name() annotations due to this bug. This removes one of them (std/capnp/persistent.capnp) and replaces it with a unit test that compiles the generated code. Actually doing a "go build" is one step father than previous unit tests. The thinking is to not add too many calls to "go build," since there is a lot of overhead and slow unit tests are discouraging. This commit appears to add about 250ms to the time it takes to run a "go test" in the capnpc-go subdir. This also tweaks templates/interfaceClient to fix a problem if an interface is empty. The {{range .Methods -}} block in interfaceClient has a side effect of marking the "context" package for import, and an empty interface skips that. Since Resolve() also depends on the "context" package, this adds a call to {{$.G.Imports.Context}}.Context to ensure it also marks "context" as used.

This is a partial fix of capnproto#46 > zenhack commented on Sep 17, 2016: > > if a name would otherwise collide with a go or go-capnpc name, append > a single underscore. e.g. `Struct` becomes `Struct_` Also see capnproto/capnproto#323 This commit changes the generated getter and setter methods for struct fields. If the field name would collide with code generated for any struct such as `Method()` or `String()`, the field getter becomes `Method_()` and the setter becomes `SetMethod_()`. Technically only `Method_()` would be necessary, but who wants to maintain the mental state? If the getter changes, the setter also changing is just simpler conceptually. The capnp files in this repo under `std` have $Go.name() annotations due to capnproto#46. This removes one of them (std/capnp/compat/json.capnp) and updates the unit tests for code coverage testing purposes.

This is a partial fix of #46 > zenhack commented on Sep 17, 2016: > > if a name would otherwise collide with a go or go-capnpc name, append > a single underscore. e.g. `Struct` becomes `Struct_` Also see capnproto/capnproto#323 This commit changes the generated getter and setter methods for struct fields. If the field name would collide with code generated for any struct such as `Method()` or `String()`, the field getter becomes `Method_()` and the setter becomes `SetMethod_()`. Technically only `Method_()` would be necessary, but who wants to maintain the mental state? If the getter changes, the setter also changing is just simpler conceptually. The capnp files in this repo under `std` have $Go.name() annotations due to #46. This removes one of them (std/capnp/compat/json.capnp) and updates the unit tests for code coverage testing purposes.

davidhubbard · 2023-08-11T02:36:13Z

With pull request #542 the capnproto files in std are up-to-date but they still have slight changes from upstream:

They all have $Go.package() and $Go.import() at the bottom
std/capnp/schema.capnp still uses $Go.name() to rename the three instances of "struct," as otherwise they would become "Struct" in the generated code which would not compile.

Thinking out loud here, it makes sense to eventually remove these duplicate copies of the capnproto files and use the vanilla unmodified copies directly from upstream. Seems like #47 is where that final goal can be reached.

If we remove the $Go.name() renames, I think this issue is completely fixed.

zombiezen added the bug label Aug 22, 2016

zenhack mentioned this issue Aug 23, 2016

capnpc-go: deal with subtyping/inheritance in capnp interfaces #42

Open

zombiezen referenced this issue in abraithwaite/go-capnproto2 Oct 11, 2016

patch

6c5f40c

zombiezen added this to the 3.0 milestone May 18, 2017

zombiezen added the help wanted label Oct 18, 2017

zombiezen mentioned this issue Feb 12, 2018

Collisions between struct member names and generated Go code #108

Closed

zenhack mentioned this issue Jan 9, 2023

Generated code does not compile when a field is named "message" or "segment" #419

Closed

zenhack mentioned this issue Feb 20, 2023

Error if attribute name "message" is used in a capnp struct definition #457

Closed

lthibault assigned davidhubbard Aug 1, 2023

davidhubbard mentioned this issue Aug 7, 2023

Annotations generate a name collision - fixed #539

Merged

davidhubbard mentioned this issue Aug 8, 2023

Fix field names that collide with generated names #46 #541

Merged

lthibault mentioned this issue Sep 5, 2023

Generator can create duplicate RegisterSchema functions #490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

capnpc-go should not generate name collisions #46

capnpc-go should not generate name collisions #46

zombiezen commented Aug 22, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016 •

edited

Loading

zenhack commented Sep 17, 2016

zenhack commented May 18, 2017

davidhubbard commented Aug 11, 2023

capnpc-go should not generate name collisions #46

capnpc-go should not generate name collisions #46

Comments

zombiezen commented Aug 22, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016

zenhack commented Sep 17, 2016 • edited Loading

zenhack commented Sep 17, 2016

zenhack commented May 18, 2017

davidhubbard commented Aug 11, 2023

zenhack commented Sep 17, 2016 •

edited

Loading