protoc: support inf, -inf, nan, and -nan in option values

[jhump]

The parser already supports these values for the special "default" field option. But, inconsistently, does not support them for other options whose type is float or double. This addresses that inconsistency.

Fixes #15010.

[acozzette]

Thank you for the fix and sorry for taking so long to get to this. This looks great to me but would you mind also adding some examples to unittest_custom_options.proto and another short unit test case in parser_unittest.cc that checks that we can read the correct values for those options? I think it would be good to just have test coverage that validates the options after they're interpreted.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

[jhump]

I think it would be good to just have test coverage that validates the options after they're interpreted.

@acozzette, sorry for the long hiatus from the branch!

This was a good idea since I totally messed up my original change and forgot to actually update the options interpreting step to allow identifiers "inf" and "nan" for double and float values 🤦.

I've done as requested... but the new test case in parser_unittest.cc is really complicated. We can't easily extract values from a parsed/linked descriptor since interpreting options stores them in the unrecognized field set. So we have to serialize and de-serialize them. I'm using a dynamic message to avoid issues where the FileOptions descriptor from the compiler output != the descriptor returned by the generated type. (Though I think I could also get around that by instead creating a new pool that uses the generated pool as its fallback database, so it wouldn't try to rebuild a descriptor from the FileDescriptorProto for google/protobuf/descriptor.proto.)

Anyhow. I wonder if this new test is really necessary? There is an existing test (ParseDescriptorDebugTestTestCustomOptions) that indirectly exercises much of the same, by effectively compiling unittest_custom_options.proto and making sure the output matches the embedded descriptor of the generated code for that file. Is that sufficient? If not, is there a way to at least make the new test a little more readable? I'm not as familiar with all of the helpers in this test file, but it doesn't look like much else is actually going through the full pool.BuildFile step to link the descriptors and interpret options 🤷.

[acozzette]

@jhump How about a test that just looks at the generated descriptors from the protos you added in unittest_custom_options.proto and verifies that the options have the expected values? That should be a lot simpler because then you can just rely on the generated descriptor pool instead of building your own.

[jhump]

How about a test that just looks at the generated descriptors from the protos you added in unittest_custom_options.proto and verifies that the options have the expected values?

@acozzette, okay, I've replaced the complicated test with that. Definitely simpler.

[jhump]

@acozzette, where does this PR go from here? Does it need to be tested internally before it can actually be merged?

[acozzette]

@jhump Sorry for the delay, I was out for a few days last week and lost track of this. Would you mind merging main into your PR (or rebasing)? I'm hitting an issue syncing it into our internal system which I think should be fixed by that. After that it should be straightforward to test it internally and land the change.

[jhump]

Would you mind merging main into your PR (or rebasing)?

@acozzette, done.

[acozzette]

@jhump The only test failure I ran into is this one, which makes sense because it expects to see the current protoc behavior of rejecting values like -inf. I assume the right fix is just to delete the line with expectedDiffWithProtoc: true?

[jhump]

@acozzette, yes, that would be the right fix (and what I will do in that repo as soon as this fix is released and that repo updated to use that latest protoc release).