[ty] Add imports when an unimported completion is selected#20439
[ty] Add imports when an unimported completion is selected#20439BurntSushi merged 15 commits intomainfrom
Conversation
BurntSushi
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
BurntSushi
force-pushed
the
ag/refactor-importer
branch
from
e39bc97 to
6ade5f7
Compare
BurntSushi
requested review from
dhruvmanila
and removed request for
carljm and
dcreager
|
Demo: even-slicker-import-demo.mp4 |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
BurntSushi
added
server
BurntSushi
force-pushed
the
ag/refactor-importer
branch
2 times, most recently
from
aebc152 to
8e4d1c7
Compare
There was a problem hiding this comment.
This is great!
I think it would be nice to make use of the LSPs lazy resolution of completion metadata (e.g. lazily resolve the Type of a `Completion) but I agree with your scoping that we should leave this to a separate improvement.
We probably also want some more advanced sorting of completion items to e.g. use the closest (and most high-level) re-export of a symbol over importing it from a very deeply nested submodule (or from a test!). But again, these are incremental improvements that we can make later.
| pub fn into_owned(self) -> Stylist<'static> { | ||
| Stylist { | ||
| source: Cow::Owned(self.source.into_owned()), | ||
| indentation: self.indentation, | ||
| quote: self.quote, | ||
| line_ending: self.line_ending, | ||
| } | ||
| } |
There was a problem hiding this comment.
Neat!
But I'm not sure if this is actually needed, given that we only need into_owned in tests and only in tests that call import (or import_from) of which, I think, most tests only contain exactly one call.
I'm leaning towards creating the Stylist ad-hoc on demand. The only downside is that we compute the line_ending a few more times than necessary but that should be neglectable, given that most test files have very short lines (it only searches for the first line ending)
There was a problem hiding this comment.
It's a little more subtle than that. An Importer wants a &Stylist. So if we create a Stylist on-demand, it can't be tied to the stack frame that creates an Importer and returns that importer.
There are a lot of different ways around this. A Stylist could always own its source for example. Or the tests could always be required to create an Importer in the same stack frame (or above) as where it's used.
But I found this to be simple and it follows a standard API pattern in Rust libraries for creating borrowed/owned versions of the same type. And I don't think the cost here is that big.
| // if predicate: | ||
| // import whatever |
There was a problem hiding this comment.
I think we'll want to support those for type-checking only imports. I think it's totally fine that this PR ignores those, but we should think about when we should add support for them.
Unlike Rust, importing a module has a runtime cost. Because of that, Python supports type-checking only imports, to avoid importing modules that are only needed to type some API.
The way type-checking only imports work in Python is that the imports are gated by an if TYPE_CHECKING guard (it's a well-known constant by type checkers or it can be imported from the typing module):
from typing import TYPE_CHECKING
if TYPE_CHECKING:
import fooNow, symbols imported in a TYPE_CHECKING block aren't available at runtime. That means, they should only be suggested when in a type position (and e.g. not in the condition of an if statement).
It's also preferred to add the imports to an if TYPE_CHECKING block if the import is only used for type checking.
I don't know how far existing LSPs go here but Ruff has a few rules to help with this: https://docs.astral.sh/ruff/rules/#flake8-type-checking-tc
There was a problem hiding this comment.
Yeah good call out! I thought about this when working on the importer, but the "how do you know when it should go into a TYPE_CHECKING block" seemed like it was big enough to be worth punting on for now. I've added this to my notes for completions.
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
This looks very exciting!!
The documentation you've added is (as usual) fantastic. I did spot a few typos reading through the comments, so I ended up doing a docs-only review pass (sorry if some comments here are a bit nitpicky!)
It seems like we'd like to remove `Locator` since it's a bit awkward in how it works: #20439 (comment) It looked pretty easy to rip it out of the `Importer`, so that's one less thing using it.
BurntSushi
force-pushed
the
ag/refactor-importer
branch
from
8e4d1c7 to
56d17da
Compare
As I was playing around in this file, it was much nicer to just use `cst::` everywhere, similar to what we do with `ruff_python_ast`.
This refactors the importer abstraction to use a shared `Insertion`. This is mostly just moving some code around with some slight tweaks. The plan here is to keep the rest of the importing code in `ruff_linter` and then write something ty-specific on top of `Insertion`. This ends up sharing some code, but not as much as would be ideal. In particular, the `ruff_linter` imported is pretty tightly coupled with ruff's semantic model. So to share the code, we'd need to abstract over that.
This makes it easier to test with in some cases and generally shouldn't cost anything.
Basically, given a `from module import name1, name2, ...` statement, we'd like to be able to insert another name in that list. This new `Insertion::existing_import` API provides such functionality. There isn't much to it, although we are careful to try and avoid inserting nonsense for import statements that are already invalid.
This can already be accomplished via a `From` impl (and indeed, that's how this is implemented). But in a generic context, the turbo-fishing that needs to be applied is quite annoying.
We're going to want to use this outside of `ty_python_semantic`. Specifically, in `ty_ide`.
The names of the submodules returned should be *complete*. This is the contract of `Module::name`. However, we were previously only returning the basename of the submodule.
I think this is a better home for it. This way, `ty_ide` more clearly owns how the "kind" of a completion is computed. In particular, it is computed differently for things where we know its type versus unimported symbols.
Based on how this API is currently implemented, this doesn't really cost us anything. But it gives us access to more information about where the symbol is defined.
This rejiggers some stuff in the main completions entrypoint in `ty_ide`. A more refined `Completion` type is defined with more information. In particular, to support auto-import, we now include a module name and an "edit" for inserting an import. This also rolls the old "detailed completion" into the new completion type. Previously, we were relying on the completion type for `ty_python_semantic`. But `ty_ide` is really the code that owns completions. Note that this code doesn't build as-is. The next commit will add the importer used here in `add_unimported_completions`.
This is somewhat inspired by a similar abstraction in `ruff_linter`. The main idea is to create an importer once for a module that you want to add imports to. And then call `import` to generate an edit for each symbol you want to add. I haven't done any performance profiling here yet. I don't know if it will be a bottleneck. In particular, I do expect `Importer::import` (but not `Importer::new`) to get called many times for a single completion request when auto-import is enabled. Particularly in projects with a lot of unimported symbols. Because I don't know the perf impact, I didn't do any premature optimization here. But there are surely some low hanging fruit if this does prove to be a problem. New tests make up a big portion of the diff here. I tried to think of a bunch of different cases, although I'm sure there are more.
It seems like we'd like to remove `Locator` since it's a bit awkward in how it works: #20439 (comment) It looked pretty easy to rip it out of the `Importer`, so that's one less thing using it.
BurntSushi
force-pushed
the
ag/refactor-importer
branch
from
56d17da to
b14afa1
Compare
|
Going to bring this in, but I'm happy to address additional feedback in follow-ups. :-) |
It seems like we'd like to remove `Locator` since it's a bit awkward in how it works: #20439 (comment) It looked pretty easy to rip it out of the `Importer`, so that's one less thing using it.
We don't attempt to fix these yet. I think there are bigger fish to fry. I came up with these based on this discussion: #20439 (comment) Here's one example: ``` if ...: from foo import MAGIC else: from bar import MAGIC MAG<CURSOR> ``` Now in this example, completions will include `MAGIC` from the local scope. That is, auto-import is involved with that completion. But at present, auto-import will suggest importing `foo` and `bar` because we haven't de-duplicated completions yet. Which is fine. Here's another example: ``` if ...: import foo as fubar else: import bar as fubar MAG<CURSOR> ``` Now here, there is no `MAGIC` symbol in scope. So auto-import is in play. Let's assume that the user selects `MAGIC` from `foo` in this example. (`bar` also has `MAGIC`.) Since we currently ignore the declaration site for symbols with multiple possible bindings, the importer today doesn't know that `fubar` _could_ contain `MAGIC`. But even if it did, what would we do with that information? Should we do this? ``` if ...: import foo as fubar from foo import MAGIC else: import bar as fubar MAGIC ``` Or could we reason that `bar` also has `MAGIC`? ``` if ...: import foo as fubar else: import bar as fubar fubar.MAGIC ``` But if we did that, we're making an assumption of user intent, since they *selected* `foo.MAGIC` but not `bar.MAGIC`. Anyway, I don't think we need to settle on an answer today, but I wanted to capture some of these tricky cases in tests at the very least.
We don't attempt to fix these yet. I think there are bigger fish to fry. I came up with these based on this discussion: #20439 (comment) Here's one example: ``` if ...: from foo import MAGIC else: from bar import MAGIC MAG<CURSOR> ``` Now in this example, completions will include `MAGIC` from the local scope. That is, auto-import is involved with that completion. But at present, auto-import will suggest importing `foo` and `bar` because we haven't de-duplicated completions yet. Which is fine. Here's another example: ``` if ...: import foo as fubar else: import bar as fubar MAG<CURSOR> ``` Now here, there is no `MAGIC` symbol in scope. So auto-import is in play. Let's assume that the user selects `MAGIC` from `foo` in this example. (`bar` also has `MAGIC`.) Since we currently ignore the declaration site for symbols with multiple possible bindings, the importer today doesn't know that `fubar` _could_ contain `MAGIC`. But even if it did, what would we do with that information? Should we do this? ``` if ...: import foo as fubar from foo import MAGIC else: import bar as fubar MAGIC ``` Or could we reason that `bar` also has `MAGIC`? ``` if ...: import foo as fubar else: import bar as fubar fubar.MAGIC ``` But if we did that, we're making an assumption of user intent, since they *selected* `foo.MAGIC` but not `bar.MAGIC`. Anyway, I don't think we need to settle on an answer today, but I wanted to capture some of these tricky cases in tests at the very least.
We don't attempt to fix these yet. I think there are bigger fish to fry. I came up with these based on this discussion: #20439 (comment) Here's one example: ``` if ...: from foo import MAGIC else: from bar import MAGIC MAG<CURSOR> ``` Now in this example, completions will include `MAGIC` from the local scope. That is, auto-import is involved with that completion. But at present, auto-import will suggest importing `foo` and `bar` because we haven't de-duplicated completions yet. Which is fine. Here's another example: ``` if ...: import foo as fubar else: import bar as fubar MAG<CURSOR> ``` Now here, there is no `MAGIC` symbol in scope. So auto-import is in play. Let's assume that the user selects `MAGIC` from `foo` in this example. (`bar` also has `MAGIC`.) Since we currently ignore the declaration site for symbols with multiple possible bindings, the importer today doesn't know that `fubar` _could_ contain `MAGIC`. But even if it did, what would we do with that information? Should we do this? ``` if ...: import foo as fubar from foo import MAGIC else: import bar as fubar MAGIC ``` Or could we reason that `bar` also has `MAGIC`? ``` if ...: import foo as fubar else: import bar as fubar fubar.MAGIC ``` But if we did that, we're making an assumption of user intent, since they *selected* `foo.MAGIC` but not `bar.MAGIC`. Anyway, I don't think we need to settle on an answer today, but I wanted to capture some of these tricky cases in tests at the very least.
What lsp feature displays the import path (green text)? In helix, I just see suggested names without imports. They are from different modules and there is no way to tell which is correct. 
|
|
We put it in |
Weird, I think they do support it. I tested ty on I can see them in other LSPs. 
|
|
I'm getting the same behavior in Zed as well. Will test it more tomorrow. 
|
Hmmm, this might be related and I don't think made it into
Thank you!!! <3
Hmmm that's not showing the import path though as far as I can see. It's just showing the kind. Although ty should be including the kind too, and that doesn't seem to be showing up for you either. |
Installed from the main branch. Here is what I see in LSP logs of helix: So I guess label details are not fully supported. Not sure why kind does not work though... |
|
Checked how it's done in other LSP.s gopls puts it in the description (if it's a module import).: It looks like even Zed does not support labelDetails yet, so perhaps it's better to put it somewhere else for now. |
|
Oh nice find! I missed that in r-a. I think you're right that stuffing it somewhere else if the LSP doesn't support |
…supported This fixes a bug where the `import module` part of a completion for unimported candidates would be missing. This makes it especially confusing because the user can't tell where the symbol is coming from, and there is no hint that an `import` statement will be inserted. Previously, we were using [`CompletionItemLabelDetails`] to render the `import module` part of the suggestion. But this is only supported in clients that support version 3.17 (or newer) of the LSP specification. It turns out that this support isn't widespread yet. In particular, Heliex doesn't seem to support "label details." To fix this, we take a [cue from rust-analyzer][rust-analyzer-details]. We detect if the client supports "label details," and if so, use it. Otherwise, we push the `import module` text into the completion label itself. Fixes #20439 (comment) [`CompletionItemLabelDetails`]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemLabelDetails [rust-analyzer-details]: https://github.com/rust-lang/rust-analyzer/blob/5d905576d49233ed843bb40df4ed57e5534558f5/crates/rust-analyzer/src/lsp/to_proto.rs#L391-L404
…supported This fixes a bug where the `import module` part of a completion for unimported candidates would be missing. This makes it especially confusing because the user can't tell where the symbol is coming from, and there is no hint that an `import` statement will be inserted. Previously, we were using [`CompletionItemLabelDetails`] to render the `import module` part of the suggestion. But this is only supported in clients that support version 3.17 (or newer) of the LSP specification. It turns out that this support isn't widespread yet. In particular, Heliex doesn't seem to support "label details." To fix this, we take a [cue from rust-analyzer][rust-analyzer-details]. We detect if the client supports "label details," and if so, use it. Otherwise, we push the `import module` text into the completion label itself. Fixes #20439 (comment) [`CompletionItemLabelDetails`]: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionItemLabelDetails [rust-analyzer-details]: https://github.com/rust-lang/rust-analyzer/blob/5d905576d49233ed843bb40df4ed57e5534558f5/crates/rust-analyzer/src/lsp/to_proto.rs#L391-L404
5 participants
This PR does the work necessary to make the "import"
part of "auto-import" work. That is, when an unimported
completion is found, it is now returned with an LSP text
edit for inserting an import that should bring that symbol
into scope.
The diffstat here is quite large, but a bulk of that are
tests. And this PR should be reviewed commit-by-commit.
Many of the commits are somewhat smaller refactorings.
The last commit is where the new
Importerabstraction isintroduced, and it's where (arguably) most of the weeds are.
For now, I've not concerned myself with performance in order
to avoid getting distracted by it. In particular, every
unimported completion gets an edit generated for it. This
strikes me as sub-optimal, and indeed, it looks like the
LSP protocol has support for running "commands" in response
to a completion being selected. Future work might involve
taking advantage of that, but it's not clear to me yet how
expensive (relatively speaking) generating these edits
actually is.