jsonpb: handle map keys' and values' properties properly

[kassiansun]

Export mtype, mkeyprop, mvalprop fields from proto.Properties for usage outside proto package.

Fix up jsonpb pending TODOs.

Fixes #644

[jhump]

FWIW, this does indeed fix things for me.

[kassiansun]

@dsnet Could you please take a look at this?

[dsnet]

It's really unfortunate that we have to expand the API surface of proto.Properties. That API exists because we lack proper proto reflection and should be deprecated in the near future.

[dsnet]

This is not used in jsonpb. Keep it unexported.

[dsnet]

Call these MapKeyProp and MapValueProp?

[kassiansun]

MapKeyProp & MapValProp seems a bit prettier

[jhump]

It's really unfortunate that we have to expand the API surface of proto.Properties.

This doesn't seem like it should be necessary. If these were added just so that jsonpb knows when a map value is an enum, there are other things that could be examined.

• For marshalling, we know the value is an enum (and can be rendered via string instead of as int) if it is a named type with an underlying type of int32 that implements interface { EnumDescriptor() ([]byte, []int) }.
• For unmarshalling, we could inspect the JSON data (vs. the reflective type info). For example, if the value is a string (e.g. has quotes) but the value inside the quotes is not parseable as a number, don't bother stripping the quotes (e.g. leave it a string literal instead of trying to transform to a number literal).

The latter might be even better addressed by tweaks to the patch in PR #630: don't treat enums the same as other int32 types, in the logic that strips the quotes from numbers that are in string literals. Instead, let enum-specific parsing code (the code that can handle either numbers or names) try to handle the possibility that it is a numeric value in a string literal.

[kassiansun]

@jhump Yes, but I want to make this more reasonable. For now, we only have proto.Properties for a pseudo reflection mechanism, so we should rely on it, and avoid logic like if A is int but not enum. That was my first idea, but soon I find that the map marshalling & unmarshalling has an obvious defect: it had nothing to do with handling embedded enums.

@dsnet For now, as we only have proto.Properties to rely on, we should expand its ability to handle more type. stype is not exported, and struct type is handled from proto.StructProperties instead of proto.Properties. Likewise, we only need to export mkeyprop & mvalprop, like Enum, for handling map type. Unlike struct, which needs an array of proto.Properties, map and enum only needs a single (for map it's actually two, for key and val), exporting mkeyprop and mvalprop in proto.Properties is reasonable, or at least as exporting Enum.

Code updated per review comments.

[jhump]

Ping @dsnet: my own repo is broken (dynamic message JSON tests, which rely on proper behavior in jsonpb) until #644 is fixed: https://travis-ci.org/jhump/protoreflect/builds
It's now been broken for almost two weeks.

[marcusljx]

Will this be in a release any time soon?