test_types_in_equals advice is bad

[Hixie]

The advice in test_types_in_equals is:

  @override
  bool operator ==(Object other) {
    if (identical(this, other)) {
      return true;
    }
    return other is Good &&
        this.someField == other.someField;
  }

...however, this is not a good pattern. If you extend the class and override its operator==, you'll end up with a weird situation where a == b will be true in some cases where b == a is false, if a is the superclass and b the subclass and the difference in the two objects is only on fields that are present in the subclass.

It's better to compare the runtimeType than use is.

(It's also not clear to me that checking identical is a good idea. If it's always a good idea, then we should just have the compiler imply that check before calling the operator. If it's not always a good idea, then why have it in the example. If it's almost always a good idea, let's have a lint for it and let's add it to all the operator==s in Flutter's codebase.)

See also https://github.com/dart-lang/linter/issues/1397 and https://github.com/dart-lang/linter/issues/1398, other issues with this lint.

[pq]

/cc @eernstg @munificent

[eernstg]

I agree that operator == by its documentation must be symmetric, so having a == b and b == a evaluate to different values is just wrong (and the given example could very easily cause this to happen).

However, the Bad example performs an explicit type check (other as Bad), which means that it will throw even though said documentation says that it should never throw. So that example is genuinely bad, but it does not necessarily present a viable (but not so preferable) alternative to the good example. I know, it's not easy to write these examples, especially for equality. ;-)

However, even though if (other.runtimeType != runtimeType) return false; (as in, e.g., #1397) is symmetric and will not throw, it may not always have the most desirable semantics.

For instance, if lists were to have a contents-based equality we might want to consider <num>[1, 2] and <int>[1, 2] as equal, and similar examples could exist for any class that takes a type argument (for instance, if one or more type variables are never used by the parts of the state that determines equality).

We're currently resolving some ambiguities around equality of Type instances that reify types (cf. https://github.com/dart-lang/language/issues/444), but the fact is that this is a delicate question, and implementations do not agree completely. Of course, we could also have an abstract Point class and two concrete implementations PolarPoint and CartesianPoint, and wish to return true if two different implementations denote the same abstract position; so objects may be intended to be equal even though they have different run-time types.

So we probably don't want a lint to say "always compare runtimeTypes".

Given that it is so difficult to write a really perfect equality method, we could aim for something simpler: This lint could focus on detecting cases where an equals method performs a type check that may throw (other as T). It could then have the following form (cf. #1397 again):

@override
bool operator ==(dynamic other) {
  if (other is AvatarImage) {
    if (other.runtimeType != runtimeType) return false;
    return username == typedOther.username
        && photoManager == typedOther.photoManager
        && twitarr == typedOther.twitarr;
  } else {
    return false;
  }
}

This would be slightly slower, but only in the case where other has a type which is a proper subtype of AvatarImage.

This means that we make no attempt to ensure that the operator is symmetric or transitive, but that might not be tractable to verify anyway.

[pq]

Thank you @eernstg! @bwilkerson likely has thoughts too; I recall some discussions we had back when this lint got implemented back in the day...

[bwilkerson]

Yes, equality can be hard to implement correctly. There are a lot of properties that need to be met simultaneously. One question is: what is the purpose of this particular rule? As I remember it, the purpose was to catch one implementation pattern (involving an unguarded use of as) that will sometimes cause an exception to be thrown when the operator is not intended to throw an exception. I think we all agree that this is a bad pattern and that it's worth catching this case.

But I agree with @Hixie that the good example can be misconstrued to be a one-size-fits-all pattern for how the equality operator should be implemented, and it isn't. I can think of three ways to improve the situation.

One would be to remove the good example. I believe that that's not ideal because I believe that users find it helpful to have suggestions for how to fix their issues.

The second is to add more good examples that illustrate some of the other valid patterns for implementing equality checks. The list can't be complete, but it would illustrate that there's more than one correct implementation.

The third, which is orthogonal to the first two, would be to include a link in the docs to a well written description of some of the tradeoffs that need to be considered when implementing equality.

This means that we make no attempt to ensure that the operator is symmetric or transitive, but that might not be tractable to verify anyway.

As far as I'm aware, no, it isn't tractable to statically verify those properties, especially in the presence of subclasses that might not be available for analysis. It also isn't tractable to verify that the method will never throw, but we do know of one pattern that we can catch, so we do. If there are patterns that we can catch that are guaranteed to prevent the method from being either symmetric or transitive we could consider writing lints for those specific cases.

For example, we could have a lint that verifies that in any equality method that overrides a concrete inherited method (other than the implementation in Object), the overriding method only does a type check against the highest class (other than Object) in which equality is defined. Any other type check is likely to break symmetry.

It's also not clear to me that checking identical is a good idea.

The advice in the early days of Dart was to use identical in order to improve the performance. I have no idea whether that's currently valid advice, but I agree that we should update the example if that's not a good practice.

It's better to compare the runtimeType than use is.

As Eric pointed out, that doesn't always the produce the desired semantics. It is, however, sometimes a valid implementation, so perhaps we should include it as a good example.

[eernstg]

A lint which is intended to catch a specific bad action A (like potentially throwing at other as T in an operator ==) will need a 'Good' example which is basically "just something that doesn't do A".

Rather than trying to give an example of that, we could take a variant of @bwilkerson's third approach and replace the good example in the lint by a link to some documentation about the art of writing a suitable operator ==. We have a section in Effective Dart; do you know about anything more specific/detailed than that?

[bwilkerson]

... replace the good example in the lint by a link to some documentation ...

I would consider that to be applying both 1 and 3. :-)

... do you know about anything more specific/detailed than that?

No I don't know of anything better.