Fix Go feature set handling in editions for checks #3590
Conversation
This fixes the handling of the Go feature set in editions for use with checks. Dynamic types would cause the google.golang.org/protobuf library to panic when trying to fetch the Go feature extension. This is a temporary workaround until upstream can be resolved. See the issue: golang/protobuf#1669 When resolving file descriptors for checks, we now check for use of google/protobuf/go_features.proto and reparse extensions with a resolver targetted at the gofeaturespb package. This ensure the types will match the ones set in the extension type gofeaturespb.E_go. Fixes #3580
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| })...), | ||
| ) | ||
| for _, descriptor := range reparseDescriptors { | ||
| // We clone the FileDescriptorProto to avoid modifying the original. |
There was a problem hiding this comment.
Does this matter? This is an unexported method, not an exported helper. So do the call sites really re-use the original? If not, seems like a waste to generate an unnecessary copy.
There was a problem hiding this comment.
It will modify the image so anything using that would see it. I don't think it matters though this was just done to be defensive. For the lint/breaking checks the images aren't used elsewhere.
private/bufpkg/bufcheck/util.go
Outdated
| fileDescriptorProto := &descriptorpb.FileDescriptorProto{} | ||
| proto.Merge(fileDescriptorProto, descriptor.FileDescriptorProto) |
There was a problem hiding this comment.
If you were to keep the defensive copy, why not just clone it?
| fileDescriptorProto := &descriptorpb.FileDescriptorProto{} | |
| proto.Merge(fileDescriptorProto, descriptor.FileDescriptorProto) | |
| fileDescriptorProto := proto.Clone(descriptor.FileDescriptorProto).(*descriptorpb.FileDescriptorProto) |
There was a problem hiding this comment.
The linter complains about type assertion so used merge to defer the proto package to handle it.
There was a problem hiding this comment.
I guess //nolint isn't accepted in this repo? 😞
There was a problem hiding this comment.
We use proto.Clone elsewhere with the two var destructor and error check. Now do the same here.
| return &goFeaturesResolver{ | ||
| Files: protoregistryFiles, | ||
| Types: protoregistryTypes, | ||
| }, nil |
There was a problem hiding this comment.
This is fine and safe right now. But this seems a little suspicious to copy these types by value. I think it might be less scary looking to instead define the var at the top:
var resolver goFeaturesResolver
And then use resolver.Files and resolver.Types to register everything and finally:
return &goFeaturesResolver
Not a blocking comment. Just my two cents... 🤷
| assert.Panics(t, func() { | ||
| // TODO: if this no longer panics, we can remove the code handling | ||
| // this workaround in bufcheck.imageToProtoFileDescriptors. | ||
| _, err := protodesc.NewFiles(fileSet) | ||
| assert.NoError(t, err) | ||
| }) |
Co-authored-by: Joshua Humphries <[email protected]>
This fixes the handling of the Go feature set in editions for use with checks. Dynamic types would cause the google.golang.org/protobuf library to panic when trying to fetch the Go feature extension. This is a temporary workaround until upstream can be resolved. See the issue: golang/protobuf#1669
When resolving file descriptors for checks, we now check for use of google/protobuf/go_features.proto and reparse extensions with a resolver targetted at the gofeaturespb package. This ensure the types will match the ones set in the extension type gofeaturespb.E_go.
Fixes #3580