Update docs for incomplete ordering + inconsistent hashing

Author: joefarebrother

python/ql/src/Classes/Comparisons/EqualsOrHash.qhelp

@@ -4,42 +4,40 @@
 <qhelp>
 
 <overview>
-<p>In order to conform to the object model, classes that define their own equality method should also 
-define their own hash method, or be unhashable. If the hash method is not defined then the <code>hash</code> of the 
-super class is used. This is unlikely to result in the expected behavior.</p>
+<p>A hashable class has an <code>__eq__</code> method, and a <code>__hash__</code> method that agrees with equality. 
+When a hash method is defined, an equality method should also be defined; otherwise object identity is used for equality comparisons 
+which may not be intended.
+ </p>
 
-<p>A class can be made unhashable by setting its <code>__hash__</code> attribute to <code>None</code>.</p>
-
-<p>In Python 3, if you define a class-level equality method and omit a <code>__hash__</code> method 
-then the class is automatically marked as unhashable.</p>
+<p>Note that defining an <code>__eq__</code> method without defining a <code>__hash__</code> method automatically makes the class unhashable in Python 3.
+(even if a superclass defines a hash method).</p>
 
 </overview>
 <recommendation>
 
-<p>When you define an <code>__eq__</code> method for a class, remember to implement a <code>__hash__</code> method or set 
-<code>__hash__ = None</code>.</p>
+<p>
+If a <code>__hash__</code> method is defined, ensure a compatible <code>__eq__</code> method is also defined.
+</p>
+
+<p>
+To explicitly declare a class as unhashable, set <code>__hash__ = None</code>, rather than defining a <code>__hash__</code> method that always
+raises an exception. Otherwise, the class would be incorrectly identified as hashable by an <code>isinstance(obj, collections.abc.Hashable)</code> call.
+</p>
 
 </recommendation>
 <example>
-<p>In the following example the <code>Point</code> class defines an equality method but 
-no hash method. If hash is called on this class then the hash method defined for <code>object</code> 
-is used. This is unlikely to give the required behavior. The <code>PointUpdated</code> class 
-is better as it defines both an equality and a hash method.
-If <code>Point</code> was not to be used in <code>dict</code>s or <code>set</code>s, then it could be defined as
-<code>UnhashablePoint</code> below.
+<p>In the following example, the <code>A</code> class defines an hash method but 
+no equality method. Equality will be determined by object identity, which may not be the expected behaviour.
 </p>
-<p>
-To comply fully with the object model this class should also define an inequality method (identified 
-by a separate rule).</p>
 
-<sample src="EqualsOrHash.py" />
+<sample src="examples/EqualsOrHash.py" />
 
 </example>
 <references>
 
 
 <li>Python Language Reference: <a href="http://docs.python.org/reference/datamodel.html#object.__hash__">object.__hash__</a>.</li>
-<li>Python Glossary: <a href="http://docs.python.org/2/glossary.html#term-hashable">hashable</a>.</li>
+<li>Python Glossary: <a href="http://docs.python.org/3/glossary.html#term-hashable">hashable</a>.</li>
 
 
 </references>

python/ql/src/Classes/Comparisons/EqualsOrHash.ql

@@ -1,6 +1,6 @@
 /**
  * @name Inconsistent equality and hashing
- * @description Defining equality for a class without also defining hashability (or vice-versa) violates the object model.
+ * @description Defining a hash operation without defining equality may be a mistake.
  * @kind problem
  * @tags quality
  *       reliability

python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql

@@ -1,6 +1,6 @@
 /**
  * @name Inconsistent equality and inequality
- * @description Defining only an equality method or an inequality method for a class violates the object model.
+ * @description Class definitions of equality and inequality operators may be inconsistent.
  * @kind problem
  * @tags quality
  *       reliability

python/ql/src/Classes/Comparisons/IncompleteOrdering.qhelp

@@ -3,32 +3,34 @@
   "qhelp.dtd">
 <qhelp>
 <overview>
-<p> A class that implements an ordering operator
-(<code>__lt__</code>, <code>__gt__</code>, <code>__le__</code> or <code>__ge__</code>) should implement
-all four in order that ordering between two objects is consistent and obeys the usual mathematical rules.
-If the ordering is inconsistent with default equality, then <code>__eq__</code> and  <code>__ne__</code>
-should also be implemented.
+<p> A class that implements the rich comparison operators
+(<code>__lt__</code>, <code>__gt__</code>, <code>__le__</code>, or <code>__ge__</code>) should ensure that all four 
+comparison operations <code>&lt;</code>, <code>&lt;=</code>, <code>&gt;</code>, and <code>&gt;=</code> function as expected, consistent 
+with expected mathematical rules.   
+In Python 3, this is ensured by implementing one of <code>__lt__</code> or <code>__gt__</code>, and one of  <code>__le__</code> or <code>__ge__</code>.
+If the ordering is not consistent with default equality, then <code>__eq__</code> should also be implemented.
 </p>
 
 </overview>
 <recommendation>
-<p>Ensure that all four ordering comparisons are implemented as well as <code>__eq__</code> and <code>
-__ne__</code> if required.</p>
+<p>Ensure that at least one of <code>__lt__</code> or <code>__gt__</code> and at least one of <code>__le__</code> or <code>__ge__</code> is defined. 
+  </p>
 
-<p>It is not necessary to manually implement all four comparisons,
-the <code>functools.total_ordering</code> class decorator can be used.</p>
+<p>
+The <code>functools.total_ordering</code> class decorator can be used to automatically implement all four comparison methods from a single one,
+which is typically the cleanest way to ensure all necessary comparison methods are implemented consistently.</p>
 
 </recommendation>
 <example>
-<p>In this example only the <code>__lt__</code> operator has been implemented which could lead to
-inconsistent behavior. <code>__gt__</code>, <code>__le__</code>, <code>__ge__</code>, and in this case,
-<code>__eq__</code> and <code>__ne__</code> should be implemented.</p>
-<sample src="IncompleteOrdering.py" />
+<p>In the following example, only the <code>__lt__</code> operator has been implemented, which would lead to unexpected
+errors if the <code>&lt;=</code> or <code>&gt;=</code> operators are used on <code>A</code> instances.
+The <code>__le__</code> method should also be defined, as well as <code>__eq__</code> in this case.</p>
+<sample src="examples/IncompleteOrdering.py" />
 
 </example>
 <references>
 
-  <li>Python Language Reference: <a href="http://docs.python.org/2/reference/datamodel.html#object.__lt__">Rich comparisons in Python</a>.</li>
+  <li>Python Language Reference: <a href="http://docs.python.org/3/reference/datamodel.html#object.__lt__">Rich comparisons in Python</a>.</li>
 
 
 </references>

python/ql/src/Classes/Comparisons/IncompleteOrdering.ql

@@ -1,6 +1,6 @@
 /**
  * @name Incomplete ordering
- * @description Class defines one or more ordering method but does not define all 4 ordering comparison methods
+ * @description Class defines ordering comparison methods, but does not define both strict and nonstrict ordering methods, to ensure all four comparison operators behave as expected.
  * @kind problem
  * @tags quality
  *       reliability

python/ql/src/Classes/Comparisons/examples/EqualsOrHash.py

@@ -1,52 +1,8 @@
-# Incorrect: equality method defined but class contains no hash method
-class Point(object):
-
-    def __init__(self, x, y):
-        self._x = x
-        self._y = y
-
-    def __repr__(self):
-        return 'Point(%r, %r)' % (self._x, self._y)
-
-    def __eq__(self, other):
-        if not isinstance(other, Point):
-            return False
-        return self._x == other._x and self._y == other._y
-
-
-# Improved: equality and hash method defined (inequality method still missing)
-class PointUpdated(object):
-
-    def __init__(self, x, y):
-        self._x = x
-        self._y = y
-
-    def __repr__(self):
-        return 'Point(%r, %r)' % (self._x, self._y)
-
-    def __eq__(self, other):
-        if not isinstance(other, Point):
-            return False
-        return self._x == other._x and self._y == other._y
-
-    def __hash__(self):  
-        return hash(self._x) ^ hash(self._y)
-
-# Improved: equality method defined and class instances made unhashable
-class UnhashablePoint(object):
-
-    def __init__(self, x, y):
-        self._x = x
-        self._y = y
-
-    def __repr__(self):
-        return 'Point(%r, %r)' % (self._x, self._y)
-
-    def __eq__(self, other):
-        if not isinstance(other, Point):
-            return False
-        return self._x == other._x and self._y == other._y
-
-    #Tell the interpreter that instances of this class cannot be hashed
-    __hash__ = None
-
+class A:
+    def __init__(self, a, b):
+        self.a = a 
+        self.b = b
+
+    # No equality method is defined
+    def __hash__(self):
+        return hash((self.a, self.b))

python/ql/src/Classes/Comparisons/examples/EqualsOrNotEquals.py

@@ -31,26 +31,3 @@ def __ne__(self, other):  # Improved: equality and inequality method defined (ha
         return not self == other
 
 
-
-class A:
-    def __init__(self, a):
-        self.a = a 
-
-    def __eq__(self, other):
-        print("A eq")
-        return self.a == other.a
-
-    def __ne__(self, other):
-        print("A ne")
-        return self.a != other.a 
-
-class B(A):
-    def __init__(self, a, b):
-        self.a = a
-        self.b = b 
-
-    def __eq__(self, other):
-        print("B eq")
-        return self.a == other.a and self.b == other.b
-
-print(B(1,2) != B(1,3)) 

python/ql/src/Classes/Comparisons/examples/IncompleteOrdering.py

@@ -1,6 +1,8 @@
-class IncompleteOrdering(object):
+class A:
     def __init__(self, i):
         self.i = i
 
+    # BAD: le is not defined, so `A(1) <= A(2) would result in an error.`
     def __lt__(self, other):
-        return self.i < other.i
+        return self.i < other.i
+