Skip to content

Commit ffa824e

Browse files
authored
[red-knot] Add Type.definition method (#17153)
## Summary This is a follow up to the goto type definition PR. Specifically, that we want to avoid exposing too many semantic model internals publicly. I want to get some feedback on the approach taken. I think it goes into the right direction but I'm not super happy with it. The basic idea is that we add a `Type::definition` method which does the "goto type definition". The parts that I think make it awkward: * We can't directly return `Definition` because we don't create a `Definition` for modules (but we could?). Although I think it makes sense to possibly have a more public wrapper type anyway? * It doesn't handle unions and intersections. Mainly because not all elements in an intersection may have a definition and we only want to show a navigation target for intersections if there's only a single positive element (besides maybe `Unknown`). An alternative design or an addition to this design is to introduce a `SemanticAnalysis(Db)` struct that has methods like `type_definition(&self, type)` which explicitly exposes the methods we want. I don't feel comfortable design this API yet because it's unclear how fine granular it has to be (and if it is very fine granular, directly using `Type` might be better after all) ## Test Plan `cargo test`
1 parent 98b95c9 commit ffa824e

File tree

10 files changed

+286
-254
lines changed

10 files changed

+286
-254
lines changed

Cargo.lock

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/red_knot_ide/Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ ruff_python_parser = { workspace = true }
1717
ruff_text_size = { workspace = true }
1818
red_knot_python_semantic = { workspace = true }
1919

20+
rustc-hash = { workspace = true }
2021
salsa = { workspace = true }
2122
smallvec = { workspace = true }
2223
tracing = { workspace = true }

crates/red_knot_ide/src/goto.rs

+19-17
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@ pub fn goto_type_definition(
2424
ty.display(db.upcast())
2525
);
2626

27+
let navigation_targets = ty.navigation_targets(db);
28+
2729
Some(RangedValue {
2830
range: FileRange::new(file, goto_target.range()),
29-
value: ty.navigation_targets(db),
31+
value: navigation_targets,
3032
})
3133
}
3234

@@ -391,12 +393,12 @@ mod tests {
391393

392394
test.write_file("lib.py", "a = 10").unwrap();
393395

394-
assert_snapshot!(test.goto_type_definition(), @r###"
396+
assert_snapshot!(test.goto_type_definition(), @r"
395397
info: lint:goto-type-definition: Type definition
396398
--> /lib.py:1:1
397399
|
398400
1 | a = 10
399-
| ^
401+
| ^^^^^^
400402
|
401403
info: Source
402404
--> /main.py:4:13
@@ -406,7 +408,7 @@ mod tests {
406408
4 | lib
407409
| ^^^
408410
|
409-
"###);
411+
");
410412
}
411413

412414
#[test]
@@ -756,14 +758,13 @@ f(**kwargs<CURSOR>)
756758

757759
assert_snapshot!(test.goto_type_definition(), @r"
758760
info: lint:goto-type-definition: Type definition
759-
--> stdlib/builtins.pyi:443:7
761+
--> stdlib/types.pyi:677:11
760762
|
761-
441 | def __getitem__(self, key: int, /) -> str | int | None: ...
762-
442 |
763-
443 | class str(Sequence[str]):
764-
| ^^^
765-
444 | @overload
766-
445 | def __new__(cls, object: object = ...) -> Self: ...
763+
675 | if sys.version_info >= (3, 10):
764+
676 | @final
765+
677 | class NoneType:
766+
| ^^^^^^^^
767+
678 | def __bool__(self) -> Literal[False]: ...
767768
|
768769
info: Source
769770
--> /main.py:3:17
@@ -774,13 +775,14 @@ f(**kwargs<CURSOR>)
774775
|
775776
776777
info: lint:goto-type-definition: Type definition
777-
--> stdlib/types.pyi:677:11
778+
--> stdlib/builtins.pyi:443:7
778779
|
779-
675 | if sys.version_info >= (3, 10):
780-
676 | @final
781-
677 | class NoneType:
782-
| ^^^^^^^^
783-
678 | def __bool__(self) -> Literal[False]: ...
780+
441 | def __getitem__(self, key: int, /) -> str | int | None: ...
781+
442 |
782+
443 | class str(Sequence[str]):
783+
| ^^^
784+
444 | @overload
785+
445 | def __new__(cls, object: object = ...) -> Self: ...
784786
|
785787
info: Source
786788
--> /main.py:3:17

crates/red_knot_ide/src/lib.rs

+46-130
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,17 @@ mod goto;
44
mod hover;
55
mod markup;
66

7-
use std::ops::{Deref, DerefMut};
8-
97
pub use db::Db;
108
pub use goto::goto_type_definition;
119
pub use hover::hover;
1210
pub use markup::MarkupKind;
13-
use red_knot_python_semantic::types::{
14-
Class, ClassBase, ClassLiteralType, FunctionType, InstanceType, IntersectionType,
15-
KnownInstanceType, ModuleLiteralType, Type,
16-
};
11+
12+
use rustc_hash::FxHashSet;
13+
use std::ops::{Deref, DerefMut};
14+
15+
use red_knot_python_semantic::types::{Type, TypeDefinition};
1716
use ruff_db::files::{File, FileRange};
18-
use ruff_db::source::source_text;
19-
use ruff_text_size::{Ranged, TextLen, TextRange};
17+
use ruff_text_size::{Ranged, TextRange};
2018

2119
/// Information associated with a text range.
2220
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
@@ -58,7 +56,7 @@ where
5856
}
5957

6058
/// Target to which the editor can navigate to.
61-
#[derive(Debug, Clone)]
59+
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
6260
pub struct NavigationTarget {
6361
file: File,
6462

@@ -99,6 +97,17 @@ impl NavigationTargets {
9997
Self(smallvec::SmallVec::new())
10098
}
10199

100+
fn unique(targets: impl IntoIterator<Item = NavigationTarget>) -> Self {
101+
let unique: FxHashSet<_> = targets.into_iter().collect();
102+
if unique.is_empty() {
103+
Self::empty()
104+
} else {
105+
let mut targets = unique.into_iter().collect::<Vec<_>>();
106+
targets.sort_by_key(|target| (target.file, target.focus_range.start()));
107+
Self(targets.into())
108+
}
109+
}
110+
102111
fn iter(&self) -> std::slice::Iter<'_, NavigationTarget> {
103112
self.0.iter()
104113
}
@@ -129,7 +138,7 @@ impl<'a> IntoIterator for &'a NavigationTargets {
129138

130139
impl FromIterator<NavigationTarget> for NavigationTargets {
131140
fn from_iter<T: IntoIterator<Item = NavigationTarget>>(iter: T) -> Self {
132-
Self(iter.into_iter().collect())
141+
Self::unique(iter)
133142
}
134143
}
135144

@@ -140,143 +149,50 @@ pub trait HasNavigationTargets {
140149
impl HasNavigationTargets for Type<'_> {
141150
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
142151
match self {
143-
Type::BoundMethod(method) => method.function(db).navigation_targets(db),
144-
Type::FunctionLiteral(function) => function.navigation_targets(db),
145-
Type::ModuleLiteral(module) => module.navigation_targets(db),
146152
Type::Union(union) => union
147153
.iter(db.upcast())
148154
.flat_map(|target| target.navigation_targets(db))
149155
.collect(),
150-
Type::ClassLiteral(class) => class.navigation_targets(db),
151-
Type::Instance(instance) => instance.navigation_targets(db),
152-
Type::KnownInstance(instance) => instance.navigation_targets(db),
153-
Type::SubclassOf(subclass_of_type) => match subclass_of_type.subclass_of() {
154-
ClassBase::Class(class) => class.navigation_targets(db),
155-
ClassBase::Dynamic(_) => NavigationTargets::empty(),
156-
},
157156

158-
Type::StringLiteral(_)
159-
| Type::BooleanLiteral(_)
160-
| Type::LiteralString
161-
| Type::IntLiteral(_)
162-
| Type::BytesLiteral(_)
163-
| Type::SliceLiteral(_)
164-
| Type::MethodWrapper(_)
165-
| Type::WrapperDescriptor(_)
166-
| Type::PropertyInstance(_)
167-
| Type::Tuple(_) => self.to_meta_type(db.upcast()).navigation_targets(db),
168-
169-
Type::TypeVar(var) => {
170-
let definition = var.definition(db);
171-
let full_range = definition.full_range(db.upcast());
172-
173-
NavigationTargets::single(NavigationTarget {
174-
file: full_range.file(),
175-
focus_range: definition.focus_range(db.upcast()).range(),
176-
full_range: full_range.range(),
177-
})
157+
Type::Intersection(intersection) => {
158+
// Only consider the positive elements because the negative elements are mainly from narrowing constraints.
159+
let mut targets = intersection
160+
.iter_positive(db.upcast())
161+
.filter(|ty| !ty.is_unknown());
162+
163+
let Some(first) = targets.next() else {
164+
return NavigationTargets::empty();
165+
};
166+
167+
match targets.next() {
168+
Some(_) => {
169+
// If there are multiple types in the intersection, we can't navigate to a single one
170+
// because the type is the intersection of all those types.
171+
NavigationTargets::empty()
172+
}
173+
None => first.navigation_targets(db),
174+
}
178175
}
179176

180-
Type::Intersection(intersection) => intersection.navigation_targets(db),
181-
182-
Type::Dynamic(_)
183-
| Type::Never
184-
| Type::Callable(_)
185-
| Type::AlwaysTruthy
186-
| Type::AlwaysFalsy => NavigationTargets::empty(),
177+
ty => ty
178+
.definition(db.upcast())
179+
.map(|definition| definition.navigation_targets(db))
180+
.unwrap_or_else(NavigationTargets::empty),
187181
}
188182
}
189183
}
190184

191-
impl HasNavigationTargets for FunctionType<'_> {
192-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
193-
let function_range = self.focus_range(db.upcast());
194-
NavigationTargets::single(NavigationTarget {
195-
file: function_range.file(),
196-
focus_range: function_range.range(),
197-
full_range: self.full_range(db.upcast()).range(),
198-
})
199-
}
200-
}
201-
202-
impl HasNavigationTargets for Class<'_> {
185+
impl HasNavigationTargets for TypeDefinition<'_> {
203186
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
204-
let class_range = self.focus_range(db.upcast());
187+
let full_range = self.full_range(db.upcast());
205188
NavigationTargets::single(NavigationTarget {
206-
file: class_range.file(),
207-
focus_range: class_range.range(),
208-
full_range: self.full_range(db.upcast()).range(),
189+
file: full_range.file(),
190+
focus_range: self.focus_range(db.upcast()).unwrap_or(full_range).range(),
191+
full_range: full_range.range(),
209192
})
210193
}
211194
}
212195

213-
impl HasNavigationTargets for ClassLiteralType<'_> {
214-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
215-
self.class().navigation_targets(db)
216-
}
217-
}
218-
219-
impl HasNavigationTargets for InstanceType<'_> {
220-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
221-
self.class().navigation_targets(db)
222-
}
223-
}
224-
225-
impl HasNavigationTargets for ModuleLiteralType<'_> {
226-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
227-
let file = self.module(db).file();
228-
let source = source_text(db.upcast(), file);
229-
230-
NavigationTargets::single(NavigationTarget {
231-
file,
232-
focus_range: TextRange::default(),
233-
full_range: TextRange::up_to(source.text_len()),
234-
})
235-
}
236-
}
237-
238-
impl HasNavigationTargets for KnownInstanceType<'_> {
239-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
240-
match self {
241-
KnownInstanceType::TypeVar(var) => {
242-
let definition = var.definition(db);
243-
let full_range = definition.full_range(db.upcast());
244-
245-
NavigationTargets::single(NavigationTarget {
246-
file: full_range.file(),
247-
focus_range: definition.focus_range(db.upcast()).range(),
248-
full_range: full_range.range(),
249-
})
250-
}
251-
252-
// TODO: Track the definition of `KnownInstance` and navigate to their definition.
253-
_ => NavigationTargets::empty(),
254-
}
255-
}
256-
}
257-
258-
impl HasNavigationTargets for IntersectionType<'_> {
259-
fn navigation_targets(&self, db: &dyn Db) -> NavigationTargets {
260-
// Only consider the positive elements because the negative elements are mainly from narrowing constraints.
261-
let mut targets = self
262-
.iter_positive(db.upcast())
263-
.filter(|ty| !ty.is_unknown());
264-
265-
let Some(first) = targets.next() else {
266-
return NavigationTargets::empty();
267-
};
268-
269-
match targets.next() {
270-
Some(_) => {
271-
// If there are multiple types in the intersection, we can't navigate to a single one
272-
// because the type is the intersection of all those types.
273-
NavigationTargets::empty()
274-
}
275-
None => first.navigation_targets(db),
276-
}
277-
}
278-
}
279-
280196
#[cfg(test)]
281197
mod tests {
282198
use crate::db::tests::TestDb;

crates/red_knot_python_semantic/src/semantic_index/definition.rs

+6-10
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,13 @@ use crate::Db;
1313

1414
/// A definition of a symbol.
1515
///
16-
/// ## Module-local type
17-
/// This type should not be used as part of any cross-module API because
18-
/// it holds a reference to the AST node. Range-offset changes
19-
/// then propagate through all usages, and deserialization requires
20-
/// reparsing the entire module.
16+
/// ## ID stability
17+
/// The `Definition`'s ID is stable when the only field that change is its `kind` (AST node).
2118
///
22-
/// E.g. don't use this type in:
23-
///
24-
/// * a return type of a cross-module query
25-
/// * a field of a type that is a return type of a cross-module query
26-
/// * an argument of a cross-module query
19+
/// The `Definition` changes when the `file`, `scope`, or `symbol` change. This can be
20+
/// because a new scope gets inserted before the `Definition` or a new symbol is inserted
21+
/// before this `Definition`. However, the ID can be considered stable and it is okay to use
22+
/// `Definition` in cross-module` salsa queries or as a field on other salsa tracked structs.
2723
#[salsa::tracked(debug)]
2824
pub struct Definition<'db> {
2925
/// The file in which the definition occurs.

0 commit comments

Comments
 (0)