Comparators of Interval and Pitch do not agree with ==

[plammens]

The comparison operators <, >, <=, >= of Interval and Pitch do not agree with the == operator.
In particular, the relation defined by <= is not antisymmetric, namely that if a <= b and a >= b then a == b.

Example

void main() {
  final eSharp = Note.e.sharp.inOctave(1);
  final f = Note.f.inOctave(1);
  [
    eSharp <= f,  // true
    eSharp >= f,  // true
    eSharp < f,  // false
    eSharp > f,  // false
    eSharp == f,  // false
  ].forEach(print);
}

Expected: eSharp == f to be true.

Why does this matter?

This could potentially confuse users, as usually one expects this property to hold.
For example:

if (a < b) {...}
else if (a > b) {...}
else {
  // user now assumes that a == b, but this might not be true currently
}

Cause

This occurs because the comparison operators and the equality operator are based on different criteria: the former only use the semitones, while the latter uses the properties, including spelling.

The compareTo method is equivalent to == in both cases (uses all the properties).

Possible solutions

The Dart documentation for Comparable says:

It is recommended that the order of a Comparable agrees with its operator operator == equality (a.compareTo(b) == 0 iff a == b), but this is not a requirement. For example, double and DateTime have compareTo methods that do not agree with operator operator ==. For doubles the compareTo method is more precise than the equality, and for DateTime it is less precise.

[...]

The Comparable interface does not imply the existence of the comparison operators <, <=, > and >=. These should only be defined if the ordering is a less-than/greater-than ordering, that is, an ordering where you would naturally use the words "less than" about the order of two elements.

If the equality operator and compareTo disagree, the comparison operators should follow the equality operator, and will likely also disagree with compareTo. Otherwise they should match the compareTo method, so that a < b iff a.compareTo(b) < 0.

So, the docs allow disagreement between the comparators and compareTo, but not within the comparators themselves.
I personally prefer that everything agrees, it makes it more intuitive and less error-prone.

Keeping this in mind, I see the following possible solutions:

• Make == and all the other comparators agree exactly with compareTo. To make this easier to implement and less error-prone, it could be useful to use a mixin such as the following:

  /// Adds comparison operators to a comparable class with a total order.
  mixin Comparators<T> implements Comparable<T> {
    @override
    bool operator ==(other) => other is T && compareTo(other as T) == 0;
  
    bool operator <(T other) => compareTo(other) < 0;
    bool operator <=(T other) => compareTo(other) <= 0;
    bool operator >(T other) => compareTo(other) > 0;
    bool operator >=(T other) => compareTo(other) >= 0;
  }

  Then, if the user wants to compare just the semitones, they are expected to do it themselves: a.semitones <= b.semitones.

• Make separate classes Pitch and PitchSpelling. PitchSpelling would be the current Pitch, and Pitch would only have octave and semitones (without note spelling). Remove the order comparators from PitchSpelling (but keep compareTo), only add them to Pitch. (Similarly for Interval and IntervalSpelling.)

• Change the == operator to make it agree with the other comparators, namely:

  bool operator==(other) => other is Pitch && semitones == other.semitones;

[albertms10]

Totally agree. I would choose the first solution: to make each comparator operator agree with compareTo.

Thank you, indeed, for such a great and detailed breakdown!