Allow any file to make use of custom descriptor.proto

[jhump]

Before this PR, if an override version of the descriptor protos/options were in use, the file making use of them had to import "google/protobuf/descriptor.proto". But this is counter-intuitive since files typically do not import that file, unless they have explicit references to types therein or unless they define custom options. So this PR allows override descriptors to be used, even for files that have no such import statement.

Note that even with the new behavior, protocompile varies from protoc in how override descriptors are supported. This is intentional as protoc's behavior relies on the fact that it is single-threaded.

Behavior of protoc

With protoc, a file need merely define the relevant type (e.g. google.protobuf.FieldOptions) and be compiled before the file that needs it (meaning it appears before the file that needs it on the command line.

For example, let's say I have two files:

// test1.proto
syntax = "proto3";

message Foo {
    string name = 1 [some_new_option = "bar"];
}

// test2.proto
syntax = "proto3";
package google.protobuf;
message FieldOptions {
    string some_new_option = 100;
}

If I specify them in this order to protoc, it fails!

> protoc test1.proto test2.proto -o /dev/null
test1.proto:5:22: Option "some_new_option" unknown. Ensure that your proto definition file imports the proto which defines the option.

But if I reverse the order it works!

> protoc test2.proto test1.proto -o /dev/null

Because it is single-threaded, it compiles the files in order (though if a file imports another, it will recursively compile the imports first, even if they also appear later on the command-line). So after test2.proto is compiled, it is now aware of the override definition of google.protobuf.FieldOptions and then allows the some_new_option declaration in test1.proto.

Note that if test1.proto explicitly imported test2.proto, then it would work every time.

Behavior of protocompile

The compiler in this repo is inherently parallel. So it cannot behave like protoc, compiling the files sequentially in order to discover where override descriptors/options are defined.

Previously, the only way to use override descriptors/options was to explicitly import the definition. So, like the example above, it would work if test1.proto imported test2.proto. But it would always fail otherwise.

With this branch, it will now work without an explicit import, but only if the override descriptors/options are in a file named "google/protobuf/descriptor.proto". So we renamed "test2.proto" to "google/protobuf/descriptor.proto" in the above example, it will work!

This constraint (to name the file with override definition "google/protobuf/descriptor.proto") should be fine since it is likely that 100% of real world cases where descriptors are overridden, they are in a file named thusly -- usually a fork of the descriptor.proto, accompanied by a fork of a runtime and/or a code gen plugin.

[jhump]

We do this as a way to cheaply detect if descriptors are overridden. That way, if the file provided is not overridden, we can avoid the cost of wrapping the descriptor in a linker.File (which is not terribly expensive, but also not free since it must create an index of all symbols in the file).

[jhump]

@pkwarren, this is needed so that buf can provide an override version of descriptor.proto (such as the new v22.0 version, with new field option debug_redact) and it will get used even for files that do not import descriptor.proto.

cc @bufdev

[pkwarren]

Changes look good - only had a couple of questions. Thanks for providing all the context to make it easier to review.

[pkwarren]

I walked through this in the debugger and saw one non-nil error reported in the linker tests - search result for "google/protobuf/descriptor.proto" returned descriptor for "foo.proto". Is that expected?

[jhump]

Possibly. What test? If you look at the resolver it's using, it may be hard-coded to return a particular descriptor, regardless of the requested file name.

In any event, since this is not an explicit dependency, I don't think we should fail here if the resolver can't supply something. If the explicitly import this file, then it will fail as expected.

[pkwarren]

I'll see if I can gather more details. I agree with the behavior just wanted to make sure there wasn't something we were missing.