[ty] Better class def completions#22571
Conversation
We also rank `Protocol`, `TypedDict`, `NamedTuple` and `Generic` equal to `ClassLiteral` as they are of type `SpecialForm` but can still be inherited from.
RasmusNygren
requested review from
AlexWaygood,
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
added
server
| fn is_in_class_def(&self) -> bool { | ||
| for node in self.covering_node.ancestors() { | ||
| if let ast::AnyNodeRef::StmtClassDef(class_def) = node { | ||
| return class_def | ||
| .arguments | ||
| .as_ref() | ||
| .is_some_and(|args| args.range.contains_range(self.range)); | ||
| } | ||
| if node.is_statement() { | ||
| return false; | ||
| } | ||
| } | ||
| false |
There was a problem hiding this comment.
This leads to us calculating whether or not we're in a class def twice, here and in add_argument_completions. I think this is fine for now but we probably want to do some refactors here and store more information on the context such that we only run the detection once and can re-use that both to add completions and for ranking. Perhaps the NonImportContext can store another enum and track if we're in a class-def/raising_exception etc.
Have you already thought about how to best scale this @BurntSushi ?
There was a problem hiding this comment.
I haven't thought too much about this, no. But what you say sounds plausible to me. Possibly the challenge here is making sure we don't try to pre-compute too much stuff, particularly if we end up not needing it.
But yeah I agree that this is fine for now. Especially given that covering_node is only computed once now. :-)
MichaReiser
requested review from
BurntSushi
and removed request for
carljm,
dcreager and
sharkdp
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Beautiful. This looks great as-is, thank you!!!
| fn is_in_class_def(&self) -> bool { | ||
| for node in self.covering_node.ancestors() { | ||
| if let ast::AnyNodeRef::StmtClassDef(class_def) = node { | ||
| return class_def | ||
| .arguments | ||
| .as_ref() | ||
| .is_some_and(|args| args.range.contains_range(self.range)); | ||
| } | ||
| if node.is_statement() { | ||
| return false; | ||
| } | ||
| } | ||
| false |
There was a problem hiding this comment.
I haven't thought too much about this, no. But what you say sounds plausible to me. Possibly the challenge here is making sure we don't try to pre-compute too much stuff, particularly if we end up not needing it.
But yeah I agree that this is fine for now. Especially given that covering_node is only computed once now. :-)
3 participants
Summary
Prefer completions of type
ClassLiteralinside class-def context.Fixes astral-sh/ty#1776
Test Plan
New completion-eval test that has incorrect ranking on main but correct after this patch.