Skip to content

Commit

Permalink
[7392] [cpp] Remove dead code path for map key of type enum in JSON p…
Browse files Browse the repository at this point in the history
…arsing (#16567)

# Changes

Remove dead code path -- we don't allow enums to be map keys ([proto2 spec](https://protobuf.dev/programming-guides/proto2/#maps), [proto3 spec](https://protobuf.dev/programming-guides/proto3/#maps)). 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](#7392)) 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:

```cpp
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
PiperOrigin-RevId: 634516984
  • Loading branch information
antongrbin authored and copybara-github committed May 16, 2024
1 parent 24b91a7 commit 34c843d
Showing 1 changed file with 0 additions and 7 deletions.
7 changes: 0 additions & 7 deletions src/google/protobuf/json/internal/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -699,13 +699,6 @@ absl::Status ParseMap(JsonLexer& lex, Field<Traits> field, Msg<Traits>& msg) {
}
break;
}
case FieldDescriptor::TYPE_ENUM: {
MaybeOwnedString key_str = key.value;
auto e = ParseEnumFromStr<Traits>(lex, key_str, field);
RETURN_IF_ERROR(e.status());
Traits::SetEnum(key_field, entry, e->value_or(0));
break;
}
case FieldDescriptor::TYPE_STRING: {
Traits::SetString(key_field, entry,
std::move(key.value.ToString()));
Expand Down

0 comments on commit 34c843d

Please sign in to comment.