-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Improve support for object.<CURSOR> completions
#18629
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
Conversation
This commit doesn't change any functionality, but instead changes the representation of `CoveringNode` to make the implementation simpler (as well as planned future additions). By putting the found node last in the list of ancestors (now just generically called `nodes`), we reduce the amount of special case handling we need. The downside is that the representation now allows invalid states (a `CoveringNode` with no elements). But I think this is well mitigated by encapsulation.
This routine lets us climb up the AST tree when we find a contiguous sequence of nodes that satisfy our predicate. This will be useful for making things like `a.b.<CURSOR>` work. That is, we don't want the `ExprAttribute` closest to a leaf. We also don't always want the `ExprAttribute` closest to the root. Rather, (I think) we want the `ExprAttribute` closest to the root that has an unbroken chain to the `ExprAttribute` closest to the leaf.
|
AlexWaygood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
| }; | ||
| CompletionTargetTokens::Generic { token: last } | ||
| } | ||
| Some( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably out of scope for this PR. But for something like import collections.<CURSOR> or from collections.<CURSOR>, do we want to be able to provide autocomplete suggestions for the collections.abc submodule of collections in due course? (That would be cool!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely. Working on import auto-completions (from module import <CURSOR>) is next on my list. I think it would make sense to put import module.<CURSOR> in that bucket too.
| } else if let Some([_]) = token_suffix_by_kinds(before, [TokenKind::Ellipsis]) { | ||
| // Similarly as above. If we've just typed an ellipsis, | ||
| // then we shouldn't show completions. Note that | ||
| // this doesn't prevent `....<CURSOR>` from showing | ||
| // completions (which would be the attributes available | ||
| // on an `ellipsis` object). | ||
| return None; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd. I can't repro that in neovim. That is, if I have this:
class Foo:
def bar(self): ...<CURSOR>
Then I don't get completions, even if I specifically ask for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh... FWIW, the way I tested this PR locally with the playground was to checkout this branch, navigate to the playground directory, then run npm start --workspace ty-playground
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(no idea why the playground might be doing something different to neovim -- @MichaReiser or @dhruvmanila might be able to help there!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this initially when running npm start --workspace ty-playground:
[INFO]: ⬇️ Installing wasm-bindgen...
[INFO]: License key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory
[INFO]: ✨ Done in 23.87s
[INFO]: 📦 Your wasm pkg is ready to publish at ../../playground/ty/ty_wasm.
> ty-playground@0.0.0 start
> vite
sh: line 1: vite: command not found
npm error Lifecycle script `start` failed with error:
npm error code 127
npm error path /home/andrew/astral/ruff/pr1/playground/ty
npm error workspace ty-playground@0.0.0
npm error location /home/andrew/astral/ruff/pr1/playground/ty
npm error command failed
npm error command sh -c vite
npm notice
npm notice New minor version of npm available! 11.3.0 -> 11.4.1
npm notice Changelog: https://github.com/npm/cli/releases/tag/v11.4.1
npm notice To update run: npm install -g npm@11.4.1
npm notice
I then went to install vite via the AUR and got this build error:
CMake Error at CMakeLists.txt:237 (message):
libglm-dev package is required, you might specify the include directory
where to find glm/glm.hpp through -DGLM_INC=/path/to/glm
I then installed glm and tried again and got more build errors:
CMake Error in src/CMakeLists.txt:
Found relative path while evaluating include directories of "vite":
"GLEW_INCLUDE_PATH-NOTFOUND"
CMake Error in src/CMakeLists.txt:
Found relative path while evaluating include directories of "vite":
"GLEW_INCLUDE_PATH-NOTFOUND"
CMake Error in src/CMakeLists.txt:
Found relative path while evaluating include directories of "vite":
"GLEW_INCLUDE_PATH-NOTFOUND"
I'll try futzing with this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I got it running... Does it serve it as a web site somewhere? How do I get to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be serving at localhost:5173 -- it should also say this on its terminal output, if it's working correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I see:
[andrew@duff playground]$ npm start --workspace ty-playground
> ty-playground@0.0.0 prestart
> npm run dev:wasm
> ty-playground@0.0.0 dev:wasm
> wasm-pack build ../../crates/ty_wasm --dev --target web --out-dir ../../playground/ty/ty_wasm
[INFO]: 🎯 Checking for the Wasm target...
[INFO]: 🌀 Compiling to Wasm...
Compiling ruff_python_ast v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ruff_python_ast)
Compiling ty_wasm v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ty_wasm)
Compiling ruff_python_parser v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ruff_python_parser)
Compiling ruff_python_literal v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ruff_python_literal)
Compiling ruff_db v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ruff_db)
Compiling ty_python_semantic v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ty_python_semantic)
Compiling ty_vendored v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ty_vendored)
Compiling ruff_python_formatter v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ruff_python_formatter)
Compiling ty_ide v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ty_ide)
Compiling ty_project v0.0.0 (/home/andrew/astral/ruff/pr1/crates/ty_project)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 10.36s
[INFO]: ⬇️ Installing wasm-bindgen...
[INFO]: License key is set in Cargo.toml but no LICENSE file(s) were found; Please add the LICENSE file(s) to your project directory
[INFO]: ✨ Done in 13.45s
[INFO]: 📦 Your wasm pkg is ready to publish at ../../playground/ty/ty_wasm.
> ty-playground@0.0.0 start
> vite
A "vite" window opens, but it's blank. And:
[andrew@duff playground]$ curl localhost:5173
curl: (7) Failed to connect to localhost port 5173 after 0 ms: Could not connect to server
I am doing this on a remote box... And I'm guessing the vite window is opening via X11 forwarding. So I wonder if that's messing with things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it looks like things are hanging right after running vite. So this is probably an X11 forwarding issue. Boooooo.
I'll try this directly on my laptop tomorrow.
| #[test] | ||
| fn betwixt_attribute_access1() { | ||
| let test = cursor_test( | ||
| "\ | ||
| class Foo: | ||
| xyz: str | ||
| class Bar: | ||
| foo: Foo | ||
| class Quux: | ||
| bar: Bar | ||
| quux = Quux() | ||
| quux.<CURSOR>.foo.xyz | ||
| ", | ||
| ); | ||
|
|
||
| test.assert_completions_include("bar"); | ||
| test.assert_completions_do_not_include("xyz"); | ||
| test.assert_completions_do_not_include("foo"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also probably not something for this PR, but consider something like this:
Eventually, it would be great if bar and mro were positioned above a in the completion suggestions in this screenshot. Foo.bar and Foo.mro both have a __defaults__ attribute (they're both FunctionType instances), but Foo.a does not (it's an int)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a tricky one. Not just taking the prefix context into account, but the suffix context as well. I added that to the completion notes.
|
Demo: object-attr-improved.mp4(I still haven't setup VS Code.) |
This makes it work for a number of additional cases, like nested attribute access and things like `[].<CURSOR>`. The basic idea is that instead of selecting a covering node closest to a leaf that contains the cursor, we walk up the tree as much as we can. This lets us access the correct `ExprAttribute` node when performing nested access.
7b4fb09 to
7da22fc
Compare
|
Going to bring this in even with the unknown playground issue (which isn't a regression) since it improves a lot of other cases. I'm happy to do follow-ups if folks have them. |
dhruvmanila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good!
I haven't been able to reproduce the nested context issue that Alex pointed out, I've asked him to see if it's reproducible now otherwise we can ignore that for now.
I think it might be useful to add couple of tests when the dot (.) character is inside a string and f-string. Following are couple of examples that already work correctly:
foo = 1
bar = 2
class Foo:
def method(self): ...
f = Foo()
# String, this is not an attribute access
"f.<CURSOR>
# F-string, this is an attribute access
f"{f.<CURSOR>
This still reproduces for me on https://play.ty.dev/: Screen.Recording.2025-06-12.at.11.59.49.mov |
Interesting! Thanks, I'm able to reproduce it on the playground and in VS Code but not in Neovim!
And, the trace logs: The above trace logs is for the following snippet along with the cursor position: class Foo:
class Bar: .<CURSOR> |
|
Interesting. I've recorded that in my notes to try and address later. Once I get around to setting up VS Code. |





This makes it work for a number of additional cases, like nested
attribute access and things like
[].<CURSOR>.The basic idea is that instead of selecting a covering node closest to a
leaf that contains the cursor, we walk up the tree as much as we can.
This lets us access the correct
ExprAttributenode when performingnested access.
This also adds a number of tests covering some interesting cases.
Ref astral-sh/ty#86