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

protoc-gen-go: cannot compile package when import public example.proto in is present in two or more files #810

Open
btc opened this issue Mar 1, 2019 · 11 comments
Labels

Comments

@btc
Copy link

btc commented Mar 1, 2019

There is a slightly unexpected behavior stemming from the use of import public.

When two proto files in a single package both contain public imports of the same dependency, the resulting package cannot be compiled.

What version of protobuf and what language are you using?

Version: v3.6.1

What did you do?

  1. create lib/lib.proto
  2. create api/foo.proto and include import public "lib/lib.proto";
  3. create api/bar.proto and include import public "lib/lib.proto";

What did you expect to see?

One would expect for the code generation to include at most one type-alias per package for a given public/aliased type.

type LibType = lib.LibType

What did you see instead?

api/foo.pb.go:

type LibType = lib.LibType

api/bar.pb.go:

type LibType = lib.LibType

LibType redeclared in bar.pb.go
previous declaration at foo.pb.go

proposal

generate the type aliases for all types in a given public import in a separate file. this way, the aliases will be present at most once per go package.

@dsnet
Copy link
Member

dsnet commented Mar 1, 2019

\cc @neild. I vaguely remember that we addressed this in the v2 re-write of protoc-gen-go.

@dsnet dsnet changed the title using import public m.proto in two files results in a go package that cannot be compiled protoc-gen-go: using import public m.proto in two files results in a go package that cannot be compiled Mar 1, 2019
@btc btc changed the title protoc-gen-go: using import public m.proto in two files results in a go package that cannot be compiled cannot compile package when import public example.proto in is present in two files Mar 1, 2019
@btc btc changed the title cannot compile package when import public example.proto in is present in two files cannot compile package when import public example.proto in is present in two or more files Mar 1, 2019
@neild
Copy link
Contributor

neild commented Mar 1, 2019

We did not, alas.

This is, unfortunately, quite difficult to fix. The problem is that we generate each file in a package independently from every other file in the package. If a file includes a public import, we generate forwarding declarations for that import. If two files in the same package include the same public import, each one includes the same forwarding declarations and we have a problem.

The fact that files are independent of each other is a rather nice property: It means that you can run protoc on each source .proto individually without worrying about how they are collected into Go packages. It also means that changes to one .proto file never result in changed output for a different .pb.go file.

We could fix this problem by only generating the forwarding declarations in one of the .pb.go files of a Go package. This would require that you generate all the files of the package in the same protoc invocation (and breaking the above rather nice property).

Or we can say that you should only import public in one of the .proto source files, which is the current state of affairs. This would be my preference, although it is a rather unfortunate limitation.

@dsnet
Copy link
Member

dsnet commented Mar 1, 2019

Just as a sanity check, 1) what does C++ do? and 2) do they even support this? I ask because my understanding is that C++ also has the per-file property that we cherish as well, right?

@neild
Copy link
Contributor

neild commented Mar 1, 2019

C++ has the advantage that the protobuf language and its namespace rules map precisely onto C++. I believe import public in C++ just means "put a #include in the .pb.h file".

@neild
Copy link
Contributor

neild commented Mar 1, 2019

Java might be the more interesting case.

@btc
Copy link
Author

btc commented Mar 1, 2019

Would it break any existing contracts or invariants if the plugin were to place all aliases for a given dependency in a single separate file. This file would be identified (named) by its corresponding public import path.

For the example above, it would mean that the aliases for lib/lib.proto would reside in api/lib_lib.aliases.pb.go.

Here, I included a path component aliases to avoid naming collisions with potential messages which might result in lib_lib.pb.go.

This way, it exists once per package regardless of the number of proto files included in generation.

@neild
Copy link
Contributor

neild commented Mar 1, 2019

Would it break any existing contracts or invariants if the plugin were to place all aliases for a given dependency in a single separate file. This file would be identified (named) by its corresponding public import path.

Interesting thought. Not very friendly to build systems that want predictable outputs from each compilation action, though. I'll have to think on that.

@btc
Copy link
Author

btc commented Mar 1, 2019

Would it break any existing contracts or invariants if the plugin were to place all aliases for a given dependency in a single separate file. This file would be identified (named) by its corresponding public import path.

Interesting thought. Not very friendly to build systems that want predictable outputs from each compilation action, though. I'll have to think on that.

That's fair.

@neild

What if the plugin were to standardize such that there is always a file called package.go in every generated package. In most cases, and at the very least, this file would contain a top-level Go package comment. But in addition, all type-aliases would be emitted there, in the case there are aliases to emit.

It would be somewhat conventional, and would succeed in providing predictable output for build systems.

// package api is a foobar
package api

type LibType = lib.LibType

@neild
Copy link
Contributor

neild commented Mar 1, 2019

Putting everything in a single package.go doesn't really help, unfortunately; if a.proto and b.proto each have a public import of a different file, then compiling one of them will clobber the package.go for the other. And if you require that they be compiled at the same time, then there isn't really any benefit to introducing a new file.

I think the problems with build system integration rule out per-import files (e.g., foo.aliases.pb.go). It's just too useful to be able to predict the output .pb.go files from the names of the inputs, rather than needing to poke into their contents.

The most practical answers I see so far are:

  • Document that we only support one import public of a given file per Go package. Unfortunate, but hopefully not too limiting.
  • When compiling multiple .proto files in the same Go package that publicly import the same file, only generate the forwarding aliases in one of the output files. Conflicts are resolved, but only if you supply all the sources for a Go package in the same protoc action.

@btc
Copy link
Author

btc commented Mar 1, 2019

Putting everything in a single package.go doesn't really help, unfortunately; if a.proto and b.proto each have a public import of a different file, then compiling one of them will clobber the package.go for the other. And if you require that they be compiled at the same time, then there isn't really any benefit to introducing a new file.

Ah, yes. I missed that case.

@dsnet
Copy link
Member

dsnet commented Mar 1, 2019

Conflicts are resolved, but only if you supply all the sources for a Go package in the same protoc action

This seems reasonable to me since protoc already has additional logic to that distinguishes between whether it were invoked on a single file or a set.

For example, consider the following files:

//a.proto
syntax = "proto2";
package foo;
message Foo{}
//b.proto
syntax = "proto2";
package foo;
message Foo{}

Compiling each one individually results in no complaint from protoc. However, compiling them together does report a problem:

$ protoc --go_out=. a.proto b.proto
b.proto:5:9: "foo.Foo" is already defined in file "a.proto".

@dsnet dsnet changed the title cannot compile package when import public example.proto in is present in two or more files protoc-gen-go: cannot compile package when import public example.proto in is present in two or more files Apr 2, 2019
@dsnet dsnet added the bug label Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants