go_features.proto has incorrect package and error-prone file path

[jhump]

The file go_features.proto indicates its package is google.protobuf. However, the other features (cpp_features.proto and java_features.proto, both in the main protobuf repo) use the shortened package pb.

Also, the actual canonical import path of the file needs to be more clear -- if users don't import the file using the same path used to compile the file, then reflection won't work correctly: the import path is used to find the imported file in the global registry (protoregistry.GlobalFiles), so the import path must match how the file is actually registered there. In this case, the file is (rather non-intuitively) registered as reflect/protodesc/proto/go_features.proto. (See this doc for more details on why the path matters so much.)

I would have expected the import path to instead be "google/protobuf/go_features.proto", using the "google/protobuf" import path typical of source files related to Protobuf and authored by Google. However, the location of the file in this repo doesn't lend itself to that, and it also doesn't lend itself to its actual path, using the "reflect/protodesc/proto" import path. Since the file seems to only be distributed as part of this Go module, users will likely find themselves importing it as "types/gofeaturespb/go_features.proto", perhaps with some other prefix, depending on how they choose to vendor/copy the file into their source tree (a prefix like "protobuf" or even "go/src/google.golang.org/protobuf").

[neild]

There are three different elements here (actually four, of which you mentioned three):

• the protobuf package name;
• the Go package import path;
• the Go package name; and
• the file name of the source file.

google.protobuf seems like a good name for the protobuf package, since this is already well-established as being under the ownership of the protobuf project. I don't know why cpp_features.proto and java_features.proto are using pb; that doesn't seem like a good name at all. Possibly these are intended to be strictly internal to the code generator, in which case the name doesn't matter? It doesn't seem likely that these will remain purely internal, though. Someone should probably open an issue in the appropriate tracker to have the name changed to something better.

It looks like when go_features.proto moved from reflect/protodesc/proto (initial home, never appeared in any release AFAIK), we forgot to update the regeneration script to operate on the new location, so the generated package has the wrong name (proto, rather than gofeaturespb) and its descriptor references the old location. We should fix that and regenerate the .pb.go.

The file name is also stale ("reflect/protodesc/proto/go_features.proto"), but simply regenerating the file isn't enough. Doing so would set the name to "types/gofeaturespb/go_features.proto", which is insufficiently qualified.

The file names for the C++ and Java files are "protobuf/compiler/cpp/cpp_features.proto" and "protobuf/compiler/java/java_features.proto". Using the same pattern for Go seems reasonable. (Perhaps all three should be under "google/protobuf" instead.)

I think the clearest way to handle this is to create a new top-level directory in the https://go.googlesource.com/protobuf repo to hold public .proto files. Possibly src (short, matches the main proto repo structure), moving the file to src/protobuf/compiler/go/go_features.proto.

[jhump]

google.protobuf seems like a good name for the protobuf package, since this is already well-established as being under the ownership of the protobuf project. I don't know why cpp_features.proto and java_features.proto are using pb; that doesn't seem like a good name at all.

This was a decision by the Protobuf team for Editions. Features are likely to appear in greater numbers in files that are "prototilled" (auto-converted from proto2 or proto3 to editions), and "pb" is a much more concise but still readable name. I was told that the internal version of go_features.proto actually uses "pb" as the package name and that I could file a bug since it is likely a Copybara misconfiguration that changed the package in the GitHub version. Cc @mkruskal-google.

The file names for the C++ and Java files are "protobuf/compiler/cpp/cpp_features.proto" and "protobuf/compiler/java/java_features.proto"

I don't think this is accurate. From what I can see in the main protobuf repo, they use "google/protobuf/cpp_features.proto" and "google/protobuf/java_features.proto".

[neild]

I don't think this is accurate. From what I can see in the main protobuf repo, they use "google/protobuf/cpp_features.proto" and "google/protobuf/java_features.proto".

I could swear that I was seeing protobuf/compiler/(lang)/lang_features.proto somewhere, and now I can't find what I was looking at. You're right, these seem to be under google/protobuf.

[lfolger]

Possibly these are intended to be strictly internal to the code generator, in which case the name doesn't matter?

I don't thik so because if you want to use the features defined in these, you need to import these files and use the types deffined in them. But I agree that these names seem suboptimal.

I think the clearest way to handle this is to create a new top-level directory in the https://go.googlesource.com/protobuf repo to hold public .proto files. Possibly src (short, matches the main proto repo structure), moving the file to src/protobuf/compiler/go/go_features.proto.

Thanks for the input. I agree and prepared https://go-review.googlesource.com/c/protobuf/+/579975 to address this.

[lfolger]

The change was submitted. This should address all of your concern. If not, feel free to reopen this issue.