Merge pull request #20038 from joefarebrother/python-qual-comparison

Python: Modernize 3 quality queries for comparison methods

python/ql/integration-tests/query-suite/python-code-quality-extended.qls.expected

@@ -1,6 +1,8 @@
+ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
+ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
+ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
 ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
 ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
-ql/python/ql/src/Classes/EqualsOrHash.ql
 ql/python/ql/src/Classes/InconsistentMRO.ql
 ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
 ql/python/ql/src/Classes/MissingCallToDel.ql

python/ql/integration-tests/query-suite/python-code-quality.qls.expected

@@ -1,6 +1,8 @@
+ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
+ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
+ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
 ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
 ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
-ql/python/ql/src/Classes/EqualsOrHash.ql
 ql/python/ql/src/Classes/InconsistentMRO.ql
 ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
 ql/python/ql/src/Classes/MissingCallToDel.ql

python/ql/integration-tests/query-suite/python-security-and-quality.qls.expected

@@ -1,8 +1,8 @@
+ql/python/ql/src/Classes/Comparisons/EqualsOrHash.ql
+ql/python/ql/src/Classes/Comparisons/EqualsOrNotEquals.ql
+ql/python/ql/src/Classes/Comparisons/IncompleteOrdering.ql
 ql/python/ql/src/Classes/ConflictingAttributesInBaseClasses.ql
 ql/python/ql/src/Classes/DefineEqualsWhenAddingAttributes.ql
-ql/python/ql/src/Classes/EqualsOrHash.ql
-ql/python/ql/src/Classes/EqualsOrNotEquals.ql
-ql/python/ql/src/Classes/IncompleteOrdering.ql
 ql/python/ql/src/Classes/InconsistentMRO.ql
 ql/python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql
 ql/python/ql/src/Classes/MissingCallToDel.ql

python/ql/lib/semmle/python/Class.qll

@@ -91,6 +91,12 @@ class Class extends Class_, Scope, AstNode {
   /** Gets a method defined in this class */
   Function getAMethod() { result.getScope() = this }
 
+  /** Gets the method defined in this class with the specified name, if any. */
+  Function getMethod(string name) {
+    result = this.getAMethod() and
+    result.getName() = name
+  }
+
   override Location getLocation() { py_scope_location(result, this) }
 
   /** Gets the scope (module, class or function) in which this class is defined */

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

@@ -0,0 +1,44 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<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>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>
+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>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>
+
+<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/3/glossary.html#term-hashable">hashable</a>.</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,26 @@
+/**
+ * @name Inconsistent equality and hashing
+ * @description Defining a hash operation without defining equality may be a mistake.
+ * @kind problem
+ * @tags quality
+ *       reliability
+ *       correctness
+ *       external/cwe/cwe-581
+ * @problem.severity warning
+ * @sub-severity high
+ * @precision very-high
+ * @id py/equals-hash-mismatch
+ */
+
+import python
+
+predicate missingEquality(Class cls, Function defined) {
+  defined = cls.getMethod("__hash__") and
+  not exists(cls.getMethod("__eq__"))
+  // In python 3, the case of defined eq without hash automatically makes the class unhashable (even if a superclass defined hash)
+  // So this is not an issue.
+}
+
+from Class cls, Function defined
+where missingEquality(cls, defined)
+select cls, "This class implements $@, but does not implement __eq__.", defined, defined.getName()

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

@@ -0,0 +1,53 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>In order to ensure the <code>==</code> and <code>!=</code> operators behave consistently as expected (i.e. they should be negations of each other), care should be taken when implementing the 
+<code>__eq__</code> and <code>__ne__</code> special methods.</p>
+
+<p>In Python 3, if the <code>__eq__</code> method is defined in a class while the <code>__ne__</code> is not, 
+then the <code>!=</code> operator will automatically delegate to the <code>__eq__</code> method in the expected way. 
+ </p>
+
+ <p>However, if the <code>__ne__</code> method is defined without a corresponding <code>__eq__</code> method, 
+ the <code>==</code> operator will still default to object identity (equivalent to the <code>is</code> operator), while the <code>!=</code>
+ operator will use the <code>__ne__</code> method, which may be inconsistent.
+ </p>
+
+<p>Additionally, if the <code>__ne__</code> method is defined on a superclass, and the subclass defines its own <code>__eq__</code> method without overriding
+the superclass <code>__ne__</code> method, the <code>!=</code> operator will use this superclass <code>__ne__</code> method, rather than automatically delegating
+to <code>__eq__</code>, which may be incorrect.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that when an <code>__ne__</code> method is defined, the <code>__eq__</code> method is also defined, and their results are consistent.
+In most cases, the <code>__ne__</code> method does not need to be defined at all, as the default behavior is to delegate to <code>__eq__</code> and negate the result. </p>
+
+</recommendation>
+<example>
+<p>In the following example, <code>A</code> defines a <code>__ne__</code> method, but not an <code>__eq__</code> method.
+This leads to inconsistent results between equality and inequality operators. 
+</p>
+
+<sample src="examples/EqualsOrNotEquals1.py" />
+
+<p>In the following example, <code>C</code> defines an <code>__eq__</code> method, but its <code>__ne__</code> implementation is inherited from <code>B</code>, 
+which is not consistent with the equality operation.
+</p>
+
+<sample src="examples/EqualsOrNotEquals2.py" />
+
+</example>
+<references>
+
+
+<li>Python Language Reference: <a href="http://docs.python.org/3/reference/datamodel.html#object.__ne__">object.__ne__</a>, 
+<a href="http://docs.python.org/3/reference/expressions.html#comparisons">Comparisons</a>.</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,37 @@
+/**
+ * @name Inconsistent equality and inequality
+ * @description Class definitions of equality and inequality operators may be inconsistent.
+ * @kind problem
+ * @tags quality
+ *       reliability
+ *       correctness
+ * @problem.severity warning
+ * @sub-severity high
+ * @precision very-high
+ * @id py/inconsistent-equality
+ */
+
+import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch
+import Classes.Equality
+
+predicate missingEquality(Class cls, Function defined, string missing) {
+  defined = cls.getMethod("__ne__") and
+  not exists(cls.getMethod("__eq__")) and
+  missing = "__eq__"
+  or
+  // In python 3, __ne__ automatically delegates to __eq__ if its not defined in the hierarchy
+  // However if it is defined in a superclass (and isn't a delegation method) then it will use the superclass method (which may be incorrect)
+  defined = cls.getMethod("__eq__") and
+  not exists(cls.getMethod("__ne__")) and
+  exists(Function neMeth |
+    neMeth = getADirectSuperclass+(cls).getMethod("__ne__") and
+    not neMeth instanceof DelegatingEqualityMethod
+  ) and
+  missing = "__ne__"
+}
+
+from Class cls, Function defined, string missing
+where missingEquality(cls, defined, missing)
+select cls, "This class implements $@, but does not implement " + missing + ".", defined,
+  defined.getName()

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

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<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 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>
+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 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/3/reference/datamodel.html#object.__lt__">Rich comparisons in Python</a>.</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,55 @@
+/**
+ * @name Incomplete ordering
+ * @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
+ *       correctness
+ * @problem.severity warning
+ * @sub-severity low
+ * @precision very-high
+ * @id py/incomplete-ordering
+ */
+
+import python
+import semmle.python.dataflow.new.internal.DataFlowDispatch
+import semmle.python.ApiGraphs
+
+/** Holds if `cls` has the `functools.total_ordering` decorator. */
+predicate totalOrdering(Class cls) {
+  API::moduleImport("functools")
+      .getMember("total_ordering")
+      .asSource()
+      .flowsTo(DataFlow::exprNode(cls.getADecorator()))
+}
+
+predicate definesStrictOrdering(Class cls, Function meth) {
+  meth = cls.getMethod("__lt__")
+  or
+  not exists(cls.getMethod("__lt__")) and
+  meth = cls.getMethod("__gt__")
+}
+
+predicate definesNonStrictOrdering(Class cls, Function meth) {
+  meth = cls.getMethod("__le__")
+  or
+  not exists(cls.getMethod("__le__")) and
+  meth = cls.getMethod("__ge__")
+}
+
+predicate missingComparison(Class cls, Function defined, string missing) {
+  definesStrictOrdering(cls, defined) and
+  not definesNonStrictOrdering(getADirectSuperclass*(cls), _) and
+  missing = "__le__ or __ge__"
+  or
+  definesNonStrictOrdering(cls, defined) and
+  not definesStrictOrdering(getADirectSuperclass*(cls), _) and
+  missing = "__lt__ or __gt__"
+}
+
+from Class cls, Function defined, string missing
+where
+  not totalOrdering(cls) and
+  missingComparison(cls, defined, missing)
+select cls, "This class implements $@, but does not implement " + missing + ".", defined,
+  defined.getName()

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

@@ -0,0 +1,8 @@
+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/EqualsOrNotEquals1.py

@@ -0,0 +1,15 @@
+class A:
+    def __init__(self, a):
+        self.a = a 
+
+    # BAD: ne is defined, but not eq.
+    def __ne__(self, other):
+        if not isinstance(other, A):
+            return NotImplemented 
+        return self.a != other.a
+
+x = A(1)
+y = A(1)
+
+print(x == y) # Prints False (potentially unexpected - object identity is used)
+print(x != y) # Prints False

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

@@ -0,0 +1,21 @@
+class B:
+    def __init__(self, b):
+        self.b = b 
+    
+    def __eq__(self, other):
+        return self.b == other.b 
+    
+    def __ne__(self, other):
+        return self.b != other.b 
+    
+class C(B):
+    def __init__(self, b, c):
+        super().__init__(b)
+        self.c = c 
+
+    # BAD: eq is defined, but != will use superclass ne method, which is not consistent
+    def __eq__(self, other):
+        return self.b == other.b and self.c == other.c 
+    
+print(C(1,2) == C(1,3)) # Prints False 
+print(C(1,2) != C(1,3)) # Prints False (potentially unexpected)

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

@@ -0,0 +1,8 @@
+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
+    

python/ql/src/Classes/Equality.qll

@@ -1,4 +1,7 @@
+/** Utility definitions for reasoning about equality methods. */
+
 import python
+import semmle.python.dataflow.new.DataFlow
 
 private Attribute dictAccess(LocalVariable var) {
   result.getName() = "__dict__" and
@@ -59,16 +62,28 @@ class IdentityEqMethod extends Function {
 /** An (in)equality method that delegates to its complement */
 class DelegatingEqualityMethod extends Function {
   DelegatingEqualityMethod() {
-    exists(Return ret, UnaryExpr not_, Compare comp, Cmpop op, Parameter p0, Parameter p1 |
+    exists(Return ret, UnaryExpr not_, Expr comp, Parameter p0, Parameter p1 |
       ret.getScope() = this and
       ret.getValue() = not_ and
       not_.getOp() instanceof Not and
-      not_.getOperand() = comp and
-      comp.compares(p0.getVariable().getAnAccess(), op, p1.getVariable().getAnAccess())
+      not_.getOperand() = comp
     |
-      this.getName() = "__eq__" and op instanceof NotEq
+      exists(Cmpop op |
+        comp.(Compare).compares(p0.getVariable().getAnAccess(), op, p1.getVariable().getAnAccess())
+      |
+        this.getName() = "__eq__" and op instanceof NotEq
+        or
+        this.getName() = "__ne__" and op instanceof Eq
+      )
       or
-      this.getName() = "__ne__" and op instanceof Eq
+      exists(DataFlow::MethodCallNode call, string name |
+        call.calls(DataFlow::exprNode(p0.getVariable().getAnAccess()), name) and
+        call.getArg(0).asExpr() = p1.getVariable().getAnAccess()
+      |
+        this.getName() = "__eq__" and name = "__ne__"
+        or
+        this.getName() = "__ne__" and name = "__eq__"
+      )
     )
   }
 }

python/ql/src/Classes/EqualsOrHash.py

@@ -1,52 +0,0 @@
-# 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
-

python/ql/src/Classes/EqualsOrHash.qhelp

@@ -1,46 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<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 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>
-
-</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>
-
-</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>
-<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" />
-
-</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>
-
-
-</references>
-</qhelp>

python/ql/src/Classes/EqualsOrHash.ql

@@ -1,63 +0,0 @@
-/**
- * @name Inconsistent equality and hashing
- * @description Defining equality for a class without also defining hashability (or vice-versa) violates the object model.
- * @kind problem
- * @tags quality
- *       reliability
- *       correctness
- *       external/cwe/cwe-581
- * @problem.severity warning
- * @sub-severity high
- * @precision very-high
- * @id py/equals-hash-mismatch
- */
-
-import python
-
-CallableValue defines_equality(ClassValue c, string name) {
-  (
-    name = "__eq__"
-    or
-    major_version() = 2 and name = "__cmp__"
-  ) and
-  result = c.declaredAttribute(name)
-}
-
-CallableValue implemented_method(ClassValue c, string name) {
-  result = defines_equality(c, name)
-  or
-  result = c.declaredAttribute("__hash__") and name = "__hash__"
-}
-
-string unimplemented_method(ClassValue c) {
-  not exists(defines_equality(c, _)) and
-  (
-    result = "__eq__" and major_version() = 3
-    or
-    major_version() = 2 and result = "__eq__ or __cmp__"
-  )
-  or
-  /* Python 3 automatically makes classes unhashable if __eq__ is defined, but __hash__ is not */
-  not c.declaresAttribute(result) and result = "__hash__" and major_version() = 2
-}
-
-/** Holds if this class is unhashable */
-predicate unhashable(ClassValue cls) {
-  cls.lookup("__hash__") = Value::named("None")
-  or
-  cls.lookup("__hash__").(CallableValue).neverReturns()
-}
-
-predicate violates_hash_contract(ClassValue c, string present, string missing, Value method) {
-  not unhashable(c) and
-  missing = unimplemented_method(c) and
-  method = implemented_method(c, present) and
-  not c.failedInference(_)
-}
-
-from ClassValue c, string present, string missing, CallableValue method
-where
-  violates_hash_contract(c, present, missing, method) and
-  exists(c.getScope()) // Suppress results that aren't from source
-select method, "Class $@ implements " + present + " but does not define " + missing + ".", c,
-  c.getName()

python/ql/src/Classes/EqualsOrNotEquals.py

@@ -1,32 +0,0 @@
-class PointOriginal(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):  # Incorrect: equality is defined but inequality is not
-        if not isinstance(other, Point):
-            return False
-        return self._x == other._x and self._y == other._y
-
-
-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 __ne__(self, other):  # Improved: equality and inequality method defined (hash method still missing)
-        return not self == other
-

python/ql/src/Classes/EqualsOrNotEquals.qhelp

@@ -1,37 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "qhelp.dtd">
-<qhelp>
-
-<overview>
-<p>In order to conform to the object model, classes should define either no equality methods, or both 
-an equality and an inequality method. If only one of <code>__eq__</code> or <code>__ne__</code> is 
-defined then the method from the super class is used. This is unlikely to result in the expected 
-behavior.</p>
-
-</overview>
-<recommendation>
-
-<p>When you define an equality or an inequality method for a class, remember to implement both an 
-<code>__eq__</code> method and an <code>__ne__</code> method.</p>
-
-</recommendation>
-<example>
-<p>In the following example the <code>PointOriginal</code> class defines an equality method but 
-no inequality method. If this class is tested for inequality then a type error will be raised. The 
-<code>PointUpdated</code> class is better as it defines both an equality and an inequality method. To
-comply fully with the object model this class should also define a hash method (identified by
-a separate rule).</p>
-
-<sample src="EqualsOrNotEquals.py" />
-
-</example>
-<references>
-
-
-<li>Python Language Reference: <a href="http://docs.python.org/2/reference/datamodel.html#object.__ne__">object.__ne__</a>, 
-<a href="http://docs.python.org/2/reference/expressions.html#comparisons">Comparisons</a>.</li>
-
-
-</references>
-</qhelp>

python/ql/src/Classes/EqualsOrNotEquals.ql

@@ -1,48 +0,0 @@
-/**
- * @name Inconsistent equality and inequality
- * @description Defining only an equality method or an inequality method for a class violates the object model.
- * @kind problem
- * @tags reliability
- *       correctness
- * @problem.severity warning
- * @sub-severity high
- * @precision very-high
- * @id py/inconsistent-equality
- */
-
-import python
-import Equality
-
-string equals_or_ne() { result = "__eq__" or result = "__ne__" }
-
-predicate total_ordering(Class cls) {
-  exists(Attribute a | a = cls.getADecorator() | a.getName() = "total_ordering")
-  or
-  exists(Name n | n = cls.getADecorator() | n.getId() = "total_ordering")
-}
-
-CallableValue implemented_method(ClassValue c, string name) {
-  result = c.declaredAttribute(name) and name = equals_or_ne()
-}
-
-string unimplemented_method(ClassValue c) {
-  not c.declaresAttribute(result) and result = equals_or_ne()
-}
-
-predicate violates_equality_contract(
-  ClassValue c, string present, string missing, CallableValue method
-) {
-  missing = unimplemented_method(c) and
-  method = implemented_method(c, present) and
-  not c.failedInference(_) and
-  not total_ordering(c.getScope()) and
-  /* Python 3 automatically implements __ne__ if __eq__ is defined, but not vice-versa */
-  not (major_version() = 3 and present = "__eq__" and missing = "__ne__") and
-  not method.getScope() instanceof DelegatingEqualityMethod and
-  not c.lookup(missing).(CallableValue).getScope() instanceof DelegatingEqualityMethod
-}
-
-from ClassValue c, string present, string missing, CallableValue method
-where violates_equality_contract(c, present, missing, method)
-select method, "Class $@ implements " + present + " but does not implement " + missing + ".", c,
-  c.getName()

python/ql/src/Classes/IncompleteOrdering.py

@@ -1,6 +0,0 @@
-class IncompleteOrdering(object):
-    def __init__(self, i):
-        self.i = i
-
-    def __lt__(self, other):
-        return self.i < other.i

python/ql/src/Classes/IncompleteOrdering.qhelp

@@ -1,35 +0,0 @@
-<!DOCTYPE qhelp PUBLIC
-  "-//Semmle//qhelp//EN"
-  "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>
-
-</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>It is not necessary to manually implement all four comparisons,
-the <code>functools.total_ordering</code> class decorator can be used.</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" />
-
-</example>
-<references>
-
-  <li>Python Language Reference: <a href="http://docs.python.org/2/reference/datamodel.html#object.__lt__">Rich comparisons in Python</a>.</li>
-
-
-</references>
-</qhelp>

python/ql/src/Classes/IncompleteOrdering.ql

@@ -1,73 +0,0 @@
-/**
- * @name Incomplete ordering
- * @description Class defines one or more ordering method but does not define all 4 ordering comparison methods
- * @kind problem
- * @tags reliability
- *       correctness
- * @problem.severity warning
- * @sub-severity low
- * @precision very-high
- * @id py/incomplete-ordering
- */
-
-import python
-
-predicate total_ordering(Class cls) {
-  exists(Attribute a | a = cls.getADecorator() | a.getName() = "total_ordering")
-  or
-  exists(Name n | n = cls.getADecorator() | n.getId() = "total_ordering")
-}
-
-string ordering_name(int n) {
-  result = "__lt__" and n = 1
-  or
-  result = "__le__" and n = 2
-  or
-  result = "__gt__" and n = 3
-  or
-  result = "__ge__" and n = 4
-}
-
-predicate overrides_ordering_method(ClassValue c, string name) {
-  name = ordering_name(_) and
-  (
-    c.declaresAttribute(name)
-    or
-    exists(ClassValue sup | sup = c.getASuperType() and not sup = Value::named("object") |
-      sup.declaresAttribute(name)
-    )
-  )
-}
-
-string unimplemented_ordering(ClassValue c, int n) {
-  not c = Value::named("object") and
-  not overrides_ordering_method(c, result) and
-  result = ordering_name(n)
-}
-
-string unimplemented_ordering_methods(ClassValue c, int n) {
-  n = 0 and result = "" and exists(unimplemented_ordering(c, _))
-  or
-  exists(string prefix, int nm1 | n = nm1 + 1 and prefix = unimplemented_ordering_methods(c, nm1) |
-    prefix = "" and result = unimplemented_ordering(c, n)
-    or
-    result = prefix and not exists(unimplemented_ordering(c, n)) and n < 5
-    or
-    prefix != "" and result = prefix + " or " + unimplemented_ordering(c, n)
-  )
-}
-
-Value ordering_method(ClassValue c, string name) {
-  /* If class doesn't declare a method then don't blame this class (the superclass will be blamed). */
-  name = ordering_name(_) and result = c.declaredAttribute(name)
-}
-
-from ClassValue c, Value ordering, string name
-where
-  not c.failedInference(_) and
-  not total_ordering(c.getScope()) and
-  ordering = ordering_method(c, name) and
-  exists(unimplemented_ordering(c, _))
-select c,
-  "Class " + c.getName() + " implements $@, but does not implement " +
-    unimplemented_ordering_methods(c, 4) + ".", ordering, name

python/ql/src/change-notes/2025-07-14-comparisons.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The queries `py/incomplete-ordering`, `py/inconsistent-equality`, and `py/equals-hash-mismatch` have been modernized; no longer relying on outdated libraries, improved documentation, and no longer producing alerts for problems specific to Python 2.

python/ql/test/2/query-tests/Classes/equals-hash/EqualsOrHash.expected

@@ -1,2 +0,0 @@
-| equals_hash.py:8:5:8:28 | Function Eq.__eq__ | Class $@ implements __eq__ but does not define __hash__. | equals_hash.py:3:1:3:17 | class Eq | Eq |
-| equals_hash.py:24:5:24:23 | Function Hash.__hash__ | Class $@ implements __hash__ but does not define __eq__ or __cmp__. | equals_hash.py:19:1:19:19 | class Hash | Hash |

python/ql/test/2/query-tests/Classes/equals-hash/EqualsOrHash.qlref

@@ -1 +0,0 @@
-Classes/EqualsOrHash.ql

python/ql/test/2/query-tests/Classes/equals-hash/EqualsOrNotEquals.expected

@@ -1,2 +0,0 @@
-| equals_hash.py:8:5:8:28 | Function Eq.__eq__ | Class $@ implements __eq__ but does not implement __ne__. | equals_hash.py:3:1:3:17 | class Eq | Eq |
-| equals_hash.py:16:5:16:28 | Function Ne.__ne__ | Class $@ implements __ne__ but does not implement __eq__. | equals_hash.py:11:1:11:17 | class Ne | Ne |

python/ql/test/2/query-tests/Classes/equals-hash/EqualsOrNotEquals.qlref

@@ -1 +0,0 @@
-Classes/EqualsOrNotEquals.ql

python/ql/test/2/query-tests/Classes/equals-hash/equals_hash.py

@@ -1,63 +0,0 @@
-#Equals and hash
-
-class Eq(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __eq__(self, other):
-        return self.data == other.data
-
-class Ne(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __ne__(self, other):
-        return self.data != other.data
-
-class Hash(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __hash__(self):
-        return hash(self.data)
-    
-class Unhashable1(object):
-    
-    __hash__ = None
-    
-
-class EqOK1(Unhashable1):
-    
-    def __eq__(self, other):
-        return False
-    
-    def __ne__(self, other):
-        return True
-    
-class Unhashable2(object):
-    
-    #Not the idiomatic way of doing it, but not uncommon either
-    def __hash__(self):
-        raise TypeError("unhashable object")
-
-
-class EqOK2(Unhashable2):
-
-    def __eq__(self, other):
-        return False
-
-    def __ne__(self, other):
-        return True
-
-class ReflectiveNotEquals(object):
-
-    def __ne__(self, other):
-        return not self == other
-
-class EqOK3(ReflectiveNotEquals, Unhashable1):
-
-    def __eq__(self, other):
-        return self.data == other.data

python/ql/test/3/query-tests/Classes/equals-hash/EqualsOrHash.expected

@@ -1 +0,0 @@
-| equals_hash.py:24:5:24:23 | Function Hash.__hash__ | Class $@ implements __hash__ but does not define __eq__. | equals_hash.py:19:1:19:19 | class Hash | Hash |

python/ql/test/3/query-tests/Classes/equals-hash/EqualsOrHash.qlref

@@ -1 +0,0 @@
-Classes/EqualsOrHash.ql

python/ql/test/3/query-tests/Classes/equals-hash/equals_hash.py

@@ -1,63 +0,0 @@
-#Equals and hash
-
-class Eq(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __eq__(self, other):
-        return self.data == other.data
-
-class Ne(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __ne__(self, other):
-        return self.data != other.data
-
-class Hash(object):
-
-    def __init__(self, data):
-        self.data = data
-
-    def __hash__(self):
-        return hash(self.data)
-    
-class Unhashable1(object):
-    
-    __hash__ = None
-    
-
-class EqOK1(Unhashable1):
-    
-    def __eq__(self, other):
-        return False
-    
-    def __ne__(self, other):
-        return True
-    
-class Unhashable2(object):
-
-    #Not the idiomatic way of doing it, but not uncommon either
-    def __hash__(self):
-        raise TypeError("unhashable object")
-
-
-class EqOK2(Unhashable2):
-
-    def __eq__(self, other):
-        return False
-
-    def __ne__(self, other):
-        return True
-
-class ReflectiveNotEquals(object):
-
-    def __ne__(self, other):
-        return not self == other
-
-class EqOK3(ReflectiveNotEquals, Unhashable1):
-
-    def __eq__(self, other):
-        return self.data == other.data

python/ql/test/3/query-tests/Classes/equals-ne/EqualsOrNotEquals.expected

@@ -1 +0,0 @@
-| test.py:9:5:9:28 | Function NotOK2.__ne__ | Class $@ implements __ne__ but does not implement __eq__. | test.py:7:1:7:13 | class NotOK2 | NotOK2 |

python/ql/test/3/query-tests/Classes/equals-ne/EqualsOrNotEquals.qlref

@@ -1 +0,0 @@
-Classes/EqualsOrNotEquals.ql

python/ql/test/3/query-tests/Classes/equals-ne/test.py

@@ -1,10 +0,0 @@
-
-class OK:
-
-    def __eq__(self, other):
-        return False
-
-class NotOK2:
-
-    def __ne__(self, other):
-        return True

python/ql/test/query-tests/Classes/equals-attr/DefineEqualsWhenAddingAttributes.expected

@@ -0,0 +1 @@
+| attr_eq_test.py:21:1:21:27 | class BadColorPoint | The class 'BadColorPoint' does not override $@, but adds the new attribute $@. | attr_eq_test.py:10:5:10:28 | Function Point.__eq__ | '__eq__' | attr_eq_test.py:25:9:25:19 | Attribute | _color |

python/ql/test/query-tests/Classes/equals-hash/DefineEqualsWhenAddingFields.expected

@@ -1 +0,0 @@
-| attr_eq_test.py:21:1:21:27 | class BadColorPoint | The class 'BadColorPoint' does not override $@, but adds the new attribute $@. | attr_eq_test.py:10:5:10:28 | Function Point.__eq__ | '__eq__' | attr_eq_test.py:25:9:25:19 | Attribute | _color |

python/ql/test/query-tests/Classes/equals-hash/DefineEqualsWhenAddingFields.qlref

@@ -1 +0,0 @@
-Classes/DefineEqualsWhenAddingAttributes.ql

python/ql/test/query-tests/Classes/equals-hash/EqualsOrHash.expected

@@ -0,0 +1,2 @@
+| equalsHash.py:13:1:13:8 | Class C | This class implements $@, but does not implement __eq__. | equalsHash.py:14:5:14:23 | Function __hash__ | __hash__ |
+| equalsHash.py:17:1:17:11 | Class D | This class implements $@, but does not implement __eq__. | equalsHash.py:18:5:18:23 | Function __hash__ | __hash__ |

python/ql/test/query-tests/Classes/equals-hash/EqualsOrHash.qlref

@@ -0,0 +1,2 @@
+query: Classes/Comparisons/EqualsOrHash.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

python/ql/test/query-tests/Classes/equals-hash/equalsHash.py

@@ -0,0 +1,19 @@
+class A:
+    def __eq__(self, other):
+        return True 
+    
+    def __hash__(self):
+        return 7 
+    
+# B is automatically non-hashable - so eq without hash never needs to alert
+class B:
+    def __eq__(self, other):
+        return True 
+    
+class C: # $ Alert
+    def __hash__(self):
+        return 5
+    
+class D(A): # $ Alert 
+    def __hash__(self):
+        return 4

python/ql/test/query-tests/Classes/equals-not-equals/EqualsOrNotEquals.expected

@@ -0,0 +1,2 @@
+| EqualsOrNotEquals.py:14:1:14:8 | Class B | This class implements $@, but does not implement __eq__. | EqualsOrNotEquals.py:19:5:19:28 | Function __ne__ | __ne__ |
+| EqualsOrNotEquals.py:37:1:37:11 | Class D | This class implements $@, but does not implement __ne__. | EqualsOrNotEquals.py:43:5:43:28 | Function __eq__ | __eq__ |

python/ql/test/query-tests/Classes/equals-not-equals/EqualsOrNotEquals.py

@@ -0,0 +1,147 @@
+class A:
+    def __init__(self, a):
+        self.a = a
+
+    # OK: __ne__ if not defined delegates to eq automatically
+    def __eq__(self, other):
+        return self.a == other.a 
+
+assert (A(1) == A(1))
+assert not (A(1) == A(2))
+assert not (A(1) != A(1))
+assert (A(1) != A(2))
+
+class B: # $ Alert 
+    def __init__(self, b):
+        self.b = b
+
+    # BAD: eq defaults to `is`
+    def __ne__(self, other):
+        return self.b != other.b 
+
+assert not (B(1) == B(1)) # potentially unexpected
+assert not (B(2) == B(2))
+assert not (B(1) != B(1))
+assert (B(1) != B(2))
+
+class C:
+    def __init__(self, c):
+        self.c = c 
+
+    def __eq__(self, other):
+        return self.c == other.c 
+
+    def __ne__(self, other):
+        return self.c != other.c 
+
+class D(C): # $ Alert 
+    def __init__(self, c, d):
+        super().__init__(c)
+        self.d = d
+
+    # BAD: ne is not defined, but the superclass ne is used instead of delegating, which may be incorrect
+    def __eq__(self, other):
+        return self.c == other.c and self.d == other.d
+
+assert (D(1,2) == D(1,2))
+assert not (D(1,2) == D(1,3))
+assert (D(1,2) != D(3,2)) 
+assert not (D(1,2) != D(1,3)) # Potentially unexpected 
+
+class E:
+    def __init__(self, e):
+        self.e = e 
+
+    def __eq__(self, other):
+        return self.e == other.e 
+
+    def __ne__(self, other):
+        return not self.__eq__(other)
+
+class F(E): 
+    def __init__(self, e, f):
+        super().__init__(e)
+        self.f = f
+
+    # OK: superclass ne delegates to eq
+    def __eq__(self, other):
+        return self.e == other.e and self.f == other.f
+
+assert (F(1,2) == F(1,2))
+assert not (F(1,2) == F(1,3))
+assert (F(1,2) != F(3,2)) 
+assert (F(1,2) != F(1,3))
+
+# Variations 
+
+class E2:
+    def __init__(self, e):
+        self.e = e 
+
+    def __eq__(self, other):
+        return self.e == other.e 
+
+    def __ne__(self, other):
+        return not self == other
+
+class F2(E2): 
+    def __init__(self, e, f):
+        super().__init__(e)
+        self.f = f
+
+    # OK: superclass ne delegates to eq
+    def __eq__(self, other):
+        return self.e == other.e and self.f == other.f
+
+assert (F2(1,2) == F2(1,2))
+assert not (F2(1,2) == F2(1,3))
+assert (F2(1,2) != F2(3,2)) 
+assert (F2(1,2) != F2(1,3))
+
+class E3:
+    def __init__(self, e):
+        self.e = e 
+
+    def __eq__(self, other):
+        return self.e == other.e 
+
+    def __ne__(self, other):
+        return not other.__eq__(self)
+
+class F3(E3): 
+    def __init__(self, e, f):
+        super().__init__(e)
+        self.f = f
+
+    # OK: superclass ne delegates to eq
+    def __eq__(self, other):
+        return self.e == other.e and self.f == other.f
+
+assert (F3(1,2) == F3(1,2))
+assert not (F3(1,2) == F3(1,3))
+assert (F3(1,2) != F3(3,2)) 
+assert (F3(1,2) != F3(1,3))
+
+class E4:
+    def __init__(self, e):
+        self.e = e 
+
+    def __eq__(self, other):
+        return self.e == other.e 
+
+    def __ne__(self, other):
+        return not other == self
+
+class F4(E4): 
+    def __init__(self, e, f):
+        super().__init__(e)
+        self.f = f
+
+    # OK: superclass ne delegates to eq
+    def __eq__(self, other):
+        return self.e == other.e and self.f == other.f
+
+assert (F4(1,2) == F4(1,2))
+assert not (F4(1,2) == F4(1,3))
+assert (F4(1,2) != F4(3,2)) 
+assert (F4(1,2) != F4(1,3))

python/ql/test/query-tests/Classes/equals-not-equals/EqualsOrNotEquals.qlref

@@ -0,0 +1,2 @@
+query: Classes/Comparisons/EqualsOrNotEquals.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

python/ql/test/query-tests/Classes/incomplete-ordering/IncompleteOrdering.expected

@@ -1 +1,2 @@
-| incomplete_ordering.py:3:1:3:26 | class PartOrdered | Class PartOrdered implements $@, but does not implement __le__ or __gt__ or __ge__. | incomplete_ordering.py:13:5:13:28 | Function PartOrdered.__lt__ | __lt__ |
+| incomplete_ordering.py:3:1:3:26 | Class LtWithoutLe | This class implements $@, but does not implement __le__ or __ge__. | incomplete_ordering.py:13:5:13:28 | Function __lt__ | __lt__ |
+| incomplete_ordering.py:28:1:28:17 | Class LendGeNoLt | This class implements $@, but does not implement __lt__ or __gt__. | incomplete_ordering.py:29:5:29:28 | Function __le__ | __le__ |

python/ql/test/query-tests/Classes/incomplete-ordering/IncompleteOrdering.qlref

@@ -1 +1,2 @@
-Classes/IncompleteOrdering.ql
+query: Classes/Comparisons/IncompleteOrdering.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

python/ql/test/query-tests/Classes/incomplete-ordering/incomplete_ordering.py

@@ -1,6 +1,6 @@
 #Incomplete ordering
 
-class PartOrdered(object):
+class LtWithoutLe(object): # $ Alert
     def __eq__(self, other):
         return self is other
 
@@ -13,6 +13,28 @@ class PartOrdered(object):
     def __lt__(self, other):
         return False
 
-#Don't blame a sub-class for super-class's sins.
-class DerivedPartOrdered(PartOrdered):
-    pass
+# Don't alert on subclass
+class LtWithoutLeSub(LtWithoutLe):
+    pass
+
+class LeSub(LtWithoutLe):
+    def __le__(self, other):
+        return self < other or self == other 
+    
+class GeSub(LtWithoutLe):
+    def __ge__(self, other):
+        return self > other or self == other 
+    
+class LendGeNoLt: # $ Alert
+    def __le__(self, other):
+        return True 
+    
+    def __ge__(self, other):
+        return other <= self 
+    
+from functools import total_ordering
+
+@total_ordering
+class Total:
+    def __le__(self, other):
+        return True