`value is ...` checks in the library are potentially wrong/buggy?

[osa1]

Imagine having this class:

class MyCls implements MyMsg {
  ...
}

where MyMsg is a proto message that implements a mixin:

message MyMsg {
  option (dart_options.mixin) = "MyMixin";
  ...
}

and MyMixin implements Map:

class MyMixin implements Map<String, String> {
  ...
}

So MyCls is both a MyMsg (which is a GeneratedMessage) and a Map, and Dart expressions MyCls() is GeneratedMessage and MyCls() is Map both return true.

Now, if I have another message with a MyMsg field:

message AnotherMessage {
  MyMsg my_msg = 1;
  ...
}

I can set myMsg field MyCls, which is both a GeneratedMessage and a Map. As a result, this code goes wrong:

protobuf.dart/protobuf/lib/src/protobuf/field_set.dart

Lines 687 to 712 in a92abc5

	static int _hashField(int hash, FieldInfo fi, value) {
	if (value is List && value.isEmpty) {
	return hash; // It's either repeated or an empty byte array.
	}
	if (value is Map && value.isEmpty) {
	return hash;
	}
	hash = _HashUtils._combine(hash, fi.tagNumber);
	if (_isBytes(fi.type)) {
	// Bytes are represented as a List<int> (Usually with byte-data).
	// We special case that to match our equality semantics.
	hash = _HashUtils._combine(hash, _HashUtils._hashObjects(value));
	} else if (!_isEnum(fi.type)) {
	hash = _HashUtils._combine(hash, value.hashCode);
	} else if (fi.isRepeated) {
	hash = _HashUtils._combine(
	hash, _HashUtils._hashObjects(value.map((enm) => enm.value)));
	} else {
	ProtobufEnum enm = value;
	hash = _HashUtils._combine(hash, enm.value);
	}
	return hash;
	}

The problem is value is Map is true, but the field type is actually a message and using the Map interface (in particular, isEmpty) is not right. It's possible that the Map interface of the message is empty, but the message has fields.

This may look like a convoluted setup, but I've just wasted a few hours debugging this exact issue in a downstream code. (thanks @sigurdm for the help with debugging this)

I think we should avoid checking field value types (e.g. value is List and value is Map above) and always rely on _FieldInfo type. If _FieldInfo says repeated, we cast to List. If it says map, we cast to Map, and so on.

Any thoughts @sigurdm?

---

To see how can a type be both a GeneratedMessage and a Map, here's a demos showing that a Dart type can subtype multiple types that are not in relation to each other:

class A {
  String getCls() {
    return 'A';
  }
}

abstract class B implements A {
  @override
  String getCls() {
    return 'B';
  }
}

class C {
  String getCls() {
    return 'C';
  }
}

class D1 extends C with B {}
class D2 with B implements C {}

void main() {
  print(D1() is C); // true
  print(D1() is B); // true

  print(D2() is C); // true
  print(D2() is B); // true
}

[sigurdm]

Any thoughts @sigurdm?

Yes, relying on fieldinfo seems to be the most reliable thing here!

[lrhn]

So MyCls is both a MyMsg (which is a GeneratedMessage) and a Map,

Any chance of just not doing that? The Map interface is fairly big in Dart, and has a number of getters (short, plain noun names), so the risk of name conflict with another interface is no insignificant.

I generally recommend not adding members to the core interfaces (List/Map/Future are the most common ones people want to "enhance", Stream has historically been extended by dart:io). The reason to implement those interfaces is usually that you want to pass the object to someone else expecting a List/Map/Future.

It's safer to have a map than to be a map.

(But that ship might have sailed a long time ago, if this is pre-existing behavior. And if it's users asking for it, they get what they asked for, and we shouldn't misbehave. In which case, what @sigurdm said!)

[osa1]

Thinking about this a bit more.. I think value is X checks would be OK if we had a "sealed" class for each field type. That would avoid types that are both field type A and B (like GeneratedMessage and Map in the original issue), and we could manually make sure we cover all cases when we check for field types.

This is basically poorly emulating sum types in languages like Haskell, OCaml, Rust.

This seems a bit difficult to implement in Dart because

1. We don't have "sealed" classes. (a sealed class is a class that cannot be subclassed)
2. We want the generated code to have GeneratedMessage subclasses, so with sealed classes, we would need a way to override the subclass check and allow generated code to have GeneratedMessage subclasses.

With the current language and the set of features we have, I think the best we could do is either

1. Use field types as suggested in my original message
2. Or, document that users should not subclass more than one of PbList, PbMap, and GeneratedMessage in a single class, and then update the library code so that we don't check if a value is List or Map, but check if it's a PbList or a PbMap.

With (2), if we also introduce classes for int, bool etc. then we could also take a more "OO" (?) approach and e.g. have a base class PbField with methods serialize, isDefault, etc. This would get rid of type checks and casts and use dynamic dispatch.

(1) is backwards compatible and not a lot of work. (2) requires significant refactoring.

[sigurdm]

This seems a bit difficult to implement in Dart because

1. We don't have "sealed" classes. (a sealed class is a class that cannot be subclassed)

We do have https://pub.dev/documentation/meta/latest/meta/sealed-constant.html

It doesn't provide exhaustiveness checking though...