bufbuild / protocompile

A parsing/linking engine for protobuf; the guts for a pure Go replacement of protoc.
Apache License 2.0
227 stars 18 forks source link

Add more thorough checks for when to adapt a value when resolving custom feature #306

Closed jhump closed 4 months ago

jhump commented 4 months ago

As is, this code is unable to handle the following case:

  1. The descriptor includes dynamic extensions for all custom options and custom features.
  2. The user supplies a generated extension type, to query for the custom feature.

The code here ostensibly handles this. But the cases where it decides it needs to adapt the value (by marshalling and then unmarshalling into different type) isn't quite thorough enough. The issue is when the custom feature is a message. In that case, it works fine to query for the extension itself. But if we then need to extract a field from the message, the field descriptor is likely referring to a generated message type, whose descriptor is an instance in protoregistry.GlobalTypes. But the value is likely to be a dynamic message, whose descriptor may have been constructed dynamically (such as being parsed from source). In that case, the protoreflect stuff will happily panic when it sees the mismatch 😱.

Up to now, I had worked around this in a rather gross way: in the "buf breaking" implementation where I need to resolve custom features, I had a bunch of code that looks very similar to this stuff, to re-parse the features (here).

But if I simply beef up this function to do a slightly more thorough check for this case, then I can remove a bunch of that redundant (and less efficient) code from the "buf breaking" implementation.

jhump commented 4 months ago

Welp, let's hold off on review for now... This is not quite working as desired (broken tests in https://github.com/bufbuild/buf/pull/2967).

I'll mark as ready for review after I get time to do more digging/troubleshooting/fixing.

jhump commented 4 months ago

I was able to step through the code to see where it was breaking. I added a test to cover that logic (which failed) and then added a fix. See the later two commits.

I also tested with bufbuild/buf to make sure that all of the tests pass there, too.

jhump commented 4 months ago

@emcfarlane, ping.