Merge pull request #7845 from haberman/sync-stage

Integrate from Piper for C++, Java, and Python

java/core/src/test/java/com/google/protobuf/ExtensionRegistryFactoryTest.java

@@ -30,7 +30,6 @@
 
 package com.google.protobuf;
 
-import org.junit.Ignore;
 import protobuf_unittest.NonNestedExtension;
 import protobuf_unittest.NonNestedExtensionLite;
 import java.lang.reflect.Method;
@@ -42,6 +41,7 @@ import java.util.Set;
 import junit.framework.Test;
 import junit.framework.TestCase;
 import junit.framework.TestSuite;
+import org.junit.Ignore;
 
 /**
  * Tests for {@link ExtensionRegistryFactory} and the {@link ExtensionRegistry} instances it
@@ -58,6 +58,7 @@ import junit.framework.TestSuite;
  * behavior in Java 11. That seems to have broken the way the test uses a custom ClassLoader to
  * exercise Lite functionality.
  */
+@SuppressWarnings("JUnit4ClassUsedInJUnit3")
 @Ignore
 public class ExtensionRegistryFactoryTest extends TestCase {
 

python/google/protobuf/descriptor.py

@@ -227,7 +227,8 @@ class _NestedDescriptorBase(DescriptorBase):
       proto: An empty proto instance from descriptor_pb2.
 
     Raises:
-      Error: If self couldn't be serialized, due to to few constructor arguments.
+      Error: If self couldn't be serialized, due to to few constructor
+        arguments.
     """
     if (self.file is not None and
         self._serialized_start is not None and

python/google/protobuf/internal/message_test.py

@@ -1059,7 +1059,7 @@ class MessageTest(unittest.TestCase):
     self.assertIsInstance(m.optional_string, six.text_type)
 
   def testLongValuedSlice(self, message_module):
-    """It should be possible to use long-valued indices in slices
+    """It should be possible to use long-valued indices in slices.
 
     This didn't used to work in the v2 C++ implementation.
     """

src/google/protobuf/compiler/cpp/cpp_helpers.cc

@@ -1415,9 +1415,7 @@ class ParseLoopGenerator {
       format_.Set("has_bits", "_has_bits_");
     }
 
-    if (descriptor->file()->options().cc_enable_arenas()) {
-      format_("$p_ns$::Arena* arena = GetArena(); (void)arena;\n");
-    }
+    format_("$p_ns$::Arena* arena = GetArena(); (void)arena;\n");
     GenerateParseLoop(descriptor, ordered_fields);
     format_.Outdent();
     format_("success:\n");
@@ -1469,8 +1467,7 @@ class ParseLoopGenerator {
       // Open source doesn't support other ctypes;
       ctype = field->options().ctype();
     }
-    if (field->file()->options().cc_enable_arenas() && !field->is_repeated() &&
-        !options_.opensource_runtime &&
+    if (!field->is_repeated() && !options_.opensource_runtime &&
         GetOptimizeFor(field->file(), options_) != FileOptions::LITE_RUNTIME &&
         // For now only use arena string for strings with empty defaults.
         field->default_value_string().empty() &&

src/google/protobuf/compiler/importer_unittest.cc

@@ -469,7 +469,8 @@ TEST_F(DiskSourceTreeTest, DiskFileToVirtualFileCanonicalization) {
                                                &shadowing_disk_file));
 
 #ifdef WIN32
-  // "C:\foo" is not mapped (it should not be misinterpreted as being under ".").
+  // "C:\foo" is not mapped (it should not be misinterpreted as being under
+  // ".").
   EXPECT_EQ(DiskSourceTree::NO_MAPPING,
             source_tree_.DiskFileToVirtualFile("C:\\foo", &virtual_file,
                                                &shadowing_disk_file));

src/google/protobuf/generated_message_reflection.cc

@@ -1884,6 +1884,15 @@ bool Reflection::InsertOrLookupMapValue(Message* message,
       ->InsertOrLookupMapValue(key, val);
 }
 
+bool Reflection::LookupMapValue(const Message& message,
+                                const FieldDescriptor* field, const MapKey& key,
+                                MapValueConstRef* val) const {
+  USAGE_CHECK(IsMapFieldInApi(field), "LookupMapValue",
+              "Field is not a map field.");
+  val->SetType(field->message_type()->FindFieldByName("value")->cpp_type());
+  return GetRaw<MapFieldBase>(message, field).LookupMapValue(key, val);
+}
+
 bool Reflection::DeleteMapValue(Message* message, const FieldDescriptor* field,
                                 const MapKey& key) const {
   USAGE_CHECK(IsMapFieldInApi(field), "DeleteMapValue",

src/google/protobuf/map_field.cc

@@ -44,20 +44,24 @@ MapFieldBase::~MapFieldBase() {
 }
 
 const RepeatedPtrFieldBase& MapFieldBase::GetRepeatedField() const {
+  ConstAccess();
   SyncRepeatedFieldWithMap();
   return *reinterpret_cast<RepeatedPtrFieldBase*>(repeated_field_);
 }
 
 RepeatedPtrFieldBase* MapFieldBase::MutableRepeatedField() {
+  MutableAccess();
   SyncRepeatedFieldWithMap();
   SetRepeatedDirty();
   return reinterpret_cast<RepeatedPtrFieldBase*>(repeated_field_);
 }
 
 size_t MapFieldBase::SpaceUsedExcludingSelfLong() const {
+  ConstAccess();
   mutex_.Lock();
   size_t size = SpaceUsedExcludingSelfNoLock();
   mutex_.Unlock();
+  ConstAccess();
   return size;
 }
 
@@ -70,6 +74,7 @@ size_t MapFieldBase::SpaceUsedExcludingSelfNoLock() const {
 }
 
 bool MapFieldBase::IsMapValid() const {
+  ConstAccess();
   // "Acquire" insures the operation after SyncRepeatedFieldWithMap won't get
   // executed before state_ is checked.
   int state = state_.load(std::memory_order_acquire);
@@ -77,23 +82,27 @@ bool MapFieldBase::IsMapValid() const {
 }
 
 bool MapFieldBase::IsRepeatedFieldValid() const {
+  ConstAccess();
   int state = state_.load(std::memory_order_acquire);
   return state != STATE_MODIFIED_MAP;
 }
 
 void MapFieldBase::SetMapDirty() {
+  MutableAccess();
   // These are called by (non-const) mutator functions. So by our API it's the
   // callers responsibility to have these calls properly ordered.
   state_.store(STATE_MODIFIED_MAP, std::memory_order_relaxed);
 }
 
 void MapFieldBase::SetRepeatedDirty() {
+  MutableAccess();
   // These are called by (non-const) mutator functions. So by our API it's the
   // callers responsibility to have these calls properly ordered.
   state_.store(STATE_MODIFIED_REPEATED, std::memory_order_relaxed);
 }
 
 void MapFieldBase::SyncRepeatedFieldWithMap() const {
+  ConstAccess();
   // acquire here matches with release below to ensure that we can only see a
   // value of CLEAN after all previous changes have been synced.
   switch (state_.load(std::memory_order_acquire)) {
@@ -106,6 +115,7 @@ void MapFieldBase::SyncRepeatedFieldWithMap() const {
         state_.store(CLEAN, std::memory_order_release);
       }
       mutex_.Unlock();
+      ConstAccess();
       break;
     case CLEAN:
       mutex_.Lock();
@@ -122,6 +132,7 @@ void MapFieldBase::SyncRepeatedFieldWithMap() const {
         state_.store(CLEAN, std::memory_order_release);
       }
       mutex_.Unlock();
+      ConstAccess();
       break;
     default:
       break;
@@ -135,6 +146,7 @@ void MapFieldBase::SyncRepeatedFieldWithMapNoLock() const {
 }
 
 void MapFieldBase::SyncMapWithRepeatedField() const {
+  ConstAccess();
   // acquire here matches with release below to ensure that we can only see a
   // value of CLEAN after all previous changes have been synced.
   if (state_.load(std::memory_order_acquire) == STATE_MODIFIED_REPEATED) {
@@ -146,6 +158,7 @@ void MapFieldBase::SyncMapWithRepeatedField() const {
       state_.store(CLEAN, std::memory_order_release);
     }
     mutex_.Unlock();
+    ConstAccess();
   }
 }
 
@@ -245,6 +258,19 @@ bool DynamicMapField::InsertOrLookupMapValue(const MapKey& map_key,
   return false;
 }
 
+bool DynamicMapField::LookupMapValue(const MapKey& map_key,
+                                     MapValueConstRef* val) const {
+  const Map<MapKey, MapValueRef>& map = GetMap();
+  Map<MapKey, MapValueRef>::const_iterator iter = map.find(map_key);
+  if (iter == map.end()) {
+    return false;
+  }
+  // map_key is already in the map. Make sure (*map)[map_key] is not called.
+  // [] may reorder the map and iterators.
+  val->CopyFrom(iter->second);
+  return true;
+}
+
 bool DynamicMapField::DeleteMapValue(const MapKey& map_key) {
   MapFieldBase::SyncMapWithRepeatedField();
   Map<MapKey, MapValueRef>::iterator iter = map_.find(map_key);

src/google/protobuf/map_field.h

@@ -346,6 +346,10 @@ class PROTOBUF_EXPORT MapFieldBase {
   virtual bool ContainsMapKey(const MapKey& map_key) const = 0;
   virtual bool InsertOrLookupMapValue(const MapKey& map_key,
                                       MapValueRef* val) = 0;
+  virtual bool LookupMapValue(const MapKey& map_key,
+                              MapValueConstRef* val) const = 0;
+  bool LookupMapValue(const MapKey&, MapValueRef*) const = delete;
+
   // Returns whether changes to the map are reflected in the repeated field.
   bool IsRepeatedFieldValid() const;
   // Insures operations after won't get executed before calling this.
@@ -386,12 +390,31 @@ class PROTOBUF_EXPORT MapFieldBase {
   // Tells MapFieldBase that there is new change to Map.
   void SetMapDirty();
 
-  // Tells MapFieldBase that there is new change to RepeatedPTrField.
+  // Tells MapFieldBase that there is new change to RepeatedPtrField.
   void SetRepeatedDirty();
 
   // Provides derived class the access to repeated field.
   void* MutableRepeatedPtrField() const;
 
+  // Support thread sanitizer (tsan) by making const / mutable races
+  // more apparent.  If one thread calls MutableAccess() while another
+  // thread calls either ConstAccess() or MutableAccess(), on the same
+  // MapFieldBase-derived object, and there is no synchronization going
+  // on between them, tsan will alert.
+#if defined(__SANITIZE_THREAD__) || defined(THREAD_SANITIZER)
+  void ConstAccess() const { GOOGLE_CHECK_EQ(seq1_, seq2_); }
+  void MutableAccess() {
+    if (seq1_ & 1) {
+      seq2_ = ++seq1_;
+    } else {
+      seq1_ = ++seq2_;
+    }
+  }
+  unsigned int seq1_ = 0, seq2_ = 0;
+#else
+  void ConstAccess() const {}
+  void MutableAccess() {}
+#endif
   enum State {
     STATE_MODIFIED_MAP = 0,       // map has newly added data that has not been
                                   // synchronized to repeated field
@@ -504,6 +527,9 @@ class MapField : public TypeDefinedMapFieldBase<Key, T> {
   // Implement MapFieldBase
   bool ContainsMapKey(const MapKey& map_key) const override;
   bool InsertOrLookupMapValue(const MapKey& map_key, MapValueRef* val) override;
+  bool LookupMapValue(const MapKey& map_key,
+                      MapValueConstRef* val) const override;
+  bool LookupMapValue(const MapKey&, MapValueRef*) const = delete;
   bool DeleteMapValue(const MapKey& map_key) override;
 
   const Map<Key, T>& GetMap() const override {
@@ -597,6 +623,9 @@ class PROTOBUF_EXPORT DynamicMapField
   // Implement MapFieldBase
   bool ContainsMapKey(const MapKey& map_key) const override;
   bool InsertOrLookupMapValue(const MapKey& map_key, MapValueRef* val) override;
+  bool LookupMapValue(const MapKey& map_key,
+                      MapValueConstRef* val) const override;
+  bool LookupMapValue(const MapKey&, MapValueRef*) const = delete;
   bool DeleteMapValue(const MapKey& map_key) override;
   void MergeFrom(const MapFieldBase& other) override;
   void Swap(MapFieldBase* other) override;
@@ -623,96 +652,76 @@ class PROTOBUF_EXPORT DynamicMapField
 
 }  // namespace internal
 
-// MapValueRef points to a map value.
-class PROTOBUF_EXPORT MapValueRef {
+// MapValueConstRef points to a map value. Users can NOT modify
+// the map value.
+class PROTOBUF_EXPORT MapValueConstRef {
  public:
-  MapValueRef() : data_(NULL), type_(0) {}
-
-  void SetInt64Value(int64 value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT64, "MapValueRef::SetInt64Value");
-    *reinterpret_cast<int64*>(data_) = value;
-  }
-  void SetUInt64Value(uint64 value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT64, "MapValueRef::SetUInt64Value");
-    *reinterpret_cast<uint64*>(data_) = value;
-  }
-  void SetInt32Value(int32 value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT32, "MapValueRef::SetInt32Value");
-    *reinterpret_cast<int32*>(data_) = value;
-  }
-  void SetUInt32Value(uint32 value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT32, "MapValueRef::SetUInt32Value");
-    *reinterpret_cast<uint32*>(data_) = value;
-  }
-  void SetBoolValue(bool value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_BOOL, "MapValueRef::SetBoolValue");
-    *reinterpret_cast<bool*>(data_) = value;
-  }
-  // TODO(jieluo) - Checks that enum is member.
-  void SetEnumValue(int value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_ENUM, "MapValueRef::SetEnumValue");
-    *reinterpret_cast<int*>(data_) = value;
-  }
-  void SetStringValue(const std::string& value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_STRING, "MapValueRef::SetStringValue");
-    *reinterpret_cast<std::string*>(data_) = value;
-  }
-  void SetFloatValue(float value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_FLOAT, "MapValueRef::SetFloatValue");
-    *reinterpret_cast<float*>(data_) = value;
-  }
-  void SetDoubleValue(double value) {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_DOUBLE, "MapValueRef::SetDoubleValue");
-    *reinterpret_cast<double*>(data_) = value;
-  }
+  MapValueConstRef() : data_(nullptr), type_(0) {}
 
   int64 GetInt64Value() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT64, "MapValueRef::GetInt64Value");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT64,
+               "MapValueConstRef::GetInt64Value");
     return *reinterpret_cast<int64*>(data_);
   }
   uint64 GetUInt64Value() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT64, "MapValueRef::GetUInt64Value");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT64,
+               "MapValueConstRef::GetUInt64Value");
     return *reinterpret_cast<uint64*>(data_);
   }
   int32 GetInt32Value() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT32, "MapValueRef::GetInt32Value");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT32,
+               "MapValueConstRef::GetInt32Value");
     return *reinterpret_cast<int32*>(data_);
   }
   uint32 GetUInt32Value() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT32, "MapValueRef::GetUInt32Value");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT32,
+               "MapValueConstRef::GetUInt32Value");
     return *reinterpret_cast<uint32*>(data_);
   }
   bool GetBoolValue() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_BOOL, "MapValueRef::GetBoolValue");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_BOOL, "MapValueConstRef::GetBoolValue");
     return *reinterpret_cast<bool*>(data_);
   }
   int GetEnumValue() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_ENUM, "MapValueRef::GetEnumValue");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_ENUM, "MapValueConstRef::GetEnumValue");
     return *reinterpret_cast<int*>(data_);
   }
   const std::string& GetStringValue() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_STRING, "MapValueRef::GetStringValue");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_STRING,
+               "MapValueConstRef::GetStringValue");
     return *reinterpret_cast<std::string*>(data_);
   }
   float GetFloatValue() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_FLOAT, "MapValueRef::GetFloatValue");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_FLOAT,
+               "MapValueConstRef::GetFloatValue");
     return *reinterpret_cast<float*>(data_);
   }
   double GetDoubleValue() const {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_DOUBLE, "MapValueRef::GetDoubleValue");
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_DOUBLE,
+               "MapValueConstRef::GetDoubleValue");
     return *reinterpret_cast<double*>(data_);
   }
 
   const Message& GetMessageValue() const {
     TYPE_CHECK(FieldDescriptor::CPPTYPE_MESSAGE,
-               "MapValueRef::GetMessageValue");
+               "MapValueConstRef::GetMessageValue");
     return *reinterpret_cast<Message*>(data_);
   }
 
-  Message* MutableMessageValue() {
-    TYPE_CHECK(FieldDescriptor::CPPTYPE_MESSAGE,
-               "MapValueRef::MutableMessageValue");
-    return reinterpret_cast<Message*>(data_);
+ protected:
+  // data_ point to a map value. MapValueConstRef does not
+  // own this value.
+  void* data_;
+  // type_ is 0 or a valid FieldDescriptor::CppType.
+  int type_;
+
+  FieldDescriptor::CppType type() const {
+    if (type_ == 0 || data_ == nullptr) {
+      GOOGLE_LOG(FATAL)
+          << "Protocol Buffer map usage error:\n"
+          << "MapValueConstRef::type MapValueConstRef is not initialized.";
+    }
+    return static_cast<FieldDescriptor::CppType>(type_);
   }
 
  private:
@@ -727,19 +736,66 @@ class PROTOBUF_EXPORT MapValueRef {
   friend class internal::DynamicMapField;
 
   void SetType(FieldDescriptor::CppType type) { type_ = type; }
-
-  FieldDescriptor::CppType type() const {
-    if (type_ == 0 || data_ == NULL) {
-      GOOGLE_LOG(FATAL) << "Protocol Buffer map usage error:\n"
-                 << "MapValueRef::type MapValueRef is not initialized.";
-    }
-    return (FieldDescriptor::CppType)type_;
-  }
   void SetValue(const void* val) { data_ = const_cast<void*>(val); }
-  void CopyFrom(const MapValueRef& other) {
+  void CopyFrom(const MapValueConstRef& other) {
     type_ = other.type_;
     data_ = other.data_;
   }
+};
+
+// MapValueRef points to a map value. Users are able to modify
+// the map value.
+class PROTOBUF_EXPORT MapValueRef final : public MapValueConstRef {
+ public:
+  MapValueRef() {}
+
+  void SetInt64Value(int64 value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT64, "MapValueRef::SetInt64Value");
+    *reinterpret_cast<int64*>(data_) = value;
+  }
+  void SetUInt64Value(uint64 value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT64, "MapValueRef::SetUInt64Value");
+    *reinterpret_cast<uint64*>(data_) = value;
+  }
+  void SetInt32Value(int32 value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_INT32, "MapValueRef::SetInt32Value");
+    *reinterpret_cast<int32*>(data_) = value;
+  }
+  void SetUInt32Value(uint32 value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_UINT32, "MapValueRef::SetUInt32Value");
+    *reinterpret_cast<uint32*>(data_) = value;
+  }
+  void SetBoolValue(bool value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_BOOL, "MapValueRef::SetBoolValue");
+    *reinterpret_cast<bool*>(data_) = value;
+  }
+  // TODO(jieluo) - Checks that enum is member.
+  void SetEnumValue(int value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_ENUM, "MapValueRef::SetEnumValue");
+    *reinterpret_cast<int*>(data_) = value;
+  }
+  void SetStringValue(const std::string& value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_STRING, "MapValueRef::SetStringValue");
+    *reinterpret_cast<std::string*>(data_) = value;
+  }
+  void SetFloatValue(float value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_FLOAT, "MapValueRef::SetFloatValue");
+    *reinterpret_cast<float*>(data_) = value;
+  }
+  void SetDoubleValue(double value) {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_DOUBLE, "MapValueRef::SetDoubleValue");
+    *reinterpret_cast<double*>(data_) = value;
+  }
+
+  Message* MutableMessageValue() {
+    TYPE_CHECK(FieldDescriptor::CPPTYPE_MESSAGE,
+               "MapValueRef::MutableMessageValue");
+    return reinterpret_cast<Message*>(data_);
+  }
+
+ private:
+  friend class internal::DynamicMapField;
+
   // Only used in DynamicMapField
   void DeleteData() {
     switch (type_) {
@@ -761,11 +817,6 @@ class PROTOBUF_EXPORT MapValueRef {
 #undef HANDLE_TYPE
     }
   }
-  // data_ point to a map value. MapValueRef does not
-  // own this value.
-  void* data_;
-  // type_ is 0 or a valid FieldDescriptor::CppType.
-  int type_;
 };
 
 #undef TYPE_CHECK

src/google/protobuf/map_field_inl.h

@@ -234,6 +234,23 @@ bool MapField<Derived, Key, T, kKeyFieldType,
   return false;
 }
 
+template <typename Derived, typename Key, typename T,
+          WireFormatLite::FieldType kKeyFieldType,
+          WireFormatLite::FieldType kValueFieldType>
+bool MapField<Derived, Key, T, kKeyFieldType, kValueFieldType>::LookupMapValue(
+    const MapKey& map_key, MapValueConstRef* val) const {
+  const Map<Key, T>& map = GetMap();
+  const Key& key = UnwrapMapKey<Key>(map_key);
+  typename Map<Key, T>::const_iterator iter = map.find(key);
+  if (map.end() == iter) {
+    return false;
+  }
+  // Key is already in the map. Make sure (*map)[key] is not called.
+  // [] may reorder the map and iterators.
+  val->SetValue(&(iter->second));
+  return true;
+}
+
 template <typename Derived, typename Key, typename T,
           WireFormatLite::FieldType kKeyFieldType,
           WireFormatLite::FieldType kValueFieldType>

src/google/protobuf/map_field_test.cc

@@ -77,6 +77,10 @@ class MapFieldBaseStub : public MapFieldBase {
                               MapValueRef* val) override {
     return false;
   }
+  bool LookupMapValue(const MapKey& map_key,
+                      MapValueConstRef* val) const override {
+    return false;
+  }
   bool DeleteMapValue(const MapKey& map_key) override { return false; }
   bool EqualIterator(const MapIterator& a,
                      const MapIterator& b) const override {

src/google/protobuf/map_test.cc

@@ -234,9 +234,10 @@ TEST_F(MapImplTest, UsageErrors) {
                "  Actual   : int64");
 
   MapValueRef value;
-  EXPECT_DEATH(value.SetFloatValue(0.1),
-               "Protocol Buffer map usage error:\n"
-               "MapValueRef::type MapValueRef is not initialized.");
+  EXPECT_DEATH(
+      value.SetFloatValue(0.1),
+      "Protocol Buffer map usage error:\n"
+      "MapValue[Const]*Ref::type MapValue[Const]*Ref is not initialized.");
 }
 
 #endif  // PROTOBUF_HAS_DEATH_TEST
@@ -3216,9 +3217,9 @@ TEST(WireFormatForMapFieldTest, MapByteSizeDynamicMessage) {
   // Protobuf used to have a bug for serialize when map it marked CLEAN. It used
   // repeated field to calculate ByteSizeLong but use map to serialize the real
   // data, thus the ByteSizeLong may bigger than real serialized size. A crash
-  // might be happen at SerializeToString(). Or an "unexpected end group" warning
-  // was raised at parse back if user use SerializeWithCachedSizes() which
-  // avoids size check at serialize.
+  // might be happen at SerializeToString(). Or an "unexpected end group"
+  // warning was raised at parse back if user use SerializeWithCachedSizes()
+  // which avoids size check at serialize.
   std::string serialized_data;
   dynamic_message->SerializeToString(&serialized_data);
   EXPECT_EQ(serialized_data, expected_serialized_data);

src/google/protobuf/map_test_util.h

@@ -495,20 +495,27 @@ inline void MapReflectionTester::SetMapFieldsViaMapReflection(
 
   Message* sub_foreign_message = nullptr;
   MapValueRef map_val;
+  MapValueConstRef map_val_const;
 
   // Add first element.
   MapKey map_key;
   map_key.SetInt32Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int32_int32"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_int32_int32"),
                                                  map_key, &map_val));
   map_val.SetInt32Value(0);
 
   map_key.SetInt64Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int64_int64"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_int64_int64"),
                                                  map_key, &map_val));
   map_val.SetInt64Value(0);
 
   map_key.SetUInt32Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_uint32_uint32"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(
       message, F("map_uint32_uint32"), map_key, &map_val));
   map_val.SetUInt32Value(0);
@@ -559,26 +566,36 @@ inline void MapReflectionTester::SetMapFieldsViaMapReflection(
   map_val.SetDoubleValue(0.0);
 
   map_key.SetBoolValue(false);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_bool_bool"), map_key,
+                                          &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_bool_bool"),
                                                  map_key, &map_val));
   map_val.SetBoolValue(false);
 
   map_key.SetStringValue("0");
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_string_string"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(
       message, F("map_string_string"), map_key, &map_val));
   map_val.SetStringValue("0");
 
   map_key.SetInt32Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int32_bytes"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_int32_bytes"),
                                                  map_key, &map_val));
   map_val.SetStringValue("0");
 
   map_key.SetInt32Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int32_enum"),
+                                          map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_int32_enum"),
                                                  map_key, &map_val));
   map_val.SetEnumValue(map_enum_bar_->number());
 
   map_key.SetInt32Value(0);
+  EXPECT_FALSE(reflection->LookupMapValue(
+      *message, F("map_int32_foreign_message"), map_key, &map_val_const));
   EXPECT_TRUE(reflection->InsertOrLookupMapValue(
       message, F("map_int32_foreign_message"), map_key, &map_val));
   sub_foreign_message = map_val.MutableMessageValue();
@@ -933,6 +950,7 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
   const Reflection* reflection = message.GetReflection();
   const Message* sub_message;
   MapKey map_key;
+  MapValueConstRef map_value_const_ref;
 
   // -----------------------------------------------------------------
 
@@ -971,6 +989,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_TRUE(
           reflection->ContainsMapKey(message, F("map_int32_int32"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int32_int32"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt32Value(), val);
     }
   }
   {
@@ -990,6 +1011,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt64Value(key);
       EXPECT_TRUE(
           reflection->ContainsMapKey(message, F("map_int64_int64"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int64_int64"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt64Value(), val);
     }
   }
   {
@@ -1009,6 +1033,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetUInt32Value(key);
       EXPECT_TRUE(
           reflection->ContainsMapKey(message, F("map_uint32_uint32"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_uint32_uint32"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetUInt32Value(), val);
     }
   }
   {
@@ -1027,6 +1054,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetUInt64Value(key);
       EXPECT_TRUE(
           reflection->ContainsMapKey(message, F("map_uint64_uint64"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_uint64_uint64"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetUInt64Value(), val);
     }
   }
   {
@@ -1045,6 +1075,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_sint32_sint32"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_sint32_sint32"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt32Value(), val);
     }
   }
   {
@@ -1063,6 +1096,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt64Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_sint64_sint64"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_sint64_sint64"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt64Value(), val);
     }
   }
   {
@@ -1081,6 +1117,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetUInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_fixed32_fixed32"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_fixed32_fixed32"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetUInt32Value(), val);
     }
   }
   {
@@ -1099,6 +1138,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetUInt64Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_fixed64_fixed64"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_fixed64_fixed64"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetUInt64Value(), val);
     }
   }
   {
@@ -1117,6 +1159,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_sfixed32_sfixed32"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(
+          message, F("map_sfixed32_sfixed32"), map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt32Value(), val);
     }
   }
   {
@@ -1135,6 +1180,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt64Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_sfixed64_sfixed64"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(
+          message, F("map_sfixed64_sfixed64"), map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetInt64Value(), val);
     }
   }
   {
@@ -1153,6 +1201,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(message, F("map_int32_float"),
                                                  map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int32_float"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetFloatValue(), val);
     }
   }
   {
@@ -1171,6 +1222,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(message, F("map_int32_double"),
                                                  map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int32_double"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetDoubleValue(), val);
     }
   }
   {
@@ -1189,6 +1243,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetBoolValue(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(message, F("map_bool_bool"),
                                                  map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_bool_bool"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetBoolValue(), val);
     }
   }
   {
@@ -1207,6 +1264,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetStringValue(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_string_string"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_string_string"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetStringValue(), val);
     }
   }
   {
@@ -1225,6 +1285,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(message, F("map_int32_bytes"),
                                                  map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int32_bytes"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetStringValue(), val);
     }
   }
   {
@@ -1243,6 +1306,9 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(message, F("map_int32_enum"),
                                                  map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message, F("map_int32_enum"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(map_value_const_ref.GetEnumValue(), val->number());
     }
   }
   {
@@ -1263,6 +1329,12 @@ inline void MapReflectionTester::ExpectMapFieldsSetViaReflection(
       map_key.SetInt32Value(key);
       EXPECT_EQ(true, reflection->ContainsMapKey(
                           message, F("map_int32_foreign_message"), map_key));
+      EXPECT_TRUE(reflection->LookupMapValue(message,
+                                             F("map_int32_foreign_message"),
+                                             map_key, &map_value_const_ref));
+      EXPECT_EQ(foreign_message.GetReflection()->GetInt32(
+                    map_value_const_ref.GetMessageValue(), foreign_c_),
+                val);
     }
   }
 }

src/google/protobuf/message.h

@@ -145,6 +145,7 @@ class MessageFactory;
 class AssignDescriptorsHelper;
 class DynamicMessageFactory;
 class MapKey;
+class MapValueConstRef;
 class MapValueRef;
 class MapIterator;
 class MapReflectionTester;
@@ -981,10 +982,19 @@ class PROTOBUF_EXPORT Reflection final {
 
   // If key is in map field: Saves the value pointer to val and returns
   // false. If key in not in map field: Insert the key into map, saves
-  // value pointer to val and returns true.
+  // value pointer to val and returns true. Users are able to modify the
+  // map value by MapValueRef.
   bool InsertOrLookupMapValue(Message* message, const FieldDescriptor* field,
                               const MapKey& key, MapValueRef* val) const;
 
+  // If key is in map field: Saves the value pointer to val and returns true.
+  // Returns false if key is not in map field. Users are NOT able to modify
+  // the value by MapValueConstRef.
+  bool LookupMapValue(const Message& message, const FieldDescriptor* field,
+                      const MapKey& key, MapValueConstRef* val) const;
+  bool LookupMapValue(const Message&, const FieldDescriptor*, const MapKey&,
+                      MapValueRef*) const = delete;
+
   // Delete and returns true if key is in the map field. Returns false
   // otherwise.
   bool DeleteMapValue(Message* message, const FieldDescriptor* field,

src/google/protobuf/util/internal/utility.h

@@ -82,8 +82,8 @@ PROTOBUF_EXPORT std::string GetStringOptionOrDefault(
 
 // Returns a boolean value contained in Any type.
 // TODO(skarvaje): Make these utilities dealing with Any types more generic,
-// add more error checking and move to a more public/shareable location so others
-// can use.
+// add more error checking and move to a more public/shareable location so
+// others can use.
 PROTOBUF_EXPORT bool GetBoolFromAny(const google::protobuf::Any& any);
 
 // Returns int64 value contained in Any type.

src/google/protobuf/util/message_differencer.cc

@@ -933,26 +933,25 @@ bool MessageDifferencer::CompareMapFieldByMapReflection(
   }
   const FieldDescriptor* val_des = map_field->message_type()->map_value();
   switch (val_des->cpp_type()) {
-#define HANDLE_TYPE(CPPTYPE, METHOD, COMPAREMETHOD)                         \
-  case FieldDescriptor::CPPTYPE_##CPPTYPE: {                                \
-    for (MapIterator it = reflection1->MapBegin(                            \
-             const_cast<Message*>(&message1), map_field);                   \
-         it !=                                                              \
-         reflection1->MapEnd(const_cast<Message*>(&message1), map_field);   \
-         ++it) {                                                            \
-      if (!reflection2->ContainsMapKey(message2, map_field, it.GetKey())) { \
-        return false;                                                       \
-      }                                                                     \
-      MapValueRef value2;                                                   \
-      reflection2->InsertOrLookupMapValue(const_cast<Message*>(&message2),  \
-                                          map_field, it.GetKey(), &value2); \
-      if (!default_field_comparator_.Compare##COMPAREMETHOD(                \
-              *val_des, it.GetValueRef().Get##METHOD(),                     \
-              value2.Get##METHOD())) {                                      \
-        return false;                                                       \
-      }                                                                     \
-    }                                                                       \
-    break;                                                                  \
+#define HANDLE_TYPE(CPPTYPE, METHOD, COMPAREMETHOD)                           \
+  case FieldDescriptor::CPPTYPE_##CPPTYPE: {                                  \
+    for (MapIterator it = reflection1->MapBegin(                              \
+             const_cast<Message*>(&message1), map_field);                     \
+         it !=                                                                \
+         reflection1->MapEnd(const_cast<Message*>(&message1), map_field);     \
+         ++it) {                                                              \
+      if (!reflection2->ContainsMapKey(message2, map_field, it.GetKey())) {   \
+        return false;                                                         \
+      }                                                                       \
+      MapValueConstRef value2;                                                \
+      reflection2->LookupMapValue(message2, map_field, it.GetKey(), &value2); \
+      if (!default_field_comparator_.Compare##COMPAREMETHOD(                  \
+              *val_des, it.GetValueRef().Get##METHOD(),                       \
+              value2.Get##METHOD())) {                                        \
+        return false;                                                         \
+      }                                                                       \
+    }                                                                         \
+    break;                                                                    \
   }
     HANDLE_TYPE(INT32, Int32Value, Int32);
     HANDLE_TYPE(INT64, Int64Value, Int64);
@@ -973,9 +972,8 @@ bool MessageDifferencer::CompareMapFieldByMapReflection(
         if (!reflection2->ContainsMapKey(message2, map_field, it.GetKey())) {
           return false;
         }
-        MapValueRef value2;
-        reflection2->InsertOrLookupMapValue(const_cast<Message*>(&message2),
-                                            map_field, it.GetKey(), &value2);
+        MapValueConstRef value2;
+        reflection2->LookupMapValue(message2, map_field, it.GetKey(), &value2);
         if (!Compare(it.GetValueRef().GetMessageValue(),
                      value2.GetMessageValue())) {
           return false;
@@ -1220,7 +1218,8 @@ bool MessageDifferencer::IsTreatedAsSet(const FieldDescriptor* field) {
       repeated_field_comparisons_.end()) {
     return repeated_field_comparisons_[field] == AS_SET;
   }
-  return repeated_field_comparison_ == AS_SET;
+  return GetMapKeyComparator(field) == nullptr &&
+         repeated_field_comparison_ == AS_SET;
 }
 
 bool MessageDifferencer::IsTreatedAsSmartSet(const FieldDescriptor* field) {
@@ -1229,7 +1228,8 @@ bool MessageDifferencer::IsTreatedAsSmartSet(const FieldDescriptor* field) {
       repeated_field_comparisons_.end()) {
     return repeated_field_comparisons_[field] == AS_SMART_SET;
   }
-  return repeated_field_comparison_ == AS_SMART_SET;
+  return GetMapKeyComparator(field) == nullptr &&
+         repeated_field_comparison_ == AS_SMART_SET;
 }
 
 bool MessageDifferencer::IsTreatedAsSmartList(const FieldDescriptor* field) {
@@ -1238,7 +1238,8 @@ bool MessageDifferencer::IsTreatedAsSmartList(const FieldDescriptor* field) {
       repeated_field_comparisons_.end()) {
     return repeated_field_comparisons_[field] == AS_SMART_LIST;
   }
-  return repeated_field_comparison_ == AS_SMART_LIST;
+  return GetMapKeyComparator(field) == nullptr &&
+         repeated_field_comparison_ == AS_SMART_LIST;
 }
 
 bool MessageDifferencer::IsTreatedAsSubset(const FieldDescriptor* field) {

src/google/protobuf/wire_format.cc

@@ -67,7 +67,7 @@ namespace internal {
 static size_t MapKeyDataOnlyByteSize(const FieldDescriptor* field,
                                      const MapKey& value);
 static size_t MapValueRefDataOnlyByteSize(const FieldDescriptor* field,
-                                          const MapValueRef& value);
+                                          const MapValueConstRef& value);
 
 // ===================================================================
 
@@ -1099,7 +1099,7 @@ static uint8* SerializeMapKeyWithCachedSizes(const FieldDescriptor* field,
 }
 
 static uint8* SerializeMapValueRefWithCachedSizes(
-    const FieldDescriptor* field, const MapValueRef& value, uint8* target,
+    const FieldDescriptor* field, const MapValueConstRef& value, uint8* target,
     io::EpsCopyOutputStream* stream) {
   target = stream->EnsureSpace(target);
   switch (field->type()) {
@@ -1184,7 +1184,8 @@ class MapKeySorter {
 
 static uint8* InternalSerializeMapEntry(const FieldDescriptor* field,
                                         const MapKey& key,
-                                        const MapValueRef& value, uint8* target,
+                                        const MapValueConstRef& value,
+                                        uint8* target,
                                         io::EpsCopyOutputStream* stream) {
   const FieldDescriptor* key_field = field->message_type()->field(0);
   const FieldDescriptor* value_field = field->message_type()->field(1);
@@ -1237,9 +1238,8 @@ uint8* WireFormat::InternalSerializeField(const FieldDescriptor* field,
             MapKeySorter::SortKey(message, message_reflection, field);
         for (std::vector<MapKey>::iterator it = sorted_key_list.begin();
              it != sorted_key_list.end(); ++it) {
-          MapValueRef map_value;
-          message_reflection->InsertOrLookupMapValue(
-              const_cast<Message*>(&message), field, *it, &map_value);
+          MapValueConstRef map_value;
+          message_reflection->LookupMapValue(message, field, *it, &map_value);
           target =
               InternalSerializeMapEntry(field, *it, map_value, target, stream);
         }
@@ -1566,7 +1566,7 @@ static size_t MapKeyDataOnlyByteSize(const FieldDescriptor* field,
 }
 
 static size_t MapValueRefDataOnlyByteSize(const FieldDescriptor* field,
-                                          const MapValueRef& value) {
+                                          const MapValueConstRef& value) {
   switch (field->type()) {
     case FieldDescriptor::TYPE_GROUP:
       GOOGLE_LOG(FATAL) << "Unsupported";