From 34c843d6718cbf3fbbb7294e71ae520e1f51b0c6 Mon Sep 17 00:00:00 2001 From: Anton Grbin Date: Thu, 16 May 2024 13:48:45 -0700 Subject: [PATCH] [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](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](https://github.com/protocolbuffers/protobuf/issues/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 absl::Status ParseMap(JsonLexer& lex, Field field, Msg& msg) { (..) return lex.VisitObject( [&](LocationWith& key) -> absl::Status { (..) return Traits::NewMsg( field, msg, [&](const Desc& type, Msg& 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(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=https://github.com/protocolbuffers/protobuf/pull/16567 from noom:anton--7392--fix-map-key-nit d992b8a2a6c999cdf1559ede9fbf723e15644b30 PiperOrigin-RevId: 634516984 --- src/google/protobuf/json/internal/parser.cc | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/google/protobuf/json/internal/parser.cc b/src/google/protobuf/json/internal/parser.cc index fbf492afa715..ae4954d75e75 100644 --- a/src/google/protobuf/json/internal/parser.cc +++ b/src/google/protobuf/json/internal/parser.cc @@ -699,13 +699,6 @@ absl::Status ParseMap(JsonLexer& lex, Field field, Msg& msg) { } break; } - case FieldDescriptor::TYPE_ENUM: { - MaybeOwnedString key_str = key.value; - auto e = ParseEnumFromStr(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()));