Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jedi autocomplete mistakes path literals (p"foo") for strings #1

Open
Techcable opened this issue Feb 25, 2022 · 5 comments
Open

Jedi autocomplete mistakes path literals (p"foo") for strings #1

Techcable opened this issue Feb 25, 2022 · 5 comments

Comments

@Techcable
Copy link

Techcable commented Feb 25, 2022

xonfig

+------------------+--------------------------+
| xonsh            | 0.11.0                   |
| Git SHA          | 4af68065                 |
| Commit Date      | Jan 8 20:16:06 2022      |
| Python           | 3.9.10                   |
| PLY              | 3.11                     |
| have readline    | True                     |
| prompt toolkit   | 3.0.27                   |
| shell type       | prompt_toolkit           |
| history backend  | sqlite                   |
| pygments         | 2.11.2                   |
| on posix         | True                     |
| on linux         | False                    |
| on darwin        | True                     |
| on windows       | False                    |
| on cygwin        | False                    |
| on msys2         | False                    |
| is superuser     | False                    |
| default encoding | utf-8                    |
| xonsh encoding   | utf-8                    |
| encoding errors  | surrogateescape          |
| on jupyter       | False                    |
| jupyter kernel   | None                     |
| xontrib 1        | back2dir                 |
| xontrib 2        | fish_completer           |
| xontrib 3        | jedi                     |
| xontrib 4        | term_integration         |
| RC file 1        | /Users/nicholas/.xonshrc |
+------------------+--------------------------+

Expected Behavior

p".".<TAB> should autocomplete as if it was a pathlib.Path, not a str

Current Behavior

image

I am using Jedi version 0.18.1 (if that makes any difference).

I would be happy to work on a fix if you can point me in the right direction :)

Steps to Reproduce

Install jedi xontrib

Just do p"~/Documents".<TAB> and it will autocomplete like a string instead of a Path

For community

⬇️ Please click the 👍 reaction instead of leaving a +1 or 👍 comment

@dev2718
Copy link

dev2718 commented Feb 25, 2022

The problem is that the jedi xontrib is not aware of any xonsh syntax, but treats the input as python code.
During parsing, while creating the AST, the path literal will get transformed to a function call here.
So one possible solution could be to try to parse the input to an AST, then ast.unparse it back to a python representation, and feed that to jedi.

@Techcable
Copy link
Author

The problem is that the jedi xontrib is not aware of any xonsh syntax, but treats the input as python code.
During parsing, while creating the AST, the path literal will get transformed to a function call here.

Thank you for pointing me in the right direction!

What you linked is for glob and regex literals like `foo.*` , g`foo.*` or gp`.*`. These are converted into calls to builtin __xonsh__.globsearch or __xonsh__.regexsearch. Like you said, this syntax isn't understood at all by Jedi.

My original issue was about path literals like p"foo". These appear to be handled to be handled by converting calls into the builtin path_literal function here.

So one possible solution could be to try to parse the input to an AST, then ast.unparse it back to a python representation, and feed that to jedi.

I can maybe try this. Do you think this would this slow things down too much?

I also noticed that the code for builtins has no type annotations. This is likely to make Jedi autocomplete much harder even if we do AST expansion.

I see that the _xonsh__.path_ literal builtin function isn't annotated with a return type (it should be pathlib.Path). That should be easy to fix and improve Jedi completion (assuming we also do AST unpause).

Annotating __xonsh__.globsearch would be more difficult, because the type it returns depends on the pathlib argument.

Based on my tests, the best way to "type" this function looks like returning Union[List[str], List[Path]] or List[Union[Path, str]]. Even if we do AST unparse, this would give lower quality autocomplete because Jedi probably can't tell if the result is list[Path] or list[str].

However Jedi ambiguity is still better than the current situation where p'foo' is treated as 'str' and gp`.*` isn't understood at all 😉

@jnoortheen jnoortheen added the enhancement New feature or request label Feb 26, 2022
@anki-code anki-code added xontrib-jedi completion and removed enhancement New feature or request labels Feb 26, 2022
@dev2718
Copy link

dev2718 commented Feb 26, 2022

I can maybe try this. Do you think this would this slow things down too much?

I don't think this would be a problem, especially as this is only about single lines in interactive use. Also, the delay caused by jedi itself can easily be an order of magnitude higher.

The bigger problem I think is that we can only parse syntactically correct (i.e. usually only finished) lines, and that we need column information for jedi that will be hard to track if the line gets transformed like this.

So maybe it would be better to invoke the lexer like this

from xonsh.lexer import Lexer
lexer = Lexer()
lexer.input(src)
tokens = list(lexer)

to identify path literal tokens (and others like $().
These could then be replaced with the corresponding builtin call, while keeping track of column information using the lexpos token attribute.

This does not need to be completely right (i.e. the arguments passed to the builtin should be irrelevant for autocompletion).

I also noticed that the code for builtins has no type annotations. This is likely to make Jedi autocomplete much harder even if we do AST expansion.

Type annotations could certainly be added, xonsh uses them at many places but not everywhere yet.

Annotating __xonsh__.globsearch would be more difficult, because the type it returns depends on the pathlib argument.

Should this mechanism work, I think we could think about factoring this into multiple functions to enable precise type annotations.

@dev2718
Copy link

dev2718 commented Mar 7, 2022

@Techcable are you working on this or should I give it a try?

@Techcable
Copy link
Author

@Techcable are you working on this or should I give it a try?

Feel free to give it a try! I haven't even started yet :)

@anki-code anki-code transferred this issue from xonsh/xonsh Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants