Skip to content

Commit e4b9b1d

Browse files
committed
[ty] Add builtins to completions derived from scope
Most of the work here was doing some light refactoring to facilitate sensible testing. That is, we don't want to list every builtin included in most tests, so we add some structure to the completion type returned. Tests can now filter based on whether a completion is a builtin or not. Otherwise, builtins are found using the existing infrastructure for `object.attr` completions (where we hard-code the module name `builtins`). I did consider changing the sort order based on whether a completion suggestion was a builtin or not. In particular, it seemed like it might be a good idea to sort builtins after other scope based completions, but before the dunder and sunder attributes. Namely, it seems likely that there is an inverse correlation between the size of a scope and the likelihood of an item in that scope being used at any given point. So it *might* be a good idea to prioritize the likelier candidates in the completions returned. Additionally, the number of items introduced by adding builtins is quite large. So I wondered whether mixing them in with everything else would become too noisy. However, it's not totally clear to me that this is the right thing to do. Right now, I feel like there is a very obvious lexicographic ordering that makes "finding" the right suggestion to activate potentially easier than if the ranking mechanism is less clear. (Technically, the dunder and sunder attributes are not sorted lexicographically, but I'd put forward that most folks don't have an intuitive understanding of where `_` ranks lexicographically with respect to "regular" letters. Moreover, since dunder and sunder attributes are all grouped together, I think the ordering here ends up being very obvious after even a quick glance.)
1 parent b1d1cf1 commit e4b9b1d

File tree

5 files changed

+122
-50
lines changed

5 files changed

+122
-50
lines changed

crates/ty_ide/src/completion.rs

Lines changed: 68 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,11 @@ use ruff_db::parsed::{ParsedModuleRef, parsed_module};
55
use ruff_python_ast as ast;
66
use ruff_python_parser::{Token, TokenAt, TokenKind};
77
use ruff_text_size::{Ranged, TextRange, TextSize};
8+
use ty_python_semantic::{Completion, SemanticModel};
89

910
use crate::Db;
1011
use crate::find_node::covering_node;
1112

12-
pub struct Completion {
13-
pub label: String,
14-
}
15-
1613
pub fn completion(db: &dyn Db, file: File, offset: TextSize) -> Vec<Completion> {
1714
let parsed = parsed_module(db.upcast(), file).load(db.upcast());
1815

@@ -23,18 +20,15 @@ pub fn completion(db: &dyn Db, file: File, offset: TextSize) -> Vec<Completion>
2320
return vec![];
2421
};
2522

26-
let model = ty_python_semantic::SemanticModel::new(db.upcast(), file);
23+
let model = SemanticModel::new(db.upcast(), file);
2724
let mut completions = match target {
2825
CompletionTargetAst::ObjectDot { expr } => model.attribute_completions(expr),
2926
CompletionTargetAst::ImportFrom { import, name } => model.import_completions(import, name),
3027
CompletionTargetAst::Scoped { node } => model.scoped_completions(node),
3128
};
32-
completions.sort_by(|name1, name2| compare_suggestions(name1, name2));
33-
completions.dedup();
29+
completions.sort_by(compare_suggestions);
30+
completions.dedup_by(|c1, c2| c1.name == c2.name);
3431
completions
35-
.into_iter()
36-
.map(|name| Completion { label: name.into() })
37-
.collect()
3832
}
3933

4034
/// The kind of tokens identified under the cursor.
@@ -330,7 +324,7 @@ fn import_from_tokens(tokens: &[Token]) -> Option<&Token> {
330324
///
331325
/// This has the effect of putting all dunder attributes after "normal"
332326
/// attributes, and all single-underscore attributes after dunder attributes.
333-
fn compare_suggestions(name1: &str, name2: &str) -> Ordering {
327+
fn compare_suggestions(c1: &Completion, c2: &Completion) -> Ordering {
334328
/// A helper type for sorting completions based only on name.
335329
///
336330
/// This sorts "normal" names first, then dunder names and finally
@@ -345,31 +339,32 @@ fn compare_suggestions(name1: &str, name2: &str) -> Ordering {
345339
}
346340

347341
impl Kind {
348-
fn classify(name: &str) -> Kind {
342+
fn classify(c: &Completion) -> Kind {
349343
// Dunder needs a prefix and suffix double underscore.
350344
// When there's only a prefix double underscore, this
351345
// results in explicit name mangling. We let that be
352346
// classified as-if they were single underscore names.
353347
//
354348
// Ref: <https://docs.python.org/3/reference/lexical_analysis.html#reserved-classes-of-identifiers>
355-
if name.starts_with("__") && name.ends_with("__") {
349+
if c.name.starts_with("__") && c.name.ends_with("__") {
356350
Kind::Dunder
357-
} else if name.starts_with('_') {
351+
} else if c.name.starts_with('_') {
358352
Kind::Sunder
359353
} else {
360354
Kind::Normal
361355
}
362356
}
363357
}
364358

365-
let (kind1, kind2) = (Kind::classify(name1), Kind::classify(name2));
366-
kind1.cmp(&kind2).then_with(|| name1.cmp(name2))
359+
let (kind1, kind2) = (Kind::classify(c1), Kind::classify(c2));
360+
kind1.cmp(&kind2).then_with(|| c1.name.cmp(&c2.name))
367361
}
368362

369363
#[cfg(test)]
370364
mod tests {
371365
use insta::assert_snapshot;
372366
use ruff_python_parser::{Mode, ParseOptions, TokenKind, Tokens};
367+
use ty_python_semantic::Completion;
373368

374369
use crate::completion;
375370
use crate::tests::{CursorTest, cursor_test};
@@ -463,7 +458,39 @@ mod tests {
463458
",
464459
);
465460

466-
assert_snapshot!(test.completions(), @"<No completions found>");
461+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
462+
}
463+
464+
#[test]
465+
fn builtins() {
466+
let test = cursor_test(
467+
"\
468+
<CURSOR>
469+
",
470+
);
471+
test.assert_completions_include("filter");
472+
}
473+
474+
#[test]
475+
fn builtins_not_included_object_attr() {
476+
let test = cursor_test(
477+
"\
478+
import re
479+
480+
re.<CURSOR>
481+
",
482+
);
483+
test.assert_completions_do_not_include("filter");
484+
}
485+
486+
#[test]
487+
fn builtins_not_included_import() {
488+
let test = cursor_test(
489+
"\
490+
from re import <CURSOR>
491+
",
492+
);
493+
test.assert_completions_do_not_include("filter");
467494
}
468495

469496
#[test]
@@ -828,7 +855,7 @@ def frob(): ...
828855
// TODO: it would be good if `bar` was included here, but
829856
// the list comprehension is not yet valid and so we do not
830857
// detect this as a definition of `bar`.
831-
assert_snapshot!(test.completions(), @"<No completions found>");
858+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
832859
}
833860

834861
#[test]
@@ -916,7 +943,7 @@ def frob(): ...
916943
//
917944
// The `lambda_blank1` test works because there are expressions
918945
// on either side of <CURSOR>.
919-
assert_snapshot!(test.completions(), @"<No completions found>");
946+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
920947
}
921948

922949
#[test]
@@ -928,7 +955,7 @@ def frob(): ...
928955
);
929956

930957
// FIXME: Should include `foo`.
931-
assert_snapshot!(test.completions(), @"<No completions found>");
958+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
932959
}
933960

934961
#[test]
@@ -940,7 +967,7 @@ def frob(): ...
940967
);
941968

942969
// FIXME: Should include `foo`.
943-
assert_snapshot!(test.completions(), @"<No completions found>");
970+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
944971
}
945972

946973
#[test]
@@ -1330,7 +1357,7 @@ hidden_<CURSOR>
13301357
",
13311358
);
13321359

1333-
assert_snapshot!(test.completions(), @"<No completions found>");
1360+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
13341361
}
13351362

13361363
#[test]
@@ -1738,7 +1765,7 @@ A.<CURSOR>
17381765
);
17391766

17401767
assert_snapshot!(
1741-
test.completions_if(|name| name.contains("FOO") || name.contains("foo")),
1768+
test.completions_if(|c| c.name.contains("FOO") || c.name.contains("foo")),
17421769
@r"
17431770
FOO
17441771
foo
@@ -1887,7 +1914,7 @@ except Type<CURSOR>:
18871914
",
18881915
);
18891916

1890-
assert_snapshot!(test.completions(), @"<No completions found>");
1917+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
18911918
}
18921919

18931920
// Ref: https://github.com/astral-sh/ty/issues/572
@@ -2226,25 +2253,32 @@ importlib.<CURSOR>
22262253
"#,
22272254
);
22282255

2229-
assert_snapshot!(test.completions(), @r"<No completions found>");
2256+
assert_snapshot!(test.completions(), @"<No completions found after filtering out 230 completions>");
22302257
}
22312258

22322259
impl CursorTest {
2260+
/// Returns all completions except for builtins.
22332261
fn completions(&self) -> String {
2234-
self.completions_if(|_| true)
2262+
self.completions_if(|c| !c.builtin)
22352263
}
22362264

2237-
fn completions_if(&self, predicate: impl Fn(&str) -> bool) -> String {
2265+
fn completions_if(&self, predicate: impl Fn(&Completion) -> bool) -> String {
22382266
let completions = completion(&self.db, self.cursor.file, self.cursor.offset);
22392267
if completions.is_empty() {
22402268
return "<No completions found>".to_string();
22412269
}
2242-
completions
2243-
.into_iter()
2244-
.map(|completion| completion.label)
2270+
let included = completions
2271+
.iter()
22452272
.filter(|label| predicate(label))
2246-
.collect::<Vec<String>>()
2247-
.join("\n")
2273+
.map(|completion| completion.name.as_str().to_string())
2274+
.collect::<Vec<String>>();
2275+
if included.is_empty() {
2276+
return format!(
2277+
"<No completions found after filtering out {} completions>",
2278+
completions.len(),
2279+
);
2280+
}
2281+
included.join("\n")
22482282
}
22492283

22502284
#[track_caller]
@@ -2254,7 +2288,7 @@ importlib.<CURSOR>
22542288
assert!(
22552289
completions
22562290
.iter()
2257-
.any(|completion| completion.label == expected),
2291+
.any(|completion| completion.name == expected),
22582292
"Expected completions to include `{expected}`"
22592293
);
22602294
}
@@ -2266,7 +2300,7 @@ importlib.<CURSOR>
22662300
assert!(
22672301
completions
22682302
.iter()
2269-
.all(|completion| completion.label != unexpected),
2303+
.all(|completion| completion.name != unexpected),
22702304
"Expected completions to not include `{unexpected}`",
22712305
);
22722306
}

crates/ty_python_semantic/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ pub use program::{
1515
PythonVersionWithSource, SearchPathSettings,
1616
};
1717
pub use python_platform::PythonPlatform;
18-
pub use semantic_model::{HasType, SemanticModel};
18+
pub use semantic_model::{Completion, HasType, SemanticModel};
1919
pub use site_packages::{PythonEnvironment, SitePackagesPaths, SysPrefixPathOrigin};
2020
pub use util::diagnostics::add_inferred_python_version_hint_to_diagnostic;
2121

crates/ty_python_semantic/src/semantic_model.rs

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ impl<'db> SemanticModel<'db> {
4646
&self,
4747
import: &ast::StmtImportFrom,
4848
_name: Option<usize>,
49-
) -> Vec<Name> {
49+
) -> Vec<Completion> {
5050
let module_name = match ModuleName::from_import_statement(self.db, self.file, import) {
5151
Ok(module_name) => module_name,
5252
Err(err) => {
@@ -58,26 +58,44 @@ impl<'db> SemanticModel<'db> {
5858
return vec![];
5959
}
6060
};
61-
let Some(module) = resolve_module(self.db, &module_name) else {
61+
self.module_completions(&module_name)
62+
}
63+
64+
/// Returns completions for symbols available in the given module as if
65+
/// it were imported by this model's `File`.
66+
fn module_completions(&self, module_name: &ModuleName) -> Vec<Completion> {
67+
let Some(module) = resolve_module(self.db, module_name) else {
6268
tracing::debug!("Could not resolve module from `{module_name:?}`");
6369
return vec![];
6470
};
6571
let ty = Type::module_literal(self.db, self.file, &module);
66-
crate::types::all_members(self.db, ty).into_iter().collect()
72+
crate::types::all_members(self.db, ty)
73+
.into_iter()
74+
.map(|name| Completion {
75+
name,
76+
builtin: module_name.as_str() == "builtins",
77+
})
78+
.collect()
6779
}
6880

6981
/// Returns completions for symbols available in a `object.<CURSOR>` context.
70-
pub fn attribute_completions(&self, node: &ast::ExprAttribute) -> Vec<Name> {
82+
pub fn attribute_completions(&self, node: &ast::ExprAttribute) -> Vec<Completion> {
7183
let ty = node.value.inferred_type(self);
72-
crate::types::all_members(self.db, ty).into_iter().collect()
84+
crate::types::all_members(self.db, ty)
85+
.into_iter()
86+
.map(|name| Completion {
87+
name,
88+
builtin: false,
89+
})
90+
.collect()
7391
}
7492

7593
/// Returns completions for symbols available in the scope containing the
7694
/// given expression.
7795
///
7896
/// If a scope could not be determined, then completions for the global
7997
/// scope of this model's `File` are returned.
80-
pub fn scoped_completions(&self, node: ast::AnyNodeRef<'_>) -> Vec<Name> {
98+
pub fn scoped_completions(&self, node: ast::AnyNodeRef<'_>) -> Vec<Completion> {
8199
let index = semantic_index(self.db, self.file);
82100

83101
// TODO: We currently use `try_expression_scope_id` here as a hotfix for [1].
@@ -96,17 +114,37 @@ impl<'db> SemanticModel<'db> {
96114
}) else {
97115
return vec![];
98116
};
99-
let mut symbols = vec![];
117+
let mut completions = vec![];
100118
for (file_scope, _) in index.ancestor_scopes(file_scope) {
101-
symbols.extend(all_declarations_and_bindings(
102-
self.db,
103-
file_scope.to_scope_id(self.db, self.file),
104-
));
119+
completions.extend(
120+
all_declarations_and_bindings(self.db, file_scope.to_scope_id(self.db, self.file))
121+
.map(|name| Completion {
122+
name,
123+
builtin: false,
124+
}),
125+
);
105126
}
106-
symbols
127+
// Builtins are available in all scopes.
128+
let builtins = ModuleName::new("builtins").expect("valid module name");
129+
completions.extend(self.module_completions(&builtins));
130+
completions
107131
}
108132
}
109133

134+
/// A suggestion for code completion.
135+
#[derive(Clone, Debug)]
136+
pub struct Completion {
137+
/// The label shown to the user for this suggestion.
138+
pub name: Name,
139+
/// Whether this suggestion came from builtins or not.
140+
///
141+
/// At time of writing (2025-06-26), this information
142+
/// doesn't make it into the LSP response. Instead, we
143+
/// use it mainly in tests so that we can write less
144+
/// noisy tests.
145+
pub builtin: bool,
146+
}
147+
110148
pub trait HasType {
111149
/// Returns the inferred type of `self`.
112150
///

crates/ty_server/src/server/api/requests/completion.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ impl BackgroundDocumentRequestHandler for CompletionRequestHandler {
5656
.into_iter()
5757
.enumerate()
5858
.map(|(i, comp)| CompletionItem {
59-
label: comp.label,
59+
label: comp.name.into(),
6060
sort_text: Some(format!("{i:-max_index_len$}")),
6161
..Default::default()
6262
})

crates/ty_wasm/src/lib.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ impl Workspace {
309309
Ok(completions
310310
.into_iter()
311311
.map(|completion| Completion {
312-
label: completion.label,
312+
name: completion.name.into(),
313313
})
314314
.collect())
315315
}
@@ -619,7 +619,7 @@ pub struct Hover {
619619
#[derive(Debug, Clone, PartialEq, Eq)]
620620
pub struct Completion {
621621
#[wasm_bindgen(getter_with_clone)]
622-
pub label: String,
622+
pub name: String,
623623
}
624624

625625
#[wasm_bindgen]

0 commit comments

Comments
 (0)