Merge pull request github#3658 from RasmusWL/python-3.8-dict-ismapping

Approved by tausbn

Author: semmle-qlci

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/2/library-tests/PointsTo/class_properties/ClassValues.expected

@@ -0,0 +1,12 @@
+| 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 list |
+| sequence | builtin-class str |
+| sequence | builtin-class tuple |
+| sequence | builtin-class unicode |
+| sequence | class MySequenceABC |
+| sequence | class MySequenceImpl |

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

@@ -0,0 +1,20 @@
+import python
+
+from ClassValue cls, string res
+where
+    exists(CallNode call |
+        call.getFunction().(NameNode).getId() = "test" and
+        call.getAnArg().pointsTo(cls)
+    ) and
+    (
+        cls.isSequence() and
+        cls.isMapping() and
+        res = "IS BOTH. SHOULD NOT HAPPEN. THEY ARE MUTUALLY EXCLUSIVE."
+        or
+        cls.isSequence() and not cls.isMapping() and res = "sequence"
+        or
+        not cls.isSequence() and cls.isMapping() and res = "mapping"
+        or
+        not cls.isSequence() and not cls.isMapping() and res = "neither sequence nor mapping"
+    )
+select res, cls.toString()

python/ql/test/2/library-tests/PointsTo/class_properties/options

@@ -0,0 +1 @@
+semmle-extractor-options: --lang=2 --max-import-depth=2

python/ql/test/2/library-tests/PointsTo/class_properties/test.py

@@ -0,0 +1,50 @@
+from collections import OrderedDict, defaultdict
+
+# Python 2 specific
+from collections import Sequence, Mapping
+
+def test(*args):
+    pass
+
+class MySequenceABC(Sequence):
+    pass
+
+class MyMappingABC(Mapping):
+    pass
+
+class MySequenceImpl(object):
+    def __getitem__(self, key):
+        pass
+
+    def __len__(self):
+        pass
+
+class MyDictSubclass(dict):
+    pass
+
+test(
+    list,
+    tuple,
+    str,
+    unicode,
+    bytes,
+    MySequenceABC,
+    MySequenceImpl,
+    set,
+    dict,
+    OrderedDict,
+    defaultdict,
+    MyMappingABC,
+    MyDictSubclass,
+)
+
+for seq_cls in (list, tuple, str, bytes):
+    assert issubclass(seq_cls, collections.abc.Sequence)
+    assert not issubclass(seq_cls, collections.abc.Mapping)
+
+for map_cls in (dict, OrderedDict, defaultdict):
+    assert not issubclass(map_cls, collections.abc.Sequence)
+    assert issubclass(map_cls, collections.abc.Mapping)
+
+assert not issubclass(set, collections.abc.Sequence)
+assert not issubclass(set, collections.abc.Mapping)

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

@@ -0,0 +1,13 @@
+| 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 list |
+| sequence | builtin-class str |
+| sequence | builtin-class tuple |
+| sequence | class MySequenceABC |
+| sequence | class MySequenceImpl |

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

@@ -0,0 +1,20 @@
+import python
+
+from ClassValue cls, string res
+where
+    exists(CallNode call |
+        call.getFunction().(NameNode).getId() = "test" and
+        call.getAnArg().pointsTo(cls)
+    ) and
+    (
+        cls.isSequence() and
+        cls.isMapping() and
+        res = "IS BOTH. SHOULD NOT HAPPEN. THEY ARE MUTUALLY EXCLUSIVE."
+        or
+        cls.isSequence() and not cls.isMapping() and res = "sequence"
+        or
+        not cls.isSequence() and cls.isMapping() and res = "mapping"
+        or
+        not cls.isSequence() and not cls.isMapping() and res = "neither sequence nor mapping"
+    )
+select res, cls.toString()

python/ql/test/3/library-tests/PointsTo/class_properties/options

@@ -0,0 +1 @@
+semmle-extractor-options: --max-import-depth=2

python/ql/test/3/library-tests/PointsTo/class_properties/test.py

@@ -0,0 +1,50 @@
+from collections import OrderedDict, defaultdict
+
+# Python 3 specific
+from collections.abc import Sequence, Mapping
+
+def test(*args):
+    pass
+
+class MySequenceABC(Sequence):
+    pass
+
+class MyMappingABC(Mapping):
+    pass
+
+class MySequenceImpl(object):
+    def __getitem__(self, key):
+        pass
+
+    def __len__(self):
+        pass
+
+class MyDictSubclass(dict):
+    pass
+
+test(
+    list,
+    tuple,
+    str,
+    unicode,
+    bytes,
+    MySequenceABC,
+    MySequenceImpl,
+    set,
+    dict,
+    OrderedDict,
+    defaultdict,
+    MyMappingABC,
+    MyDictSubclass,
+)
+
+for seq_cls in (list, tuple, str, bytes):
+    assert issubclass(seq_cls, collections.abc.Sequence)
+    assert not issubclass(seq_cls, collections.abc.Mapping)
+
+for map_cls in (dict, OrderedDict, defaultdict):
+    assert not issubclass(map_cls, collections.abc.Sequence)
+    assert issubclass(map_cls, collections.abc.Mapping)
+
+assert not issubclass(set, collections.abc.Sequence)
+assert not issubclass(set, collections.abc.Mapping)