Avoid unsafe completion on array elements

thomasballinger · thomasballinger · commit 05c83beb32b1 · 2015-11-28T18:51:22.000-05:00
This commit removes functionality it would be nice to reimplement
but wasn't safe:
* completion on nested arrays ( list[1][2].a )
* completion on subclasses of list, tuple etc.
diff --git a/bpython/autocomplete.py b/bpython/autocomplete.py
@@ -118,8 +118,10 @@ def matches(self, cursor_offset, line, **kwargs):
         raise NotImplementedError
 
     def locate(self, cursor_offset, line):
-        """Returns a start, stop, and word given a line and cursor, or None
-        if no target for this type of completion is found under the cursor"""
+        """Returns a Linepart namedtuple instance or None given cursor and line
+
+        A Linepart namedtuple contains a start, stop, and word. None is returned
+        if no target for this type of completion is found under the cursor."""
         raise NotImplementedError
 
     def format(self, word):
@@ -346,28 +348,47 @@ def matches(self, cursor_offset, line, **kwargs):
             return None
         locals_ = kwargs['locals_']
 
-        r = self.locate(cursor_offset, line)
-        if r is None:
+        full = self.locate(cursor_offset, line)
+        if full is None:
             return None
-        member_part = r[2]
-        _, _, dexpr = lineparts.current_array_with_indexer(cursor_offset, line)
+
+        arr = lineparts.current_indexed_member_access_identifier(
+                cursor_offset, line)
+        index = lineparts.current_indexed_member_access_identifier_with_index(
+                cursor_offset, line)
+        member = lineparts.current_indexed_member_access_member(
+                cursor_offset, line)
+
         try:
-            locals_['temp_val_from_array'] = safe_eval(dexpr, locals_)
+            obj = safe_eval(arr.word, locals_)
+        except EvaluationError:
+            return None
+        if type(obj) not in (list, tuple) + string_types:
+            # then is may be unsafe to do attribute lookup on it
+            return None
+
+        try:
+            locals_['temp_val_from_array'] = safe_eval(index.word, locals_)
         except (EvaluationError, IndexError):
-            return set()
+            return None
 
-        temp_line = line.replace(member_part, 'temp_val_from_array.')
+        temp_line = line.replace(index.word, 'temp_val_from_array.')
 
         matches = self.completer.matches(len(temp_line), temp_line, **kwargs)
+        if matches is None:
+            return None
+
         matches_with_correct_name = \
-            set(match.replace('temp_val_from_array.', member_part) for match in matches)
+            set(match.replace('temp_val_from_array.', index.word+'.')
+                for match in matches if match[20:].startswith(member.word))
 
         del locals_['temp_val_from_array']
 
         return matches_with_correct_name
 
     def locate(self, current_offset, line):
-        return lineparts.current_array_item_member_name(current_offset, line)
+        a = lineparts.current_indexed_member_access(current_offset, line)
+        return a
 
     def format(self, match):
         return after_last_dot(match)
diff --git a/bpython/line.py b/bpython/line.py
@@ -228,25 +228,38 @@ def current_string_literal_attr(cursor_offset, line):
     return None
 
 
-current_array_with_indexer_re = LazyReCompile(
-    r'''([\w_][\w0-9._]*(?:\[[a-zA-Z0-9_"']+\])+)\.(.*)''')
+current_indexed_member_re = LazyReCompile(
+    r'''([a-zA-Z_][\w.]*)\[([a-zA-Z0-9_"']+)\]\.([\w.]*)''')
 
 
-def current_array_with_indexer(cursor_offset, line):
-    """an array and indexer, e.g. foo[1]"""
-    matches = current_array_with_indexer_re.finditer(line)
+def current_indexed_member_access(cursor_offset, line):
+    """An identifier being indexed and member accessed"""
+    matches = current_indexed_member_re.finditer(line)
     for m in matches:
-        if m.start(1) <= cursor_offset and m.end(1) <= cursor_offset:
+        if m.start(3) <= cursor_offset and m.end(3) >= cursor_offset:
+            return LinePart(m.start(1), m.end(3), m.group())
+
+
+def current_indexed_member_access_identifier(cursor_offset, line):
+    """An identifier being indexed, e.g. foo in foo[1].bar"""
+    matches = current_indexed_member_re.finditer(line)
+    for m in matches:
+        if m.start(3) <= cursor_offset and m.end(3) >= cursor_offset:
             return LinePart(m.start(1), m.end(1), m.group(1))
 
 
-current_array_item_member_name_re = LazyReCompile(
-    r'''([\w_][\w0-9._]*(?:\[[a-zA-Z0-9_"']+\])+\.)(.*)''')
+def current_indexed_member_access_identifier_with_index(cursor_offset, line):
+    """An identifier being indexed with the index, e.g. foo[1] in foo[1].bar"""
+    matches = current_indexed_member_re.finditer(line)
+    for m in matches:
+        if m.start(3) <= cursor_offset and m.end(3) >= cursor_offset:
+            return LinePart(m.start(1), m.end(2)+1,
+                            "%s[%s]" % (m.group(1), m.group(2)))
 
 
-def current_array_item_member_name(cursor_offset, line):
-    """the member name after an array indexer, e.g. foo[1].bar"""
-    matches = current_array_item_member_name_re.finditer(line)
+def current_indexed_member_access_member(cursor_offset, line):
+    """The member name of an indexed object, e.g. bar in foo[1].bar"""
+    matches = current_indexed_member_re.finditer(line)
     for m in matches:
-        if m.start(2) <= cursor_offset and m.end(2) >= cursor_offset:
-            return LinePart(m.start(1), m.end(2), m.group(1))
+        if m.start(3) <= cursor_offset and m.end(3) >= cursor_offset:
+            return LinePart(m.start(3), m.end(3), m.group(3))
diff --git a/bpython/test/test_autocomplete.py b/bpython/test/test_autocomplete.py
@@ -283,6 +283,32 @@ def test_att_matches_found_on_old_style_instance(self):
                                              locals_={'a': [OldStyleFoo()]}),
                             set(['a[0].method', 'a[0].a', 'a[0].b']))
 
+    def test_other_getitem_methods_not_called(self):
+        class FakeList(object):
+            def __getitem__(inner_self, i):
+                self.fail("possibly side-effecting __getitem_ method called")
+
+        self.com.matches(5, 'a[0].', locals_={'a': FakeList()})
+
+    def test_tuples_complete(self):
+        self.assertSetEqual(self.com.matches(5, 'a[0].',
+                            locals_={'a': (Foo(),)}),
+                            set(['a[0].method', 'a[0].a', 'a[0].b']))
+
+    @unittest.skip('TODO, subclasses do not complete yet')
+    def test_list_subclasses_complete(self):
+        class ListSubclass(list): pass
+        self.assertSetEqual(self.com.matches(5, 'a[0].',
+                            locals_={'a': ListSubclass([Foo()])}),
+                            set(['a[0].method', 'a[0].a', 'a[0].b']))
+
+    def test_getitem_not_called_in_list_subclasses_overriding_getitem(self):
+        class FakeList(list):
+            def __getitem__(inner_self, i):
+                self.fail("possibly side-effecting __getitem_ method called")
+
+        self.com.matches(5, 'a[0].', locals_={'a': FakeList()})
+
 
 class TestMagicMethodCompletion(unittest.TestCase):
 
diff --git a/bpython/test/test_line_properties.py b/bpython/test/test_line_properties.py
@@ -5,7 +5,9 @@
     current_string, current_object, current_object_attribute, \
     current_from_import_from, current_from_import_import, current_import, \
     current_method_definition_name, current_single_word, \
-    current_string_literal_attr
+    current_string_literal_attr, current_indexed_member_access_identifier, \
+    current_indexed_member_access_identifier_with_index, \
+    current_indexed_member_access_member
 
 
 def cursor(s):
@@ -20,7 +22,7 @@ def decode(s):
 
     if not s.count('|') == 1:
         raise ValueError('match helper needs | to occur once')
-    if s.count('<') != s.count('>') or not s.count('<') in (0, 1):
+    if s.count('<') != s.count('>') or s.count('<') not in (0, 1):
         raise ValueError('match helper needs <, and > to occur just once')
     matches = list(re.finditer(r'[<>|]', s))
     assert len(matches) in [1, 3], [m.group() for m in matches]
@@ -305,6 +307,40 @@ def test_simple(self):
         self.assertAccess('"hey".asdf d|')
         self.assertAccess('"hey".<|>')
 
+class TestCurrentIndexedMemberAccessIdentifier(LineTestCase):
+    def setUp(self):
+        self.func = current_indexed_member_access_identifier
+
+    def test_simple(self):
+        self.assertAccess('<abc>[def].ghi|')
+        self.assertAccess('<abc>[def].|ghi')
+        self.assertAccess('<abc>[def].gh|i')
+        self.assertAccess('abc[def].gh |i')
+        self.assertAccess('abc[def]|')
+
+
+class TestCurrentIndexedMemberAccessIdentifierWithIndex(LineTestCase):
+    def setUp(self):
+        self.func = current_indexed_member_access_identifier_with_index
+
+    def test_simple(self):
+        self.assertAccess('<abc[def]>.ghi|')
+        self.assertAccess('<abc[def]>.|ghi')
+        self.assertAccess('<abc[def]>.gh|i')
+        self.assertAccess('abc[def].gh |i')
+        self.assertAccess('abc[def]|')
+
+
+class TestCurrentIndexedMemberAccessMember(LineTestCase):
+    def setUp(self):
+        self.func = current_indexed_member_access_member
+
+    def test_simple(self):
+        self.assertAccess('abc[def].<ghi|>')
+        self.assertAccess('abc[def].<|ghi>')
+        self.assertAccess('abc[def].<gh|i>')
+        self.assertAccess('abc[def].gh |i')
+        self.assertAccess('abc[def]|')
 
 if __name__ == '__main__':
     unittest.main()
