[ty] Offer string literal completion suggestions in function calls, annotated assignments, and TypedDict contexts#22154
Conversation
…nnotated assignments
Minibrams
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
added
server
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
most of our completions tests are in |
|
(Thanks for the PR! 😃) |
|
Found time to add completions for TypedDict keys as well, so went ahead and added it. I'll leave it alone for review now 😄 |
No particularly good reason, it just seemed like the natural place to put them 😄 Wasn't sure what the scope of the tests in crates/ty_ide/completions.rs was, but was sure I wanted to write e2e tests, so there they went. I tend to prefer e2e tests as I find they capture the important bits of functionality more naturally than typical unit tests do. But I see some of the tests in crates/ty_ide/completions.rs practically do the same thing but with less setup so I can put them in there instead no problem if we prefer. |
|
If there's no specific reason these tests need the full e2e framework, I'd be inclined to put them with most of our other completions tests in |
AlexWaygood
changed the title
…e/src/completion.rs
Done |
|
(Sorry for the slow review here, most of us have been on leave over the Christmas period! Normal Astral review response times should resume from the beginning of next week 😄) |
| return None; | ||
| } | ||
|
|
||
| if cursor.is_in_string() { |
There was a problem hiding this comment.
I would do if let Some(literal) = cursor.string_literal() { here. That would avoid calling cursor.is_in_string() twice.
There was a problem hiding this comment.
This breaks the incomplete t-string tests like no_completions_in_tstring_incomplete_double_quote since there the cursor is inside a string token, but there's no ExprStringLiteral node yet (which cursor.string_literal() requires)
There was a problem hiding this comment.
I'm not following. If there's no ExprStringLiteral, then doesn't cursor.string_literal() return None and thus this function bails and no completions are provided?
There was a problem hiding this comment.
Basically, string_literal() starts by bailing when cursor.is_in_string() is false. But the only call to string_literal() is guarded by the condition that cursor.is_in_string() is true. I think there should be a simplification here?
crates/ty_ide/src/completion.rs
Outdated
| /// Determine whether we're in a syntactic place where literal suggestions make sense. | ||
| fn string_literal_site(&self) -> Option<StringLiteralSite<'m>> { | ||
| let range = TextRange::empty(self.offset); | ||
| let covering = self.covering_node(TextRange::empty(self.offset)); |
There was a problem hiding this comment.
| let covering = self.covering_node(TextRange::empty(self.offset)); | |
| let covering = self.covering_node(range); |
crates/ty_ide/src/completion.rs
Outdated
| } | ||
|
|
||
| /// Determine whether we're in a syntactic place where literal suggestions make sense. | ||
| fn string_literal_site(&self) -> Option<StringLiteralSite<'m>> { |
There was a problem hiding this comment.
I'm curious about the approach here. It seems fraught to need to enumerate all the possible places where the type of a string literal is known. For example:
from typing import Literal
type A = Literal["foo", "bar", "baz"]
xs: list[A] = ["<CURSOR>"]I don't mean to suggest that you should add the above case here, but rather, is there a more general approach we can use here? This approach gets further complicated below where there is a lot logic dedicated to each particular instance. This will also only grow as more sources of string literals are supported.
It seems like we want to offer these sorts of completions anywhere a string literal is found. Can we find the type of a string literal more generally and then offer completions from there? It seems like that should be possible.
(Maybe I'm wrong about this. @AlexWaygood might be a good person to chime in here.)
There was a problem hiding this comment.
That would definitely be a more robust approach, do we have any expected_type(expr) or similar API in the codebase currently? If not I can take a shot at one later today.
There was a problem hiding this comment.
I have added a HasExpectedType API in SemanticModel with a expected_type() function that returns the contextual type annotation (if any), which add_string_literal_completions() now uses to offer completions anywhere the expected type of an expression is a string literal.
I also added your list-of-literals example as a test-case.
There was a problem hiding this comment.
Oof, 20% performance hit. Happy to hear ideas on how to optimize. Maybe we could lazyload expected types?
…xpression is string literal
feat: offer string literal completions always when expected type of e…
CodSpeed Performance ReportMerging this PR will degrade performance by 19.45%Comparing Summary
Performance Changes
Footnotes
|
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Thank you for working on this! I feel like the new approach here is probably the right way to go about this instead of trying to write bespoke code for each type of string literal. I left a number of comments specifically on the completion side of things here.
I'm less well equipped to review the changes for adding HasExpectedType though. It might make sense to land that in a separate PR and get review from probably @AlexWaygood. But I'll defer to Alex to whether a separate PR makes sense.
| return None; | ||
| } | ||
|
|
||
| if cursor.is_in_string() { |
There was a problem hiding this comment.
I'm not following. If there's no ExprStringLiteral, then doesn't cursor.string_literal() return None and thus this function bails and no completions are provided?
| .and_then(|ty| imp(db, ty, &CompletionKindVisitor::default())) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this should go away. I'm guessing it slipped in via a bad merge or something.
| return None; | ||
| } | ||
|
|
||
| if cursor.is_in_string() { |
There was a problem hiding this comment.
Basically, string_literal() starts by bailing when cursor.is_in_string() is false. But the only call to string_literal() is guarded by the condition that cursor.is_in_string() is true. I think there should be a simplification here?
| fn add_string_literal_completions<'db>( | ||
| db: &'db dyn Db, | ||
| file: File, | ||
| _parsed: &ParsedModuleRef, |
There was a problem hiding this comment.
I think this should be removed? It's available at cursor.parsed anyway.
| } | ||
| } | ||
|
|
||
| fn escape_for_quote(value: &str, quote: char) -> Cow<'_, str> { |
There was a problem hiding this comment.
It seems like you could accept QuoteStyle here and just do QuoteStyle::as_char(). And then delete the case analysis above.
There was a problem hiding this comment.
Also, please add a contract to this function.
It took me a minute to figure out why this was needed, so saying more about that would help too.
Also, from a quick glance, I don't see tests that exercise a code path that uses this.
Finally, are we sure a routine like this doesn't already exist? It looks like maybe there is something in ruff_python_literal. And if not, perhaps this routine should live there. cc @MichaReiser
| let insert: Box<str> = match escaped { | ||
| Cow::Borrowed(s) => Box::<str>::from(s), | ||
| Cow::Owned(s) => s.into_boxed_str(), | ||
| }; |
There was a problem hiding this comment.
If we're just going to create a Box<str> anyway, then why have escape_for_quote return a Cow?
Even better, probably just have escape_for_quote return a Name, since that's ultimately what we want here.
| }; | ||
|
|
||
| for (value, literal_ty) in out { | ||
| let escaped = escape_for_quote(value, quote_char); |
There was a problem hiding this comment.
Hmmm. Somewhat recently, I changed the completion display to prioritize the actual insertion text:
But I think this implies that this will show escape quotes to the human when that really should be hidden.
I guess this means that we probably shouldn't show the insertion text in completions and instead have more sophistication around just improving name instead.
I'd be fine ignoring this for now and creating an issue when this PR gets merged tracking a fix for this.
| } | ||
| Type::TypeAlias(alias) => { | ||
| collect_string_literals_from_type(db, alias.value_type(db), seen_types, out); | ||
| } |
There was a problem hiding this comment.
My understanding is that if we're doing recursion based on the structure of Type, then we need cycle detection. e.g.,
ruff/crates/ty_ide/src/completion.rs
Lines 2331 to 2387 in 1094009
| Type::Intersection(intersection) => { | ||
| for ty in intersection.iter_positive(db) { | ||
| collect_string_literals_from_type(db, ty, seen_types, out); | ||
| } |
There was a problem hiding this comment.
Should this only add values present in all branches of the intersection?
| collect_string_literals_from_type(db, alias.value_type(db), seen_types, out); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
Can you add tests the cover each of these cases?
|
Can either of you say more about what |
|
Putting back into draft pending addressing review comments. |
5 participants
Summary
This PR adds completion logic to the
tylanguage server to offer string literal completions when the user is typing a string literal in a function call (arg or kwarg), in the rhs of an annotated assignment, or as a key in a TypedDict-compatible object.This functionality addresses two of the items in the
tyCode completion plan.Fixes astral-sh/ty#2189
Test Plan
Four e2e tests have been added to
./crates/ty_server/tests/e2e/completions.rs: