Fix issues with protobuf equality comparisons

- Reading a repeated field could change equality comparisons,
because repeated fields are initialized lazily.

- If a GeneratedMessage implements Map, we would compare it as
a map instead of as a GeneratedMessage.

BUG=#48
R=sgjesse@google.com

Review URL: https://chromiumcodereview.appspot.com//1852983002 .

lib/src/protobuf/field_set.dart

@@ -307,7 +307,9 @@ class _FieldSet {
 
   bool _equals(_FieldSet o) {
     if (_meta != o._meta) return false;
-    if (!_areListsEqual(_values, o._values)) return false;
+    for (var i = 0; i < _values.length; i++) {
+      if (!_equalFieldValues(_values[i], o._values[i])) return false;
+    }
 
     if (!_hasExtensions || !_extensions._hasValues) {
       // Check if other extensions are logically empty.
@@ -331,6 +333,28 @@ class _FieldSet {
     return true;
   }
 
+  bool _equalFieldValues(left, right) {
+    if (left != null && right != null) return _deepEquals(left, right);
+
+    var val = left ?? right;
+
+    // Two uninitialized fields are equal.
+    if (val == null) return true;
+
+    // One field is null. We are comparing an initialized field
+    // with its default value.
+
+    // An empty repeated field is the same as uninitialized.
+    // This is because accessing a repeated field automatically creates it.
+    // We don't want reading a field to change equality comparisons.
+    if (val is List && val.isEmpty) return true;
+
+    // For now, initialized and uninitialized fields are different.
+    // TODO(skybrian) consider other cases; should we compare with the
+    // default value or not?
+    return false;
+  }
+
   /// Calculates a hash code based on the contents of the protobuf.
   ///
   /// The hash may change when any field changes (recursively).

lib/src/protobuf/utils.dart

@@ -6,6 +6,9 @@ part of protobuf;
 
 // TODO(antonm): reconsider later if PbList should take care of equality.
 bool _deepEquals(lhs, rhs) {
+  // Some GeneratedMessages implement Map, so test this first.
+  if (lhs is GeneratedMessage) return lhs == rhs;
+  if (rhs is GeneratedMessage) return false;
   if ((lhs is List) && (rhs is List)) return _areListsEqual(lhs, rhs);
   if ((lhs is Map) && (rhs is Map)) return _areMapsEqual(lhs, rhs);
   if ((lhs is ByteData) && (rhs is ByteData)) {

test/event_test.dart

@@ -13,11 +13,11 @@ import 'package:protobuf/src/protobuf/mixins/event_mixin.dart'
     show PbEventMixin, PbFieldChange;
 import 'package:test/test.dart' show test, expect, predicate, same;
 
-import 'mock_util.dart' show MockMessage;
+import 'mock_util.dart' show MockMessage, mockInfo;
 
 class Rec extends MockMessage with PbEventMixin {
-  get className => "Rec";
-  Rec create() => new Rec();
+  get info_ => _info;
+  static final _info = mockInfo("Rec", () => new Rec());
 }
 
 Extension comment = new Extension("Rec", "comment", 5, PbFieldType.OS);

test/json_test.dart

@@ -6,11 +6,11 @@ library json_test;
 import 'dart:convert';
 import 'package:test/test.dart';
 
-import 'mock_util.dart' show MockMessage;
+import 'mock_util.dart' show MockMessage, mockInfo;
 
 class T extends MockMessage {
-  get className => "T";
-  T create() => new T();
+  get info_ => _info;
+  static final _info = mockInfo("T", () => new T());
 }
 
 main() {

test/map_mixin_test.dart

@@ -12,12 +12,12 @@ import 'dart:collection' show MapMixin;
 import 'package:protobuf/src/protobuf/mixins/map_mixin.dart';
 import 'package:test/test.dart' show test, expect, predicate, same, throws;
 
-import 'mock_util.dart' show MockMessage;
+import 'mock_util.dart' show MockMessage, mockInfo;
 
 // A minimal protobuf implementation compatible with PbMapMixin.
 class Rec extends MockMessage with MapMixin, PbMapMixin {
-  get className => "Rec";
-  Rec create() => new Rec();
+  get info_ => _info;
+  static final _info = mockInfo("Rec", () => new Rec());
 
   @override
   String toString() => "Rec(${val}, \"${str}\")";
@@ -65,4 +65,42 @@ main() {
     expect(r["int32s"], [123]);
     expect(r["int32s"], same(r["int32s"]));
   });
+
+  test('operator== and hashCode work for Map mixin', () {
+    var a = new Rec();
+    expect(a == a, true);
+    expect(a == {}, false);
+    expect({} == a, false);
+
+    var b = new Rec();
+    expect(a.info_ == b.info_, true, reason: "BuilderInfo should be the same");
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+
+    a.val = 123;
+    expect(a == b, false);
+    b.val = 123;
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+
+    a.child = new Rec();
+    expect(a == b, false);
+    b.child = new Rec();
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+  });
+
+  test("protobuf doesn't compare equal to a map with the same values", () {
+    var a = new Rec();
+    expect(a == new Map.from(a), false);
+    expect(new Map.from(a) == a, false);
+  });
+
+  test("reading protobuf values shouldn't change equality", () {
+    var a = new Rec();
+    var b = new Rec();
+    expect(a == b, true);
+    new Map.from(a);
+    expect(a == b, true);
+  });
 }

test/message_test.dart

@@ -7,19 +7,19 @@ library message_test;
 
 import 'package:test/test.dart' show test, expect, predicate, throwsA;
 
-import 'mock_util.dart' show MockMessage;
+import 'mock_util.dart' show MockMessage, mockInfo;
 
 class Rec extends MockMessage {
-  get className => "Rec";
-  Rec create() => new Rec();
+  get info_ => _info;
+  static final _info = mockInfo("Rec", () => new Rec());
 }
 
-throwsError(Type expectedType, String expectedMessage) => throwsA(
-    predicate((x) {
-  expect(x.runtimeType, expectedType);
-  expect(x.message, expectedMessage);
-  return true;
-}));
+throwsError(Type expectedType, String expectedMessage) =>
+    throwsA(predicate((x) {
+      expect(x.runtimeType, expectedType);
+      expect(x.message, expectedMessage);
+      return true;
+    }));
 
 main() {
   test('getField with invalid tag throws exception', () {
@@ -35,4 +35,26 @@ main() {
       r.getDefaultForField(123);
     }, throwsError(ArgumentError, "tag 123 not defined in Rec"));
   });
+
+  test('operator== and hashCode work for a simple record', () {
+    var a = new Rec();
+    expect(a == a, true);
+
+    var b = new Rec();
+    expect(a.info_ == b.info_, true, reason: "BuilderInfo should be the same");
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+
+    a.val = 123;
+    expect(a == b, false);
+    b.val = 123;
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+
+    a.child = new Rec();
+    expect(a == b, false);
+    b.child = new Rec();
+    expect(a == b, true);
+    expect(a.hashCode, b.hashCode);
+  });
 }

test/mock_util.dart

@@ -5,15 +5,20 @@
 library mock_util;
 
 import 'package:protobuf/protobuf.dart'
-    show GeneratedMessage, BuilderInfo, PbFieldType;
+    show GeneratedMessage, BuilderInfo, CreateBuilderFunc, PbFieldType;
+
+BuilderInfo mockInfo(String className, CreateBuilderFunc create) {
+  return new BuilderInfo(className)
+    ..a(1, "val", PbFieldType.O3, 42)
+    ..a(2, "str", PbFieldType.OS)
+    ..a(3, "child", PbFieldType.OM, create, create)
+    ..p(4, "int32s", PbFieldType.P3);
+}
 
 /// A minimal protobuf implementation for testing.
 abstract class MockMessage extends GeneratedMessage {
-  BuilderInfo _infoCache;
-
   // subclasses must provide these
-  String get className;
-  MockMessage create();
+  BuilderInfo get info_;
 
   int get val => $_get(0, 1, 42);
   set val(x) => setField(1, x);
@@ -26,16 +31,8 @@ abstract class MockMessage extends GeneratedMessage {
 
   List<int> get int32s => $_get(3, 4, null);
 
-  @override
-  BuilderInfo get info_ {
-    if (_infoCache != null) return _infoCache;
-    _infoCache = new BuilderInfo(className)
-      ..a(1, "val", PbFieldType.O3, 42)
-      ..a(2, "str", PbFieldType.OS)
-      ..a(3, "child", PbFieldType.OM, create, create)
-      ..p(4, "int32s", PbFieldType.P3);
-    return _infoCache;
+  clone() {
+    CreateBuilderFunc create = info_.byName["child"].subBuilder;
+    return create()..mergeFromMessage(this);
   }
-
-  clone() => create()..mergeFromMessage(this);
 }