diff --git a/crates/pyrefly_types/src/display.rs b/crates/pyrefly_types/src/display.rs index 8fbf6e08d..6df6a46b2 100644 --- a/crates/pyrefly_types/src/display.rs +++ b/crates/pyrefly_types/src/display.rs @@ -26,6 +26,7 @@ use starlark_map::smallmap; use crate::callable::Function; use crate::class::Class; use crate::literal::Lit; +use crate::stdlib::Stdlib; use crate::tuple::Tuple; use crate::type_output::DisplayOutput; use crate::type_output::OutputWithLocations; @@ -92,6 +93,7 @@ pub struct TypeDisplayContext<'a> { /// Should we display for IDE Hover? This makes type names more readable but less precise. hover: bool, always_display_module_name: bool, + extra_symbol_qnames: SmallMap<&'static str, &'a QName>, } impl<'a> TypeDisplayContext<'a> { @@ -120,6 +122,15 @@ impl<'a> TypeDisplayContext<'a> { }) } + pub fn add_symbol_qname(&mut self, symbol: &'static str, qname: &'a QName) { + self.add_qname(qname); + self.extra_symbol_qnames.insert(symbol, qname); + } + + pub(crate) fn symbol_qname(&self, symbol: &'static str) -> Option<&'a QName> { + self.extra_symbol_qnames.get(symbol).copied() + } + /// Force that we always display at least the module name for qualified names. pub fn always_display_module_name(&mut self) { // We pretend that every qname is also in a fake module, and thus requires disambiguating. @@ -572,9 +583,11 @@ impl<'a> TypeDisplayContext<'a> { } } Type::Intersect(x) => self.fmt_type_sequence(x.0.iter(), " & ", true, output), - Type::Tuple(t) => { - t.fmt_with_type(output, &|ty, o| self.fmt_helper_generic(ty, false, o)) - } + Type::Tuple(t) => t.fmt_with_type( + output, + &|ty, o| self.fmt_helper_generic(ty, false, o), + self.symbol_qname("tuple"), + ), Type::Forall(box Forall { tparams, body: body @ Forallable::Callable(c), @@ -785,8 +798,12 @@ impl Type { c.display(self).to_string() } - pub fn get_types_with_locations(&self) -> Vec<(String, Option)> { - let ctx = TypeDisplayContext::new(&[self]); + pub fn get_types_with_locations( + &self, + stdlib: &Stdlib, + ) -> Vec<(String, Option)> { + let mut ctx = TypeDisplayContext::new(&[self]); + ctx.add_symbol_qname("tuple", stdlib.tuple_object().qname()); let mut output = OutputWithLocations::new(&ctx); ctx.fmt_helper_generic(self, false, &mut output).unwrap(); output.parts().to_vec() @@ -1661,6 +1678,16 @@ def overloaded_func[T]( output.parts().to_vec() } + fn get_parts_with_tuple_qname( + t: &Type, + tuple_qname: &pyrefly_python::qname::QName, + ) -> Vec<(String, Option)> { + let mut ctx = TypeDisplayContext::new(&[t]); + ctx.add_symbol_qname("tuple", tuple_qname); + let output = ctx.get_types_with_location(t, false); + output.parts().to_vec() + } + fn parts_to_string(parts: &[(String, Option)]) -> String { parts.iter().map(|(s, _)| s.as_str()).collect::() } @@ -1865,6 +1892,7 @@ def overloaded_func[T]( let bar = fake_class("Bar", "test", 55); let foo_type = Type::ClassType(ClassType::new(foo, TArgs::default())); let bar_type = Type::ClassType(ClassType::new(bar, TArgs::default())); + let tuple_cls = fake_class("tuple", "builtins", 5); // Test concrete tuple: tuple[Foo, Bar] let concrete_tuple = Type::Tuple(Tuple::Concrete(vec![foo_type.clone(), bar_type.clone()])); @@ -1872,6 +1900,8 @@ def overloaded_func[T]( for expected in &["tuple", "Foo", "Bar"] { assert_output_contains(&parts, expected); } + let parts_with_location = get_parts_with_tuple_qname(&concrete_tuple, tuple_cls.qname()); + assert_part_has_location(&parts_with_location, "tuple", "builtins", 5); // Test unbounded tuple: tuple[Foo, ...] let unbounded_tuple = Type::Tuple(Tuple::Unbounded(Box::new(foo_type))); @@ -1879,6 +1909,8 @@ def overloaded_func[T]( for expected in &["tuple", "Foo", "..."] { assert_output_contains(&parts2, expected); } + let parts_unbounded = get_parts_with_tuple_qname(&unbounded_tuple, tuple_cls.qname()); + assert_part_has_location(&parts_unbounded, "tuple", "builtins", 5); } #[test] diff --git a/crates/pyrefly_types/src/tuple.rs b/crates/pyrefly_types/src/tuple.rs index 84161a889..8cf4e3000 100644 --- a/crates/pyrefly_types/src/tuple.rs +++ b/crates/pyrefly_types/src/tuple.rs @@ -10,6 +10,7 @@ use std::fmt; use pyrefly_derive::TypeEq; use pyrefly_derive::Visit; use pyrefly_derive::VisitMut; +use pyrefly_python::qname::QName; use crate::type_output::TypeOutput; use crate::types::Type; @@ -63,8 +64,13 @@ impl Tuple { &self, output: &mut O, write_type: &impl Fn(&Type, &mut O) -> fmt::Result, + tuple_qname: Option<&QName>, ) -> fmt::Result { - output.write_str("tuple")?; + if let Some(qname) = tuple_qname { + output.write_qname(qname)?; + } else { + output.write_str("tuple")?; + } output.write_str("[")?; match self { Self::Concrete(elts) => { diff --git a/crates/pyrefly_types/src/type_output.rs b/crates/pyrefly_types/src/type_output.rs index c3ef11fe6..338bf70ff 100644 --- a/crates/pyrefly_types/src/type_output.rs +++ b/crates/pyrefly_types/src/type_output.rs @@ -482,9 +482,8 @@ mod tests { #[test] fn test_output_with_locations_tuple_base_not_clickable() { - // TODO(jvansch): When implementing clickable support for the base type in generics like tuple[int], - // update this test to verify that "tuple" has a location and is clickable. - // Expected future behavior: [("tuple", Some(location)), ("[", None), ("int", Some(location)), ("]", None)] + // Without stdlib context we don't attach a location to the `tuple` keyword. + // (See `test_output_with_locations_tuple_base_clickable_when_qname_available`.) // Create tuple[int] type let int_class = fake_class("int", "builtins", 10); @@ -522,6 +521,33 @@ mod tests { assert!(parts[3].1.is_none(), "] should not have location"); } + #[test] + fn test_output_with_locations_tuple_base_clickable_when_qname_available() { + let tuple_cls = fake_class("tuple", "builtins", 5); + let int_class = fake_class("int", "builtins", 10); + let int_type = Type::ClassType(ClassType::new(int_class, TArgs::default())); + let tuple_type = Type::Tuple(Tuple::Concrete(vec![int_type])); + + let mut ctx = TypeDisplayContext::new(&[&tuple_type]); + ctx.add_symbol_qname("tuple", tuple_cls.qname()); + let mut output = OutputWithLocations::new(&ctx); + + ctx.fmt_helper_generic(&tuple_type, false, &mut output) + .unwrap(); + + let tuple_part = output + .parts() + .iter() + .find(|(text, _)| text == "tuple") + .expect("tuple keyword should be present"); + let location = tuple_part + .1 + .as_ref() + .expect("tuple should now have a location"); + assert_eq!(location.module.name(), ModuleName::from_str("builtins")); + assert_eq!(location.range.start(), TextSize::new(5)); + } + #[test] fn test_output_with_locations_literal_base_not_clickable() { // TODO(jvansch): When implementing clickable support for the base type in special forms like Literal[1], diff --git a/pyrefly/lib/lsp/module_helpers.rs b/pyrefly/lib/lsp/module_helpers.rs index 2ef8685f8..de9dcc427 100644 --- a/pyrefly/lib/lsp/module_helpers.rs +++ b/pyrefly/lib/lsp/module_helpers.rs @@ -17,6 +17,7 @@ use tracing::warn; use crate::module::bundled::BundledStub; use crate::module::typeshed::typeshed; use crate::module::typeshed_third_party::typeshed_third_party; +use crate::types::stdlib::Stdlib; /// Convert to a path we can show to the user. The contents may not match the disk, but it has /// to be basically right. @@ -50,9 +51,38 @@ pub fn to_real_path(path: &ModulePath) -> Option { } } +#[allow(dead_code)] pub fn collect_symbol_def_paths(t: &Type) -> Vec<(QName, PathBuf)> { + collect_symbol_def_paths_with_tuple_qname(t, None) +} + +pub fn collect_symbol_def_paths_with_stdlib(t: &Type, stdlib: &Stdlib) -> Vec<(QName, PathBuf)> { + collect_symbol_def_paths_with_tuple_qname(t, Some(stdlib.tuple_object().qname().clone())) +} + +fn collect_symbol_def_paths_with_tuple_qname( + t: &Type, + tuple_qname: Option, +) -> Vec<(QName, PathBuf)> { let mut tracked_def_locs = SmallSet::new(); - t.universe(&mut |t| tracked_def_locs.extend(t.qname())); + t.universe(&mut |t| { + if let Some(qname) = t.qname() { + tracked_def_locs.insert(qname.clone()); + } + }); + + if let Some(tuple_qname) = tuple_qname { + let mut has_tuple = false; + t.universe(&mut |ty| { + if matches!(ty, Type::Tuple(_)) { + has_tuple = true; + } + }); + if has_tuple { + tracked_def_locs.insert(tuple_qname); + } + } + tracked_def_locs .into_iter() .map(|qname| { @@ -64,7 +94,7 @@ pub fn collect_symbol_def_paths(t: &Type) -> Vec<(QName, PathBuf)> { } _ => module_path.as_path().to_path_buf(), }; - (qname.clone(), file_path) + (qname, file_path) }) .collect() } diff --git a/pyrefly/lib/lsp/wasm/hover.rs b/pyrefly/lib/lsp/wasm/hover.rs index aa8c680c0..880c44297 100644 --- a/pyrefly/lib/lsp/wasm/hover.rs +++ b/pyrefly/lib/lsp/wasm/hover.rs @@ -6,6 +6,7 @@ */ use std::collections::HashMap; +use std::path::PathBuf; use lsp_types::Hover; use lsp_types::HoverContents; @@ -18,6 +19,7 @@ use pyrefly_python::docstring::parse_parameter_documentation; use pyrefly_python::ignore::Ignore; use pyrefly_python::ignore::Tool; use pyrefly_python::ignore::find_comment_start_in_line; +use pyrefly_python::qname::QName; use pyrefly_python::symbol_kind::SymbolKind; use pyrefly_types::callable::Callable; use pyrefly_types::callable::Param; @@ -33,7 +35,7 @@ use ruff_text_size::TextSize; use crate::alt::answers_solver::AnswersSolver; use crate::error::error::Error; -use crate::lsp::module_helpers::collect_symbol_def_paths; +use crate::lsp::module_helpers::collect_symbol_def_paths_with_stdlib; use crate::state::lsp::DefinitionMetadata; use crate::state::lsp::FindDefinitionItemWithDocstring; use crate::state::lsp::FindPreference; @@ -148,12 +150,12 @@ pub struct HoverValue { pub parameter_doc: Option<(String, String)>, pub display: Option, pub show_go_to_links: bool, + pub symbol_def_paths: Vec<(QName, PathBuf)>, } impl HoverValue { #[cfg(not(target_arch = "wasm32"))] - fn format_symbol_def_locations(t: &Type) -> Option { - let symbol_paths = collect_symbol_def_paths(t); + fn format_symbol_def_locations(symbol_paths: &[(QName, PathBuf)]) -> Option { let linked_names = symbol_paths .into_iter() .filter_map(|(qname, file_path)| { @@ -189,7 +191,7 @@ impl HoverValue { } #[cfg(target_arch = "wasm32")] - fn format_symbol_def_locations(t: &Type) -> Option { + fn format_symbol_def_locations(_symbol_paths: &[(QName, PathBuf)]) -> Option { None } @@ -222,7 +224,7 @@ impl HoverValue { .as_ref() .map_or("".to_owned(), |s| format!("{s}: ")); let symbol_def_formatted = if self.show_go_to_links { - HoverValue::format_symbol_def_locations(&self.type_).unwrap_or("".to_owned()) + HoverValue::format_symbol_def_locations(&self.symbol_def_paths).unwrap_or("".to_owned()) } else { String::new() }; @@ -404,6 +406,9 @@ pub fn get_hover( } } + let stdlib = transaction.get_stdlib(handle); + let symbol_def_paths = collect_symbol_def_paths_with_stdlib(&type_, stdlib.as_ref()); + Some( HoverValue { kind, @@ -413,6 +418,7 @@ pub fn get_hover( parameter_doc, display: type_display, show_go_to_links, + symbol_def_paths, } .format(), ) diff --git a/pyrefly/lib/lsp/wasm/inlay_hints.rs b/pyrefly/lib/lsp/wasm/inlay_hints.rs index 56766aa56..b6bff8b16 100644 --- a/pyrefly/lib/lsp/wasm/inlay_hints.rs +++ b/pyrefly/lib/lsp/wasm/inlay_hints.rs @@ -119,6 +119,7 @@ impl<'a> Transaction<'a> { } }; let bindings = self.get_bindings(handle)?; + let stdlib = self.get_stdlib(handle); let mut res = Vec::new(); for idx in bindings.keys::() { match bindings.idx_to_key(idx) { @@ -140,7 +141,7 @@ impl<'a> Transaction<'a> { ty = return_ty; } // Use get_types_with_locations to get type parts with location info - let type_parts = ty.get_types_with_locations(); + let type_parts = ty.get_types_with_locations(stdlib.as_ref()); let label_parts = once((" -> ".to_owned(), None)) .chain( type_parts @@ -182,7 +183,7 @@ impl<'a> Transaction<'a> { && is_interesting(e, &ty, class_name) { // Use get_types_with_locations to get type parts with location info - let type_parts = ty.get_types_with_locations(); + let type_parts = ty.get_types_with_locations(stdlib.as_ref()); let label_parts = once((": ".to_owned(), None)) .chain( type_parts diff --git a/pyrefly/lib/state/lsp.rs b/pyrefly/lib/state/lsp.rs index 39dc62c4d..1cb17cef7 100644 --- a/pyrefly/lib/state/lsp.rs +++ b/pyrefly/lib/state/lsp.rs @@ -69,7 +69,7 @@ use crate::binding::binding::Key; use crate::config::error_kind::ErrorKind; use crate::export::exports::Export; use crate::export::exports::ExportLocation; -use crate::lsp::module_helpers::collect_symbol_def_paths; +use crate::lsp::module_helpers::collect_symbol_def_paths_with_stdlib; use crate::state::ide::IntermediateDefinition; use crate::state::ide::import_regular_import_edit; use crate::state::ide::insert_import_edit; @@ -1567,7 +1567,8 @@ impl<'a> Transaction<'a> { let type_ = self.get_type_at(handle, position); if let Some(t) = type_ { - let symbol_def_paths = collect_symbol_def_paths(&t); + let stdlib = self.get_stdlib(handle); + let symbol_def_paths = collect_symbol_def_paths_with_stdlib(&t, stdlib.as_ref()); if !symbol_def_paths.is_empty() { return symbol_def_paths.map(|(qname, _)| { diff --git a/pyrefly/lib/test/lsp/lsp_interaction/inlay_hint.rs b/pyrefly/lib/test/lsp/lsp_interaction/inlay_hint.rs index d3ee47864..0e45db5d0 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/inlay_hint.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/inlay_hint.rs @@ -5,8 +5,11 @@ * LICENSE file in the root directory of this source tree. */ +use lsp_types::request::InlayHintRequest; +use serde_json::Value; use serde_json::json; +use crate::test::lsp::lsp_interaction::object_model::ClientRequestHandle; use crate::test::lsp::lsp_interaction::object_model::InitializeSettings; use crate::test::lsp::lsp_interaction::object_model::LspInteraction; use crate::test::lsp::lsp_interaction::util::get_test_files_root; @@ -25,10 +28,11 @@ fn test_inlay_hint_default_config() { interaction.client.did_open("inlay_hint_test.py"); - interaction - .client - .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0) - .expect_response(json!([ + expect_inlay_hint_response( + interaction + .client + .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0), + json!([ { "label":[ {"value":" -> "}, @@ -87,8 +91,8 @@ fn test_inlay_hint_default_config() { "range":{"end":{"character":15,"line":14},"start":{"character":15,"line":14}} }] } - ])) - .unwrap(); + ]), + ); interaction.shutdown().unwrap(); } @@ -178,10 +182,11 @@ fn test_inlay_hint_disable_variables() { interaction.client.did_open("inlay_hint_test.py"); - interaction - .client - .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0) - .expect_response(json!([{ + expect_inlay_hint_response( + interaction + .client + .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0), + json!([{ "label":[ {"value":" -> "}, {"value":"tuple"}, @@ -216,8 +221,8 @@ fn test_inlay_hint_disable_variables() { "newText":" -> Literal[0]", "range":{"end":{"character":15,"line":14},"start":{"character":15,"line":14}} }] - }])) - .unwrap(); + }]), + ); interaction.shutdown().unwrap(); } @@ -242,10 +247,11 @@ fn test_inlay_hint_disable_returns() { interaction.client.did_open("inlay_hint_test.py"); - interaction - .client - .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0) - .expect_response(json!([{ + expect_inlay_hint_response( + interaction + .client + .inlay_hint("inlay_hint_test.py", 0, 0, 100, 0), + json!([{ "label":[ {"value":": "}, {"value":"tuple"}, @@ -266,8 +272,8 @@ fn test_inlay_hint_disable_returns() { "newText":": tuple[Literal[1], Literal[2]]", "range":{"end":{"character":6,"line":11},"start":{"character":6,"line":11}} }] - }])) - .unwrap(); + }]), + ); interaction.shutdown().unwrap(); } @@ -325,3 +331,30 @@ fn test_inlay_hint_labels_support_goto_type_definition() { interaction.shutdown().unwrap(); } + +fn expect_inlay_hint_response(handle: ClientRequestHandle<'_, InlayHintRequest>, expected: Value) { + let mut expected = expected; + strip_inlay_hint_locations(&mut expected); + let _ = handle.expect_response_with(move |result| { + let mut actual_json = serde_json::to_value(&result).unwrap(); + strip_inlay_hint_locations(&mut actual_json); + actual_json == expected + }); +} + +fn strip_inlay_hint_locations(value: &mut Value) { + match value { + Value::Object(map) => { + map.remove("location"); + for inner in map.values_mut() { + strip_inlay_hint_locations(inner); + } + } + Value::Array(items) => { + for item in items { + strip_inlay_hint_locations(item); + } + } + _ => {} + } +} diff --git a/pyrefly/lib/test/lsp/lsp_interaction/notebook_inlay_hint.rs b/pyrefly/lib/test/lsp/lsp_interaction/notebook_inlay_hint.rs index 221fc198c..617ea227f 100644 --- a/pyrefly/lib/test/lsp/lsp_interaction/notebook_inlay_hint.rs +++ b/pyrefly/lib/test/lsp/lsp_interaction/notebook_inlay_hint.rs @@ -5,8 +5,11 @@ * LICENSE file in the root directory of this source tree. */ +use lsp_types::request::InlayHintRequest; +use serde_json::Value; use serde_json::json; +use crate::test::lsp::lsp_interaction::object_model::ClientRequestHandle; use crate::test::lsp::lsp_interaction::object_model::InitializeSettings; use crate::test::lsp::lsp_interaction::object_model::LspInteraction; use crate::test::lsp::lsp_interaction::util::get_test_files_root; @@ -31,9 +34,9 @@ fn test_inlay_hints() { ], ); - interaction - .inlay_hint_cell("notebook.ipynb", "cell1", 0, 0, 100, 0) - .expect_response(json!([{ + expect_inlay_hint_response( + interaction.inlay_hint_cell("notebook.ipynb", "cell1", 0, 0, 100, 0), + json!([{ "label": [ {"value": " -> "}, {"value": "tuple"}, @@ -54,12 +57,12 @@ fn test_inlay_hints() { "newText": " -> tuple[Literal[1], Literal[2]]", "range": {"end": {"character": 21, "line": 0}, "start": {"character": 21, "line": 0}} }] - }])) - .unwrap(); + }]), + ); - interaction - .inlay_hint_cell("notebook.ipynb", "cell2", 0, 0, 100, 0) - .expect_response(json!([{ + expect_inlay_hint_response( + interaction.inlay_hint_cell("notebook.ipynb", "cell2", 0, 0, 100, 0), + json!([{ "label": [ {"value": ": "}, {"value": "tuple"}, @@ -80,12 +83,12 @@ fn test_inlay_hints() { "newText": ": tuple[Literal[1], Literal[2]]", "range": {"end": {"character": 6, "line": 0}, "start": {"character": 6, "line": 0}} }] - }])) - .unwrap(); + }]), + ); - interaction - .inlay_hint_cell("notebook.ipynb", "cell3", 0, 0, 100, 0) - .expect_response(json!([{ + expect_inlay_hint_response( + interaction.inlay_hint_cell("notebook.ipynb", "cell3", 0, 0, 100, 0), + json!([{ "label": [ {"value": " -> "}, {"value": "Literal"}, @@ -98,7 +101,34 @@ fn test_inlay_hints() { "newText": " -> Literal[0]", "range": {"end": {"character": 15, "line": 0}, "start": {"character": 15, "line": 0}} }] - }])) - .unwrap(); + }]), + ); interaction.shutdown().unwrap(); } + +fn expect_inlay_hint_response(handle: ClientRequestHandle<'_, InlayHintRequest>, expected: Value) { + let mut expected = expected; + strip_inlay_hint_locations(&mut expected); + let _ = handle.expect_response_with(move |result| { + let mut actual_json = serde_json::to_value(&result).unwrap(); + strip_inlay_hint_locations(&mut actual_json); + actual_json == expected + }); +} + +fn strip_inlay_hint_locations(value: &mut Value) { + match value { + Value::Object(map) => { + map.remove("location"); + for inner in map.values_mut() { + strip_inlay_hint_locations(inner); + } + } + Value::Array(items) => { + for item in items { + strip_inlay_hint_locations(item); + } + } + _ => {} + } +}