Python: Fix isSequence() and isMapping()

Author: RasmusWL

python/ql/src/semmle/python/objects/ObjectAPI.qll

@@ -517,7 +517,14 @@ class ClassValue extends Value {
     /** Holds if this class is a container(). That is, does it have a __getitem__ method. */
     predicate isContainer() { exists(this.lookup("__getitem__")) }
 
-    /** Holds if this class is probably a sequence. */
+    /**
+     * Holds if this class is a sequence. Mutually exclusive with `isMapping()`.
+     *
+     * Following the definition from
+     * https://docs.python.org/3/glossary.html#term-sequence.
+     * We don't look at the keys accepted by `__getitem__, but default to treating a class
+     * as a sequence (so might treat some mappings as sequences).
+     */
     predicate isSequence() {
         /*
          * To determine whether something is a sequence or a mapping is not entirely clear,
@@ -538,16 +545,26 @@ class ClassValue extends Value {
         or
         major_version() = 3 and this.getASuperType() = Value::named("collections.abc.Sequence")
         or
-        /* Does it have an index or __reversed__ method? */
-        this.isContainer() and
-        (
-            this.hasAttribute("index") or
-            this.hasAttribute("__reversed__")
-        )
+        this.hasAttribute("__getitem__") and
+        this.hasAttribute("__len__") and
+        not this.getASuperType() = ClassValue::dict() and
+        not this.getASuperType() = Value::named("collections.Mapping") and
+        not this.getASuperType() = Value::named("collections.abc.Mapping")
     }
 
-    /** Holds if this class is a mapping. */
+    /**
+     * Holds if this class is a mapping. Mutually exclusive with `isSequence()`.
+     *
+     * Although a class will satisfy the requirement by the definition in
+     * https://docs.python.org/3.8/glossary.html#term-mapping, we don't look at the keys
+     * accepted by `__getitem__, but default to treating a class as a sequence (so might
+     * treat some mappings as sequences).
+     */
     predicate isMapping() {
+        major_version() = 2 and this.getASuperType() = Value::named("collections.Mapping")
+        or
+        major_version() = 3 and this.getASuperType() = Value::named("collections.abc.Mapping")
+        or
         this.hasAttribute("__getitem__") and
         not this.isSequence()
     }

python/ql/test/library-tests/PointsTo/class_properties/ClassValues.expected

@@ -1,13 +1,13 @@
-| mapping | class MySequenceImpl |
+| mapping | builtin-class collections.OrderedDict |
+| mapping | builtin-class collections.defaultdict |
+| mapping | builtin-class dict |
+| mapping | class MyDictSubclass |
+| mapping | class MyMappingABC |
+| mapping | class OrderedDict |
 | neither sequence nor mapping | builtin-class set |
 | sequence | builtin-class bytes |
-| sequence | builtin-class collections.OrderedDict |
-| sequence | builtin-class collections.defaultdict |
-| sequence | builtin-class dict |
 | sequence | builtin-class list |
 | sequence | builtin-class str |
 | sequence | builtin-class tuple |
-| sequence | class MyDictSubclass |
-| sequence | class MyMappingABC |
 | sequence | class MySequenceABC |
-| sequence | class OrderedDict |
+| sequence | class MySequenceImpl |