Fix Tab Completion Context Detection#14838
Conversation
|
@Darshan808 can you merge my branch with a test case into your branch and then I will close #14837? It looks like CI indicates a failure on a pre-existing test: General feedback on the fix:
|
|
I am not sure if the test failure is due to my changes. Initially, 4 tests were failing, and on rerun, only 1 is failing. When I run it locally, the test sometimes passes and sometimes fails—are these tests flaky? Also, about adding tests for my changes—I tested the I'll start by adding a test to verify |
|
I think it a good idea to have both unit tests (for |
|
I’ve added two more integration tests: one for completion inside f-strings and another for completion inside dictionaries. I believe these cover the main cases, but do you think there are any other scenarios worth testing? Also, regarding the test failures, I see that one test, test_greedy_completions, is consistently failing. I’m looking into it, but I haven’t found any reasons for why it’s failing yet. |
|
I modified the regex to check for patterns like |
| if "#" in text: | ||
| parts = text.split("#") | ||
| if len(parts) > 1: | ||
| return True |
There was a problem hiding this comment.
- What is the
split()used for? - What about
d["#"].a- this will be relatively common with pandas column access, some columns will contain#in name
There was a problem hiding this comment.
Improved the logic for comment checking!
| i = 0 | ||
|
|
||
| while i < len(text): | ||
| # Check for f-string start |
There was a problem hiding this comment.
To be future-proof we could allow t-strings (https://peps.python.org/pep-0750/). There is a strong indication that this JEP will be approved (https://discuss.python.org/t/pep750-template-strings-new-updates/71594/118).
There was a problem hiding this comment.
Thanks. Added this!
| if line.endswith("."): | ||
| return self._CompletionContextType.ATTRIBUTE | ||
|
|
||
| last_token_match = re.search(r"([\w]+)$", line) |
There was a problem hiding this comment.
Can you add a comment with an example explaining which case it addresses?
In Python tokens can include digits (but cannot start with digits), not sure if this is relevant here yet.
There was a problem hiding this comment.
Added comments!
can include digits (but cannot start with digits)
Initially, I didn't think this was important, but I've now updated the regex to account for it.
|
After reanalyzing the logic, I found redundant checks and simplified it to a single regex search. |
| """Match attributes or global python names""" | ||
| text = context.line_with_cursor | ||
| if "." in text: | ||
| completion_type = self._determine_completion_context(text) |
There was a problem hiding this comment.
A fundamental problem here is that it ignores the cursor position. So for example this works:
This fails:
I think we need to use context.text_until_cursor instead, see:
ipython/IPython/core/completer.py
Lines 618 to 653 in 801c9ad
| and text[i] in ("f", "t") | ||
| and (text[i + 1] == '"' or text[i + 1] == "'") | ||
| ): | ||
| in_template_string = True |
There was a problem hiding this comment.
This is never reset to False, here is how it fails - can you add a test case?
| # Ex: 'var', '', '3.', '-42.5.' | ||
| return self._CompletionContextType.GLOBAL | ||
|
|
||
| def _is_in_string_or_comment(self, text): |
There was a problem hiding this comment.
This will fail on nested f-strings:
I am not sure if we want to support it (if it is easy then sure, if it is hard - not sure), but at least need to document it, possibly by adding xfail test?
| # Match 'anything-dot-word' at end for attribute context. | ||
| # Ex: 'obj.', 'np.random.ran'. | ||
| chain_match = re.search(r"([a-zA-Z_].*)\.([a-zA-Z_][a-zA-Z0-9_]*)?$", line) | ||
| if chain_match: | ||
| return self._CompletionContextType.ATTRIBUTE | ||
|
|
||
| # No attribute pattern → GLOBAL. | ||
| # Ex: 'var', '', '3.', '-42.5.' | ||
| return self._CompletionContextType.GLOBAL |
There was a problem hiding this comment.
Some edge cases not covered, probably not important enough to cover, but listing them for completness:
(3).to_<tab>
expecting (3).to_bytes
Note that 3.to_bytes is invalid in Python
3.1.as_<tab>
-3.1.r<tab>
Expecting 3.1.as_integer_ratio and -3.1.real respectively.
|
covered cases like these 3.1.as_<tab>
-3.1.r<tab>
(3).to_<tab>and used Since Jedi is a dependency of IPython and its completion is very robust, I think we can skip some very special cases like these. |
|
I’ve run into an interesting issue with the python_matcher. For # Debug line
completion_type = self._determine_completion_context(text)
print(completion_type, " for ", text)a = []
In [3]: f'{a.| # suggests .append
_CompletionContextType.ATTRIBUTE for f'{a.
In [4]: f'{a.|}' # suggests .ipynb
_CompletionContextType.ATTRIBUTE for f'{a. |
|
I see that there is a couple of things that could go wrong in: ipython/IPython/core/completer.py Lines 1135 to 1145 in 801c9ad Maybe it is because it uses the If this is not as easy to solve maybe we can open an issue to track it. |
Yes, using argument |
| ATTRIBUTE = "attribute" # For attribute completion | ||
| GLOBAL = "global" # For global completion | ||
|
|
||
| def _determine_completion_context(self, line): |
There was a problem hiding this comment.
Documenting another edge case - double braces represent literals braces so should return global:
There was a problem hiding this comment.
I think fixing this one might be of relatively little value.
| # Match tuples followed by a dot (e.g., (a, b).index) | ||
| tuple_attr_match = re.search( | ||
| r"\(\s*[a-zA-Z_][a-zA-Z0-9_]*\s*(,\s*[a-zA-Z_][a-zA-Z0-9_]*\s*)+\)\.$", line | ||
| ) |
There was a problem hiding this comment.
This is overly strict as it only handles tuples with two or more identifiers. It will miss on cases like:
(a,)(1, 2)('a', 'b')
etc
Now, I don't know if we should special-case tuples over other similar collections (lists, sets). I also worry that making this syntax more complex to account for all these cases would make it slower, and completer needs to be very fast.
I know that using tokenizer out right will often not work as it does not work for incomplete lines, but could we use a hybrid approach where for cases like this - where we assume that there is a valid tuple (list, set) followed by a dot we just remove the final dot and see if tokenizer can parse it and if the result is one of: list, tuple, dictionary, set?
There was a problem hiding this comment.
If you're interested in speed you could compile the expression to speed things up.
|
There are so many edge cases, and trying to make the logic perfect keeps causing some tests to fail. Covering all cases would require a lot of extra checks. My initial approach of matching |
krassowski
left a comment
There was a problem hiding this comment.
Thank you @Darshan808. I think this is a good trade-off between completeness and complexity. I left one tiny suggestion on making part of the code easier to read.
|
@meeseeksdev please backport to 8.x |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
Fixes #14836
This PR resolves the issue with IPython tab completion incorrectly triggering attribute completion when a dot (
.) appears in the line. Instead of just checking for '.', I've added a new function,_determine_completion_context, to accurately determine whether the cursor is in an attribute or global completion context.The solution has been tested against multiple edge cases (e.g., nested expressions, strings, comments) and handles them all correctly. Please review the changes and let me know if you have any feedback or suggestions!