Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7392] [cpp] Remove dead code path for map key of type enum in JSON parsing (#16567) #16876

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link

[7392] [cpp] Remove dead code path for map key of type enum in JSON parsing (#16567)

Changes

Remove dead code path -- we don't allow enums to be map keys (proto2 spec, proto3 spec). In other words the case block case FieldDescriptor::TYPE_ENUM is dead code. Potential enum type keys will be caught in default: return lex.Invalid("unsupported map key type"); block below similar to other unsupported map key types like double.

Motivation

While working on fixing IgnoreUnknownEnumStringValueInMap conformance tests for cpp (related issue) I stumbled upon a bug where we pass the wrong field parameter to the enum parsing function.

In this scope:

  • the variable field is a map field of the message that holds the map. This field is not of enum type, it's a repeated message of map entires.
  • the variable key_field is the key field of the map message entry. This field is the enum type that we need to parse here.

The function is long, so I clarified it here:

template <typename Traits>
absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) {
  (..)
  return lex.VisitObject(
      [&](LocationWith<MaybeOwnedString>& key) -> absl::Status {
          (..)
          return Traits::NewMsg(
            field, msg,
            [&](const Desc<Traits>& type, Msg<Traits>& entry) -> absl::Status {
              auto key_field = Traits::KeyField(type);
              switch (Traits::FieldType(key_field)) {
                (..)
                case FieldDescriptor::TYPE_ENUM: {
                  MaybeOwnedString key_str = key.value;
                  auto e = ParseEnumFromStr<Traits>(lex, key_str, /** bug here **/ field);

The correct reference should be key_field.

Instead of fixing the bug and leaving the dead code, it's better to remove the dead block alltogether.

Closes #16567

COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a
FUTURE_COPYBARA_INTEGRATE_REVIEW=#16567 from noom:anton--7392--fix-map-key-nit d992b8a

@copybara-service copybara-service bot closed this May 16, 2024
@copybara-service copybara-service bot deleted the test_634509516 branch May 16, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0 participants