Change GeneratedMessage.toBuilder() to only make a shallow copy (#145)

protobuf/CHANGELOG.md

@@ -1,3 +1,12 @@
+## 0.11.0
+
+* Breaking change: changed semantics of `GeneratedMessage.toBuilder()`` to only make a shallow copy.
+
+  `GeneratedMessage` has a new abstract method: `createEmptyInstance()` that subclasses must
+  implement.
+
+  Proto files must be rebuilt using protoc_plugin 0.15.0 or newer.
+
 ## 0.10.8
 
 * Fix freezing of map fields.

protobuf/lib/meta.dart

@@ -11,6 +11,7 @@ const GeneratedMessage_reservedNames = const <String>[
   'hashCode',
   'noSuchMethod',
   'copyWith',
+  'createEmptyInstance',
   'runtimeType',
   'toString',
   'freeze',

protobuf/lib/src/protobuf/extension_field_set.dart

@@ -125,4 +125,24 @@ class _ExtensionFieldSet {
       _areMapsEqual(_values, other._values);
 
   void _clearValues() => _values.clear();
+
+  /// Makes a shallow copy of all values from [original] to this.
+  ///
+  /// Repeated fields are copied.
+  /// Extensions cannot contain map fields.
+  void _shallowCopyValues(_ExtensionFieldSet original) {
+    for (int tagNumber in original._tagNumbers) {
+      Extension extension = original._getInfoOrNull(tagNumber);
+      _addInfoUnchecked(extension);
+
+      final value = original._getFieldOrNull(extension);
+      if (value == null) continue;
+      if (extension.isRepeated) {
+        assert(value is PbList);
+        _ensureRepeatedField(extension)..addAll(value);
+      } else {
+        _setFieldUnchecked(extension, value);
+      }
+    }
+  }
 }

protobuf/lib/src/protobuf/field_info.dart

@@ -6,6 +6,8 @@ part of protobuf;
 
 /// An object representing a protobuf message field.
 class FieldInfo<T> {
+  FrozenPbList<T> _emptyList;
+
   final String name;
   final int tagNumber;
   final int index; // index of the field's value. Null for extensions.
@@ -80,7 +82,9 @@ class FieldInfo<T> {
   /// Returns a read-only default value for a field.
   /// (Unlike getField, doesn't create a repeated field.)
   get readonlyDefault {
-    if (isRepeated) return const <Null>[];
+    if (isRepeated) {
+      return _emptyList ??= FrozenPbList._([]);
+    }
     return makeDefault();
   }
 

protobuf/lib/src/protobuf/field_set.dart

@@ -83,7 +83,7 @@ class _FieldSet {
   bool get hasUnknownFields => _unknownFields != null;
 
   _ExtensionFieldSet _ensureExtensions() {
-    assert(!_isReadOnly);
+    _ensureWritable();
     if (!_hasExtensions) _extensions = new _ExtensionFieldSet(this);
     return _extensions;
   }
@@ -744,4 +744,35 @@ class _FieldSet {
     // TODO(skybrian): search extensions as well
     // https://github.com/dart-lang/protobuf/issues/46
   }
+
+  /// Makes a shallow copy of all values from [original] to this.
+  ///
+  /// Map fields and repeated fields are copied.
+  void _shallowCopyValues(_FieldSet original) {
+    _values.setRange(0, original._values.length, original._values);
+    for (int index = 0; index < _meta.byIndex.length; index++) {
+      FieldInfo fieldInfo = _meta.byIndex[index];
+      if (fieldInfo.isMapField) {
+        PbMap map = _values[index];
+        if (map != null) {
+          _values[index] = (fieldInfo as MapFieldInfo)._createMapField(_message)
+            ..addAll(map);
+        }
+      } else if (fieldInfo.isRepeated) {
+        PbListBase list = _values[index];
+        if (list != null) {
+          _values[index] = fieldInfo._createRepeatedField(_message)
+            ..addAll(list);
+        }
+      }
+    }
+
+    if (original._hasExtensions) {
+      _ensureExtensions()._shallowCopyValues(original._extensions);
+    }
+
+    if (original.hasUnknownFields) {
+      _ensureUnknownFields()._fields?.addAll(original._unknownFields._fields);
+    }
+  }
 }

protobuf/lib/src/protobuf/generated_message.dart

@@ -46,9 +46,11 @@ abstract class GeneratedMessage {
 
   /// Creates a deep copy of the fields in this message.
   /// (The generated code uses [mergeFromMessage].)
-  // TODO(nichite): preserve frozen state on clone.
   GeneratedMessage clone();
 
+  /// Creates an empty instance of the same message type as this.
+  GeneratedMessage createEmptyInstance();
+
   UnknownFieldSet get unknownFields => _fieldSet._ensureUnknownFields();
 
   /// Make this message read-only.
@@ -59,10 +61,22 @@ abstract class GeneratedMessage {
     return this;
   }
 
-  /// Returns a writable copy of this message.
-  // TODO(nichite): Return an actual builder object that lazily creates builders
-  // for sub-messages, instead of cloning everything here.
-  GeneratedMessage toBuilder() => clone();
+  /// Returns a writable, shallow copy of this message.
+  ///
+  /// Sub messages will be shared with [this] and will still be frozen if [this]
+  /// is frozen.
+  ///
+  /// The lists representing repeated fields are copied. But their elements will
+  /// be shared with the corresponding list in [this].
+  ///
+  /// Similarly for map fields, the maps will be copied, but share the elements.
+  // TODO(nichite, sigurdm): Consider returning an actual builder object that
+  // lazily creates builders.
+  GeneratedMessage toBuilder() {
+    final result = createEmptyInstance();
+    result._fieldSet._shallowCopyValues(_fieldSet);
+    return result;
+  }
 
   /// Apply [updates] to a copy of this message.
   ///

protobuf/pubspec.yaml

@@ -1,5 +1,5 @@
 name: protobuf
-version: 0.10.8
+version: 0.11.0
 author: Dart Team <misc@dartlang.org>
 description: >
   Runtime library for protocol buffers support.

protobuf/test/event_test.dart

@@ -18,6 +18,7 @@ import 'mock_util.dart' show MockMessage, mockInfo;
 class Rec extends MockMessage with PbEventMixin {
   get info_ => _info;
   static final _info = mockInfo("Rec", () => new Rec());
+  Rec createEmptyInstance() => new Rec();
 }
 
 Extension comment = new Extension("Rec", "comment", 6, PbFieldType.OS);

protobuf/test/json_test.dart

@@ -68,7 +68,6 @@ checkJsonMap(Map m) {
   expect(m.length, 3);
   expect(m["1"], 123);
   expect(m["2"], "hello");
-  print(m.toString());
   expect(m["4"], [1, 2, 3]);
 }
 

protobuf/test/map_mixin_test.dart

@@ -18,6 +18,7 @@ import 'mock_util.dart' show MockMessage, mockInfo;
 class Rec extends MockMessage with MapMixin, PbMapMixin {
   get info_ => _info;
   static final _info = mockInfo("Rec", () => new Rec());
+  Rec createEmptyInstance() => new Rec();
 
   @override
   String toString() => "Rec(${val}, \"${str}\")";

protobuf/test/message_test.dart

@@ -12,6 +12,7 @@ import 'mock_util.dart' show MockMessage, mockInfo;
 class Rec extends MockMessage {
   get info_ => _info;
   static final _info = mockInfo("Rec", () => new Rec());
+  Rec createEmptyInstance() => new Rec();
 }
 
 throwsError(Type expectedType, String expectedMessage) =>

protobuf/test/mock_util.dart

@@ -45,4 +45,5 @@ abstract class MockMessage extends GeneratedMessage {
 class T extends MockMessage {
   get info_ => _info;
   static final _info = mockInfo("T", () => new T());
+  T createEmptyInstance() => new T();
 }

protobuf/test/readonly_message_test.dart

@@ -26,6 +26,7 @@ throwsError(Type expectedType, Matcher expectedMessage) =>
 class Rec extends GeneratedMessage {
   static Rec getDefault() => new Rec()..freeze();
   static Rec create() => new Rec();
+  Rec createEmptyInstance() => new Rec();
 
   @override
   BuilderInfo info_ = new BuilderInfo('rec')
@@ -221,7 +222,7 @@ main() {
     expect(rebuilt.sub.length, 2);
   });
 
-  test("can modify sub-messages while rebuilding a frozen message", () {
+  test("cannot modify sub-messages while rebuilding a frozen message", () {
     final subMessage = Rec.create()..value = 1;
     final orig = Rec.create()
       ..sub.add(Rec.create()..sub.add(subMessage))
@@ -232,7 +233,12 @@ main() {
           () => subMessage.value = 5,
           throwsError(UnsupportedError,
               equals('Attempted to change a read-only message (rec)')));
-      m.sub[0].sub[0].value = 2;
+      expect(
+          () => m.sub[0].sub[0].value = 2,
+          throwsError(UnsupportedError,
+              equals('Attempted to change a read-only message (rec)')));
+      m.sub[0] = m.sub[0].copyWith(
+          (m2) => m2.sub[0] = m2.sub[0].copyWith((m3) => m3.value = 2));
     });
     expect(identical(subMessage, orig.sub[0].sub[0]), true);
     expect(identical(subMessage, rebuilt.sub[0].sub[0]), false);

protoc_plugin/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 14.0.0
+
+* Breaking change: generated message classes now have a new instance method `createEmptyInstance`
+  that is used to support the new `toBuilder()` semantics of protobuf 0.11.0.
+  The generated code requires at least protobuf 0.11.0.
+
 ## 13.0.1
 
 * Add test for recursive merging and update protobuf dependency to 0.10.7.

protoc_plugin/lib/message_generator.dart

@@ -330,6 +330,8 @@ class MessageGenerator extends ProtobufContainer {
       // Factory functions which can be used as default value closures.
       out.println('static ${classname} create() =>'
           ' new ${classname}();');
+      out.println('${classname} createEmptyInstance() => create();');
+
       out.println(
           'static $_protobufImportPrefix.PbList<${classname}> createRepeated() =>'
           ' new $_protobufImportPrefix.PbList<${classname}>();');

protoc_plugin/lib/src/dart_options.pb.dart

@@ -29,6 +29,7 @@ class DartMixin extends $pb.GeneratedMessage {
       super.copyWith((message) => updates(message as DartMixin));
   $pb.BuilderInfo get info_ => _i;
   static DartMixin create() => new DartMixin();
+  DartMixin createEmptyInstance() => create();
   static $pb.PbList<DartMixin> createRepeated() => new $pb.PbList<DartMixin>();
   static DartMixin getDefault() => _defaultInstance ??= create()..freeze();
   static DartMixin _defaultInstance;
@@ -80,6 +81,7 @@ class Imports extends $pb.GeneratedMessage {
       super.copyWith((message) => updates(message as Imports));
   $pb.BuilderInfo get info_ => _i;
   static Imports create() => new Imports();
+  Imports createEmptyInstance() => create();
   static $pb.PbList<Imports> createRepeated() => new $pb.PbList<Imports>();
   static Imports getDefault() => _defaultInstance ??= create()..freeze();
   static Imports _defaultInstance;