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

[antongrbin]

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.

[antongrbin]

@sbenzaquen friendly ping -- please advise if we should continue with the bugfix or remove the dead code block.

[antongrbin]

@jskeet, I am reaching out to you since you worked with me on https://github.com/protocolbuffers/protobuf/issues/7392 and this PR is first in line to fix the remaining corner case for C++.

Can you help me reach @sbenzaquen -- I suspect they are not seeing github notifications here.

Btw, nothing urgent here, I am not blocked on this -- just minimizng the amount of things I have open.

[jskeet]

Have sent a ping by internal email.

[sbenzaquen]

Sorry for the delay. Given that this code path is dead and it can't be tested, I think a better approach is to remove the ENUM case as you mention. If we ever start supporting this, I rather we write the code at that time when unittests are added instead of silently turning on code that has never run. We already do not handle other invalid key types (like double) so this type should go into that defualt: case too.

[antongrbin]

@sbenzaquen -- thank you! Updated the PR accordingly.

[antongrbin]

@sbenzaquen thank you for the review! There was (I believe) an unrelated test failure that blocked merge: https://github.com/protocolbuffers/protobuf/actions/runs/9036284406/job/24832883483#step:3:909

I merged with latest main to see if it's still there. I need another "safe for test" label.