Python: Exclude iterators guarded by isinstance checks

A common pattern is to check `isinstance(it, (list, tuple))` before
proceeding with the iteration.

python/ql/src/Statements/NonIteratorInForLoop.ql

@@ -12,16 +12,48 @@
  */
 
 import python
-private import LegacyPointsTo
+private import semmle.python.dataflow.new.internal.DataFlowDispatch
+private import semmle.python.ApiGraphs
 
-from For loop, ControlFlowNodeWithPointsTo iter, Value v, ClassValue t, ControlFlowNode origin
+/**
+ * Holds if `cls_arg` references a known iterable builtin type, either directly
+ * (e.g. `list`) or as an element of a tuple (e.g. `(list, tuple)`).
+ */
+private predicate isIterableTypeArg(DataFlow::Node cls_arg) {
+  cls_arg =
+    API::builtin([
+        "list", "tuple", "set", "frozenset", "dict", "str", "bytes", "bytearray", "range",
+        "memoryview"
+      ]).getAValueReachableFromSource()
+  or
+  isIterableTypeArg(DataFlow::exprNode(cls_arg.asExpr().(Tuple).getAnElt()))
+}
+
+/**
+ * Holds if `iter` is guarded by an `isinstance` check that tests for
+ * an iterable type (e.g. `list`, `tuple`, `set`, `dict`).
+ */
+predicate guardedByIsinstanceIterable(DataFlow::Node iter) {
+  exists(
+    DataFlow::GuardNode guard, DataFlow::CallCfgNode isinstance_call, DataFlow::LocalSourceNode src
+  |
+    isinstance_call = API::builtin("isinstance").getACall() and
+    src.flowsTo(isinstance_call.getArg(0)) and
+    src.flowsTo(iter) and
+    isIterableTypeArg(isinstance_call.getArg(1)) and
+    guard = isinstance_call.asCfgNode() and
+    guard.controlsBlock(iter.asCfgNode().getBasicBlock(), true)
+  )
+}
+
+from For loop, DataFlow::Node iter, Class cls
 where
-  loop.getIter().getAFlowNode() = iter and
-  iter.pointsTo(_, v, origin) and
-  v.getClass() = t and
-  not t.isIterable() and
-  not t.failedInference(_) and
-  not v = Value::named("None") and
-  not t.isDescriptorType()
-select loop, "This for-loop may attempt to iterate over a $@ of class $@.", origin,
-  "non-iterable instance", t, t.getName()
+  iter.asExpr() = loop.getIter() and
+  iter = classInstanceTracker(cls) and
+  not DuckTyping::isIterable(cls) and
+  not DuckTyping::isDescriptor(cls) and
+  not (loop.isAsync() and DuckTyping::hasMethod(cls, "__aiter__")) and
+  not DuckTyping::hasUnresolvedBase(getADirectSuperclass*(cls)) and
+  not guardedByIsinstanceIterable(iter)
+select loop, "This for-loop may attempt to iterate over a $@ of class $@.", iter.asExpr(),
+  "non-iterable instance", cls, cls.getName()

python/ql/test/3/query-tests/Statements/iter/NonIteratorInForLoop.expected

@@ -1,2 +1 @@
-| async_iterator.py:26:11:26:34 | For | This for-loop may attempt to iterate over a $@ of class $@. | async_iterator.py:26:20:26:33 | ControlFlowNode for MissingAiter() | non-iterable instance | async_iterator.py:13:1:13:19 | class MissingAiter | MissingAiter |
-| statements_test.py:34:5:34:19 | For | This for-loop may attempt to iterate over a $@ of class $@. | statements_test.py:34:18:34:18 | ControlFlowNode for IntegerLiteral | non-iterable instance | file://:0:0:0:0 | builtin-class int | int |
+| async_iterator.py:26:11:26:34 | For | This for-loop may attempt to iterate over a $@ of class $@. | async_iterator.py:26:20:26:33 | MissingAiter() | non-iterable instance | async_iterator.py:13:1:13:19 | Class MissingAiter | MissingAiter |

python/ql/test/query-tests/Statements/general/NonIteratorInForLoop.expected

@@ -1 +1 @@
-| test.py:50:1:50:23 | For | This for-loop may attempt to iterate over a $@ of class $@. | test.py:50:10:50:22 | ControlFlowNode for NonIterator() | non-iterable instance | test.py:45:1:45:26 | class NonIterator | NonIterator |
+| test.py:50:1:50:23 | For | This for-loop may attempt to iterate over a $@ of class $@. | test.py:50:10:50:22 | NonIterator() | non-iterable instance | test.py:45:1:45:26 | Class NonIterator | NonIterator |

python/ql/test/query-tests/Statements/general/test.py

@@ -174,3 +174,24 @@ def assert_ok(seq):
 # False positive. ODASA-8042. Fixed in PR #2401.
 class false_positive:
     e = (x for x in [])
+
+# isinstance guard should suppress non-iterable warning
+def guarded_iteration(x):
+    ni = NonIterator()
+    if isinstance(ni, (list, tuple)):
+        for item in ni:
+            pass
+
+def guarded_iteration_single(x):
+    ni = NonIterator()
+    if isinstance(ni, list):
+        for item in ni:
+            pass
+
+# Negated isinstance guard: early return when NOT iterable
+def guarded_iteration_negated(x):
+    ni = NonIterator()
+    if not isinstance(ni, list):
+        return
+    for item in ni:  # OK: guarded by negated isinstance + early return
+        pass