Merge pull request #22965 from owen-mc/copilot/reintroduce-functionality

Python: reintroduce instance-attribute type-tracking without the classInstanceTracker blow-up (try 2)

Author: owen-mc

python/ql/lib/change-notes/2026-06-30-instance-attribute-typetracking-performance.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Type tracking now works for values stored in instance attributes and read from outside the class (for example `instance.attr` where the value was assigned to `self.attr` in a method), provided the instance at the read is reachable by local flow from a resolved constructor call. This excludes, for example, instances returned by a function and module-level instances read inside a function.

python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll

@@ -170,13 +170,7 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
 
   /** Holds if there is a level step from `nodeFrom` to `nodeTo`, which may depend on the call graph. */
   predicate levelStepCall(Node nodeFrom, LocalSourceNode nodeTo) {
-    // HOTFIX: `instanceFieldStep` is temporarily disabled (via `and none()`).
-    // It uses `classInstanceTracker(cls)` -- itself a type-tracker run --
-    // from inside `levelStepCall`, creating a structural mutual recursion
-    // that causes catastrophic query slowdowns on some OOP-heavy Python
-    // codebases (e.g. mypy and dask). The `and none()` should be removed
-    // once that recursion is redesigned.
-    instanceFieldStep(nodeFrom, nodeTo) and none()
+    instanceFieldStep(nodeFrom, nodeTo)
     or
     inheritedFieldStep(nodeFrom, nodeTo)
   }
@@ -355,11 +349,25 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
    * `instance.attr`, where `instance` is a reference to an instance of `cls`).
    *
    * This complements `selfAttrRef`, which only handles `self.attr` accesses inside the
-   * methods of `cls`. Unlike `selfAttrRef`, this depends on the call graph (via
-   * `classInstanceTracker`), so steps using it must be reported as `levelStepCall`.
+   * methods of `cls`. The instance is identified using *local* flow from a constructor
+   * call `cls(...)` (resolved via the call graph by `resolveClassCall`), rather than a
+   * dedicated instance type-tracker (`classInstanceTracker`).
+   *
+   * This removes the direct use of `classInstanceTracker` here, which previously caused
+   * catastrophic query slowdowns on some OOP-heavy Python code bases (e.g. `mypy` and
+   * `dask`). Indirect recursion remains: `resolveClassCall` uses `classTracker` and
+   * `clsArgumentTracker`, and `classTracker` can depend on `classInstanceTracker` through
+   * `type(obj)`. The performance impact on those code bases still needs validation.
+   *
+   * Only instances reachable by local flow from a resolved constructor call are covered
+   * by this step. This excludes, for example, instances returned by a function and
+   * module-level instances read inside a function.
    */
   private predicate instanceAttrRead(Class cls, string attr, DataFlowPublic::AttrRead read) {
-    read.getObject() = DataFlowDispatch::classInstanceTracker(cls) and
+    exists(DataFlowPublic::CallCfgNode construction |
+      DataFlowDispatch::resolveClassCall(construction.asCfgNode(), cls) and
+      read.getObject().getALocalSource() = construction
+    ) and
     read.mayHaveAttributeName(attr)
   }
 
@@ -438,9 +446,9 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
    * This is the cross-instance counterpart of `localFieldStep`: it relates a write of
    * `self.attr` inside a class to a read of `attr` on a reference to an instance of that
    * class or one of its subclasses. Identifying instances relies on the call graph (via
-   * `classInstanceTracker`), so this step is reported as `levelStepCall` rather than
-   * `levelStepNoCall`. The write may occur in the instance's own class or in any of its
-   * superclasses, since those methods are inherited.
+   * `resolveClassCall`, see `instanceAttrRead`), so this step is reported as
+   * `levelStepCall` rather than `levelStepNoCall`. The write may occur in the instance's
+   * own class or in any of its superclasses, since those methods are inherited.
    *
    * Like `localFieldStep`, this is an over-approximation: it is both instance-insensitive
    * and order-insensitive.

python/ql/test/library-tests/dataflow/typetracking/attribute_tests.py

@@ -157,10 +157,22 @@ def possibly_uncalled_method(self): # $ MISSING: tracked=foo
         print(self.foo) # $ tracked MISSING: tracked=foo
 
 instance = MyClass2()
-print(instance.foo) # $ MISSING: tracked=foo tracked
+print(instance.foo) # $ tracked MISSING: tracked=foo
 instance.print_foo() # $ MISSING: tracked=foo
 
 
+# attribute set in method, but the instance flows across a call/return before the read.
+# `instanceFieldStep` identifies the instance using only local flow from the constructor
+# call, so a value stored on `self.foo` is not seen once the instance has crossed a
+# function boundary.
+
+def make_my_class2():
+    return MyClass2()
+
+returned_instance = make_my_class2()
+print(returned_instance.foo) # $ MISSING: tracked
+
+
 # attribute set from outside of class
 
 class MyClass3(object):
@@ -195,7 +207,7 @@ def read_foo(self): # $ MISSING: tracked=foo
 
 sub1 = Sub1()
 sub1.read_foo()
-print(sub1.foo) # $ MISSING: tracked=foo tracked
+print(sub1.foo) # $ tracked MISSING: tracked=foo
 
 
 # attribute written in a subclass method, read in an inherited base class method
@@ -210,7 +222,7 @@ def __init__(self): # $ tracked=bar
 
 sub2 = Sub2()
 sub2.read_bar()
-print(sub2.bar) # $ MISSING: tracked=bar tracked
+print(sub2.bar) # $ tracked MISSING: tracked=bar
 
 
 # attribute written in a base class method, read on an instance of the subclass
@@ -223,4 +235,4 @@ class Sub3(Base3):
     pass
 
 sub3 = Sub3()
-print(sub3.baz) # $ MISSING: tracked=baz tracked
+print(sub3.baz) # $ tracked MISSING: tracked=baz

python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected

@@ -1,6 +1,7 @@
 #select
 | app.py:23:20:23:24 | ControlFlowNode for query | app.py:20:18:20:21 | ControlFlowNode for name | app.py:23:20:23:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:20:18:20:21 | ControlFlowNode for name | user-provided value |
 | app.py:30:20:30:24 | ControlFlowNode for query | app.py:27:19:27:22 | ControlFlowNode for name | app.py:30:20:30:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:27:19:27:22 | ControlFlowNode for name | user-provided value |
+| app.py:37:20:37:24 | ControlFlowNode for query | app.py:34:19:34:22 | ControlFlowNode for name | app.py:37:20:37:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:34:19:34:22 | ControlFlowNode for name | user-provided value |
 | app.py:44:20:44:24 | ControlFlowNode for query | app.py:41:19:41:22 | ControlFlowNode for name | app.py:44:20:44:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:41:19:41:22 | ControlFlowNode for name | user-provided value |
 | app.py:51:20:51:24 | ControlFlowNode for query | app.py:48:19:48:22 | ControlFlowNode for name | app.py:51:20:51:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:48:19:48:22 | ControlFlowNode for name | user-provided value |
 | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | This SQL query depends on a $@. | sql_injection.py:14:15:14:22 | ControlFlowNode for username | user-provided value |
@@ -24,6 +25,8 @@ edges
 | app.py:21:5:21:9 | ControlFlowNode for query | app.py:23:20:23:24 | ControlFlowNode for query | provenance |  |
 | app.py:27:19:27:22 | ControlFlowNode for name | app.py:28:5:28:9 | ControlFlowNode for query | provenance |  |
 | app.py:28:5:28:9 | ControlFlowNode for query | app.py:30:20:30:24 | ControlFlowNode for query | provenance |  |
+| app.py:34:19:34:22 | ControlFlowNode for name | app.py:35:5:35:9 | ControlFlowNode for query | provenance |  |
+| app.py:35:5:35:9 | ControlFlowNode for query | app.py:37:20:37:24 | ControlFlowNode for query | provenance |  |
 | app.py:41:19:41:22 | ControlFlowNode for name | app.py:42:5:42:9 | ControlFlowNode for query | provenance |  |
 | app.py:42:5:42:9 | ControlFlowNode for query | app.py:44:20:44:24 | ControlFlowNode for query | provenance |  |
 | app.py:48:19:48:22 | ControlFlowNode for name | app.py:49:5:49:9 | ControlFlowNode for query | provenance |  |
@@ -51,6 +54,9 @@ nodes
 | app.py:27:19:27:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | app.py:28:5:28:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
 | app.py:30:20:30:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
+| app.py:34:19:34:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
+| app.py:35:5:35:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
+| app.py:37:20:37:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
 | app.py:41:19:41:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
 | app.py:42:5:42:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
 | app.py:44:20:44:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |

python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py

@@ -31,10 +31,10 @@ async def unsafe2(name: str): # $ Source
     cursor.close()
 
 @app.get("/unsafe3/")
-async def unsafe3(name: str): # $ MISSING: Source
+async def unsafe3(name: str): # $ Source
     query = "select * from users where name=" + name
     cursor = hdb_con3.cursor()
-    cursor.execute(query) # $ MISSING: Alert
+    cursor.execute(query) # $ Alert
     cursor.close()
 
 @app.get("/unsafe4/")