protoc: validate custom json_name configuration (take 2)

[jhump]

This is a re-do of #10581. Merging this PR previously caused issues when importing the change into Google's internal repo (IIUC). So additional testing is needed before attempting to merge again. cc @mkruskal-google

---

This makes a few other related changes, for consistency:

• Adds the existing checks, for default JSON names, to run even for proto2 files, but only as warnings (symmetry with similar check for enum value names).
• Tweaks the similar check for enum value names to include "This is not allowed in proto3", for clarity, since it's only a warning in proto2.

When checking custom JSON names, this considers it an error even in proto2 if two json_name options conflict. However, if a json_name option conflicts with a default JSON name, it is just a warning in proto2 (aligns with the existing check and the similar check for enum value names).

Fixes #5063.

[mkruskal-google]

Ok so on the plus side, this does break our tests. Let's try to get those green and then I can test internally

[jhump]

@mkruskal-google, I guess you are referring to the red "Bazel" and various "Windows C++ ..." kokoro builds?

How do I troubleshoot these? The links for the failed checks do not provide me any details whatsoever as to what exactly failed. I get "Unable to retrieve invocation log". If I look at assets and try to download the build log, I get a 403 Forbidden error.

[mkruskal-google]

Ah sorry, I guess the results are internal only >.<. I'll look into that, but in the meantime I'll summarize the issues and send them your way

[mkruskal-google]

These tests fail on linux under --config=asan, and on windows for normal builds:
//src/google/protobuf:descriptor_unittest
//src/google/protobuf/compiler:parser_unittest

[jhump]

@mkruskal-google, I don't suppose you could provide any advice on how I'd run linux tests with --config=asan? Is that a command-line argument to bazel?

Also, I don't suppose you can dig up any more details (like error message details for which assertion failed)?

If you can, thanks in advance.

[fowles]

bazel build --config asan ... will produce an asan enabled binary.

[mkruskal-google]

Here are your errors in windows:

[ RUN      ] ValidationErrorTest.ValidateProto3JsonName

src/google/protobuf/descriptor_unittest.cc(3935): error: Expected equality of these values:

  expected_errors

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"Name\" (\"Name\") conflicts with the default JSON name of field \"name\" (\"name\"). This is not allowed in proto3.\n"

  error_collector.text_

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"Name\" (\"Name\") conflicts with the default JSON name of field \"name\"\0(\"name\"). This is not allowed in proto3.\n"

src/google/protobuf/descriptor_unittest.cc(3935): error: Expected equality of these values:

  expected_errors

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" (\"AB\") conflicts with the default JSON name of field \"ab\" (\"ab\"). This is not allowed in proto3.\n"

  error_collector.text_

    Which is: "foo.proto: Foo: NAME: The default JSON name of field \"_a__b_\" (\"AB\") conflicts with the default JSON name of field \"ab\"\0(\"ab\"). This is not allowed in proto3.\n"

[  FAILED  ] ValidationErrorTest.ValidateProto3JsonName (1 ms)

[ RUN      ] ParserValidationErrorTest.Proto3JsonConflictError

src/google/protobuf/compiler/parser_unittest.cc(187): error: Expected equality of these values:

  expected_errors

    Which is: "4:15: The default JSON name of field \"Foo\" (\"Foo\") conflicts with the default JSON name of field \"foo\" (\"foo\"). This is not allowed in proto3.\n"

  error_collector_.text_

    Which is: "4:15: The default JSON name of field \"Foo\" (\"Foo\") conflicts with the default JSON name of field \"foo\"\0(\"foo\"). This is not allowed in proto3.\n"

[  FAILED  ] ParserValidationErrorTest.Proto3JsonConflictError (0 ms)

As for the ASAN failures, Matt seems to have tracked it down :)

[jhump]

I am hoping the same issue tickling ASAN errors may also be causing the Windows issues. I really have no idea how a NUL character is sneaking into that string where a space belongs, but it is exactly the same part of code as the ASAN issue, with construction of the name_suffix variable. Maybe changing the type will fix it there, too. 🤞

[mkruskal-google]

Yea I was thinking the same thing. It could just be UB acting differently on windows and linux

[jhump]

@mkruskal-google, looks like everything is green. 🎉

Anything else I can do to help this get merged safely?

[mkruskal-google]

Nope, I will try to import this into google and see if there's any uncaught issues. Side note: what's the timeline on this look like? If we can wait a few weeks this would be much easier to port than it is today

[jhump]

If we can wait a few weeks this would be much easier to port than it is today

@mkruskal-google, in my ideal world, this would get into the next release. So that question goes back to you: if we wait a few weeks, would it still be able to make it into the next release?

I'm trying to reconcile Buf's compiler with protoc. In most cases when there are differences, I update the compiler in Buf to match protoc. But in cases that seem like bugs in protoc (especially when there are issues filed against protoc where the team has suggested they're open to a fix, like this one), I prefer to update/fix in protoc.

In this case, we implemented some checks in Buf's compiler since it was indicated in #5063 that this functionality is acceptable for protoc, too. So now I want to get this into the next release so that the two compilers will behave the same.

[fowles]

Yes, this is not at risk of missing the 22.x release

[mkruskal-google]

Just merged into main! Currently it's setup to only warn for proto2, I'm going to work on enabling errors there next