[ty] Exclude already-included classes in base completions#23085
[ty] Exclude already-included classes in base completions#23085charliermarsh merged 1 commit intomainfrom
Conversation
charliermarsh
requested review from
AlexWaygood,
Gankra,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
There was a problem hiding this comment.
Nice, though I'm not totally sure about the auto-import filtering (would like @BurntSushi's review!)
| if builder.import.is_none() && self.existing_class_bases.contains(&builder.name) { | ||
| return true; | ||
| } | ||
| // For auto-import completions, check if the qualified name matches. |
There was a problem hiding this comment.
For auto-import completions, we might want to do something a bit cleverer that looks to see what kind of import the completion would add...? Because an auto-import completion can result in a from mod import Bar import being added
There was a problem hiding this comment.
Yeah, I agree that this isn't quite either (see above in the non-auto-import case for why that isn't quite right either for similar reasons).
But I don't think this will hide anything that it shouldn't hide? So I think it's still an improvement.
| // For in-scope completions, check if the simple name matches. | ||
| if builder.import.is_none() && self.existing_class_bases.contains(&builder.name) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Hmmm I'm not sure this is quite correct. e.g.,
import foo
class MyClass(foo.Bar, foo.B<CURSOR>): passThe completions returned on foo.B<CURSOR> won't have an import attached to them (because foo is already in scope). But the name on the completions won't be qualified. And indeed, the completions returned in that case won't have a module name associated with them at all. So you'll end up searching for Bar against foo.Bar and thus not find a match.
With that said, I don't think this will hide completions that it shouldn't. I think it just won't hide some completions that it should. So this is not really making anything worse.
I think fixing this particular issue is a bigger change so I'd be okay landing with this flaw personally.
There was a problem hiding this comment.
Documented with a test...
crates/ty_ide/src/completion.rs
Outdated
| // This handles cases like `class Foo(mod.Bar, ...)` where we want to | ||
| // filter out auto-import suggestions for `Bar` from module `mod`. | ||
| if let Some(module_name) = builder.module_name { | ||
| let qualified = Name::new(format!("{}.{}", module_name, builder.name)); |
There was a problem hiding this comment.
I think you can just use builder.qualified here.
| if builder.import.is_none() && self.existing_class_bases.contains(&builder.name) { | ||
| return true; | ||
| } | ||
| // For auto-import completions, check if the qualified name matches. |
There was a problem hiding this comment.
Yeah, I agree that this isn't quite either (see above in the non-auto-import case for why that isn't quite right either for similar reasons).
But I don't think this will hide anything that it shouldn't hide? So I think it's still an improvement.
charliermarsh
force-pushed
the
charlie/c
branch
from
130624a to
200ad7d
Compare
3 participants
Summary
Closes astral-sh/ty#2524.