Conversation
This PR fixes a couple more conformance issues found through the broader
test suite:
- A bug in `fdp` where comments attribution should be checking for
`R{Paren,Bracket,Brace}` rather than `L{Paren,Bracket,Brace}`
- `proto3_optional` should be set on a field for explicit presence
- A small bug in `ir_test.go` where the source code info location should
set trailing comments
- We fixed allowing prefixed group fields in the lexer, but we also need
to handle the prefix in the lowering step for prefixed group fields
- Fix a `TODO` to resolve group fields
mcy
left a comment
mcy
left a comment
There was a problem hiding this comment.
Can u make sure theres regression tests for everything
| if label == presence.Explicit && g.currentFile.Syntax() == syntax.Proto3 { | ||
| // For proto3, if the presence is set explicitly with "optional", we need to set | ||
| // "proto3_optional" field to true. | ||
| fdp.Proto3Optional = addr(true) |
There was a problem hiding this comment.
Uhh, so technically you didn't ^^"
protocompile/experimental/fdp/generator.go
Line 306 in a1c17f8
But this doesn't cover extensions that are optional in valid proto3 extension blocks, so this just moves the setting of this to field and the extend/proto3.proto test case has been extended to cover it.
Pushed up a new test case, The group case is covered by the change to the group golden -- I should've caught that earlier, but the presence is now correctly set there. Extended the |
2 participants
This PR fixes a couple more conformance issues found through the broader test suite:
fdpwhere comments attribution should be checking forR{Paren,Bracket,Brace}rather thanL{Paren,Bracket,Brace}fdpwhere the wrong tag was used for message extensionsproto3_optionalshould be set on a field for explicit presence, covering the case of optional extensions inproto3ir_test.gowhere the source code info location should set trailing commentsTODOto resolve group fields