Do not allow packed option on anything but repeated primitive fields #189
Conversation
| if err := r.validateExtensions(r, handler); err != nil { | ||
| return err | ||
| } | ||
| return r.validateJSONNamesInFile(handler) |
There was a problem hiding this comment.
When trying to figure out the best way to incorporate the packed option check into these existing traversals of the hierarchy, I figured it would be better to get rid of the manual recursive traversal and do this more like a visitor pattern. The existing walk package in this repo was intended exactly for this sort of thing.
|
cc @mfridman |
linker/validate.go
Outdated
| pos := file.NodeInfo(r.FieldNode(fd.proto).FieldLabel()).Start() | ||
| err := handler.HandleErrorf(pos, "packed option is only allowed on repeated fields") | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
Should this return the error?
Doh, nvm.
There was a problem hiding this comment.
It should, that's a good catch.
The reason it does not impact tests is because the handler (in above call to HandleErrorf) can choose to return nil, which tells the compiler to keep going. At the end, even if the handler never returned an error in this flow, the compiler will see that an error was reported and return that. This is to support tools like buf, where they want the compiler to process as much as possible and report as many errors as possible instead of failing fast.
So the downside to this returning nil was that it was failing to short-circuit -- even though the handler returned an error, indicating the process should terminate, it would continue to do more wasted work.
I've fixed this line and also found another similar issue: in the caller of this function, when we get to the end of processing the file, even if no error was returned, we should still consider the file an erroneous one if any errors were reported. (The impact of that is rather subtle: but it's the difference between the file being included in "partial" results on failure or not -- since it is erroneous, it is not supported to be returned in partial results, but it would have been.)
There was a problem hiding this comment.
So the downside to this returning nil was that it was failing to short-circuit
Ye, hence the comment. I just wasn't sure what the correct intended behaviour was in the end, whether we gather as much as possible or bail early.
This is a check that
protocdoes. It is also enforced by the Go protobuf runtime: if you try to create create aprotoreflect.FileDescriptorwhere the underlying proto uses the packed field option on a field that is not packable, it will return an error.