linker: support editions in linker descriptors

[lfolger]

Open questions:

• Is there a better way to resolve the options. At the moment we clone the parse result which feels extremely expensive.
• Is generating the full google.golang.org/protobuf descriptors in the linker a performance issue?
• Can we get rid of the noOpDescriptors completely or are there cases where the google.golang.org/protobuf descriptor validation is stricter than what protocompile allows?

This is very much work in progress and I would like to have feedback on how to move forward.

This commit does not yet work as it requires an update to google.golang.org/protobuf. Once we agreed on the direction of this change, I'm going to make the necessary changes to google.golang.org/protobuf.

[CLAassistant]

All committers have signed the CLA.

[lfolger]

@jhump could you take a look and let me know what you think?

[jhump]

@lfolger, maybe we can schedule a chat? I already have a lot of the work to support editions properly stubbed out. I was mostly awaiting the core runtime to be updated with changes to the interfaces so I can implement them here. My plan was to resurrect a lot of the code that I removed in #191, in order to implement new FeatureSet-related methods that I expect to be added to protoreflect.Descriptor.

Is there a better way to resolve the options. At the moment we clone the parse result which feels extremely expensive.

I don't understand the necessity for cloning. The compiler already clones parse result and/or descriptor proto if necessary, in (*task).asParseResult.

Is generating the full google.golang.org/protobuf descriptors in the linker a performance issue?

Almost certainly. The process of building those will duplicate a decent bit of the work the linker has done and also allocate another full copy of the descriptor hierarchy.

Can we get rid of the noOpDescriptors completely or are there cases where the google.golang.org/protobuf descriptor validation is stricter than what protocompile allows?

We can't get rid of the embedded interfaces, to make sure this module can compile in the face of future interface additions to the protoreflect package. These implementations are unused, and are only present so that if methods are in fact added in the future, a user calling them won't immediately panic (see #198 for context). Admittedly, the no-op values will likely return an incorrect value (which could possibly cause their code to panic subsequently since the protoreflect APIs are quite panic-proone and not very friendly ). The thinking was that any issues introduced this way would be short-lived bugs until the repo is updated with any new descriptor methods, and a logic defect is likely better for users than a panic bug.

And we can't just use "real" implementations instead of the no-op ones, like this PR proposes, because there is a key case where google.golang.org/protobuf descriptor validation is stricter: that package performs various checks on options to detect if things are valid. But at this point in time in the compilation process, we haven't interpreted options yet. However, we need a full implementation of protoreflect.Descriptor interfaces in order to use dynamicpb, which is needed to interpret the options. It's kind of a chicken-egg thing.

[lfolger]

Happy to schedule a chat.

I prepared this pull request to find out what we (Google) need to support the use cases we have in Google. I'm happy for alternative solutions that cover the same use cases.
I tested this against the Google corpus and this worked. This doesn't mean it is the only solution but just one that I found to be working.

[jhump]

@lfolger, I think #260 should do the trick. It implements feature resolution with defaults based on the version of google/protobuf/descriptor.proto that is baked into the generated code in google.golang.org/protobuf/types/descriptorpb. The resolved features are used to implement FieldDescriptor.HasPresence, FieldDescriptor.IsPacked, and EnumDescriptor.IsClosed in this repo's implementations of those descriptor interfaces.

Note that branch also adds a method to its FileDescriptor implementation, one that is not in the base interface:

Edition() descriptorpb.Edition

Ideally, the protobuf-go runtime, the reflect/protodesc package in particular, would accept an implementation of protoreflect.FileDescriptor that provides this method (instead of type-asserting the file descriptor to an internal type, to extract a value from an internal/unexported field).

[lfolger]

Thanks for the prompt response.

Also thanks for adding the Edition() function. I think the signature of that function needs to change because we need to implement it in protobuf/internal/filedesc which cannot depend on the descriptor proto for bootstrapping reasons.

I think the best alternative is to return and int32. I'm not a big fan that it lacks the type information but unless we want to introduce an extra package public package that defines these editions as constants, I don't think there is a much better way.

[jhump]

I think the best alternative is to return and int32.

Yep, that totally makes sense that you don't want a dependency on descriptorpb: aside from dependency bloat, that could trivially lead to import cycle problems. I just pushed a commit to change it to int32.

[jhump]

@lfolger, any chance you can easily test that other PR, to verify that it resolves any issues with protocompile in your codebase and corpus of protos?

[lfolger]

Let me give this a try.

It might not be able to report results back today though.

[jhump]

Let me give this a try.

Thank you!

It might not be able to report results back today though.

No worries. I'm working on some other things and can let that PR remain open for a bit if needed. I'd prefer to wait to merge it until I have confirmation from you that it will resolve any issues.

Thanks again!

[lfolger]

It tested these changes and couldn't find any issue with it.