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/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,51 @@
+<!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>Additionally, if the <code>__ne__</code> method is defined on a superclass, and the subclass defines its own <cde>__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.
+
+</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__"
+      )
     )
   }
 }