"string" in F("...") hangs

[Tim Graham]

Prior to 94b6f101f7dc363a8e71593570b17527dbb9f77f, this test passes:

tests/expressions/tests.py

@@ -1292 +1292 @@
         self.assertNotEqual(f, value)
         self.assertNotEqual(value, f)
 
+    def test_in(self):
+        with self.assertRaisesMessage(TypeError, "argument of type 'F' is not iterable"):
+            "foo" in F("name")
+
 
 class ExpressionsTests(TestCase):
     def test_F_reuse(self):

Afterward, it hangs indefinitely.

[Simon Charette]

The issue lies in implementing __getitem__ without implementing __contains__, per ​the Python docs

For objects that don’t define __contains__(), the membership test first tries iteration via __iter__(), then the old sequence iteration protocol via __getitem__(), see ​this section in the language reference.

Basically what "foo" in F("name") translates to is the following old-style iteration protocol

Lastly, the old-style iteration protocol is tried: if a class defines __getitem__(), x in y is True if and only if there is a non-negative integer index i such that x is y[i] or x == y[i], and no lower integer index raises the IndexError exception.

def __contains__(self, value):
    index = 0
    while True:
        try:
            item = ref.__getitem__(index)
        except IndexError:
            return False
        if item is value or item == value:
            return True
        index += 1

which explains why it hangs indefinitely.

[Tim Graham]

Simon, do you think it's better to return False than the old behavior of raising an exception saying that F isn't iterable? The code where I encountered this is:

if LOOKUP_SEP in self.query.order_by:
    raise NotSupportedError("Ordering can't span tables.")

This naive implementation doesn't account for F objects in ordering. If we change the behavior to return False, my code will crash a bit later. That doesn't matter since this problem needs to be fixed eventually, but I tend to think continuing to raise an exception in F.__contains__() would be fine since it doesn't have any meaning.

[Simon Charette]

Tim, given we don't actually support it I think raising a TypeError might effectively be more appropriate. I tried skimming the Python docs to understand if doing so would be against any established protocol and I couldn't find anything we'd be in violation of.

To make sure we're aligned, you were thinking of something like

django/db/models/expressions.py

@@ -884 +884 @@
     def __getitem__(self, subscript):
         return Sliced(self, subscript)
 
+    def __contains__(self, other):
+        # Disable old-style iteration protocol inherited from implementing
+        # `__getitem__` for slicing to prevent containment checks from hanging.
+        raise TypeError(f"argument of type '{self.__class__.__name__}' is not iterable")
+
     def resolve_expression(
         self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False
     ):

[Tim Graham]

​PR. LGTM!

[Simon Charette]

Lufafa, we appreciate your interesting in contributing code to Django but please ask before jumping on assigning tickets to yourself when an active discussion is taking place.

[Sarah Boyce <42296566+sarahboyce@…>]

In 6b3f554:

Fixed #35603 -- Prevented F.contains() from hanging.

Regression in 94b6f101f7dc363a8e71593570b17527dbb9f77f.

[Sarah Boyce <42296566+sarahboyce@…>]

In 8fb7d304:

[5.1.x] Fixed #35603 -- Prevented F.contains() from hanging.

Regression in 94b6f101f7dc363a8e71593570b17527dbb9f77f.

Backport of 6b3f55446fdc62bd277903fd188a1781e4d92d29 from main.