fix Types from the typing module are currently not clickable in type hints #1681 #1682
Conversation
asukaminato0721
force-pushed
the
1681
branch
from
26236bf to
3b1acd1
Compare
|
Thanks for taking a look at this! I have been looking at this as well as #1683 and I am wondering if there is one solution that we can use for both of these. It seems like to get tuples working we are including the entire stdlib along with TypeOutput which seems like a large lift for just that purpose. I am wondering if we could also combine that logic with what we do here somehow. What do you think? |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1681 by making types from the typing module clickable in LSP inlay hints. The implementation adds metadata tracking for type symbols (module/name pairs) through a new TypeSymbolReference structure and enriches TypeLabelPart records to carry both rendered text and optional symbol references. The LSP layer resolves these symbols on-demand to their source locations in typing.pyi or typing_extensions.pyi, enabling IDE features like go-to-definition for types like Literal, Any, and other typing constructs.
Key Changes:
- Introduced
TypeSymbolReferenceandTypeLabelPartstructures to track symbol metadata alongside display text - Added
write_symbolmethod toTypeOutputtrait for emitting symbols with module/name information - Implemented
resolve_type_symbol_referencein LSP transaction to dynamically resolve typing module symbols to their stub file locations - Updated display logic to use
maybe_fmt_with_moduleforSpecialFormtypes, enabling clickability for Literal, TypeGuard, etc. - Refactored test infrastructure with helper functions for structured inlay hint validation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/test/lsp/lsp_interaction/util.rs | Added helper functions inlay_hints_match_expected, ExpectedInlayHint, and ExpectedTextEdit for structured test assertions |
| pyrefly/lib/test/lsp/lsp_interaction/notebook_inlay_hint.rs | Migrated from JSON-based test expectations to structured helper functions |
| pyrefly/lib/test/lsp/lsp_interaction/inlay_hint.rs | Migrated tests to structured helpers and added test_inlay_hint_typing_literals_have_locations to verify Literal symbols resolve to typing.pyi |
| pyrefly/lib/lsp/wasm/inlay_hints.rs | Added resolve_type_symbol_reference method with re-export following and integrated symbol resolution into return-type and variable-type hint builders |
| crates/pyrefly_types/src/type_output.rs | Introduced TypeSymbolReference and TypeLabelPart, added write_symbol to trait, updated all implementations and tests |
| crates/pyrefly_types/src/display.rs | Modified maybe_fmt_with_module to use write_symbol, updated SpecialForm handling, attempted TypedDict changes (contains bugs), updated test helpers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &self, | ||
| handle: &Handle, | ||
| symbol: &TypeSymbolReference, | ||
| ) -> Option<TextRangeWithModule> { |
There was a problem hiding this comment.
The constant MAX_LOOKUP_DEPTH is set to 8, which seems arbitrary. Consider documenting why this specific depth limit was chosen, or if there's a risk of infinite loops due to circular module imports, explain how this bound protects against that.
| ) -> Option<TextRangeWithModule> { | |
| ) -> Option<TextRangeWithModule> { | |
| // Limit the number of module re-export traversals to prevent infinite loops | |
| // or excessive resource usage in the case of circular or deeply nested re-exports. | |
| // The value 8 is a conservative default and can be adjusted if necessary. |
| for (part, expected_value) in parts.iter().zip(expected_hint.labels.iter()) { | ||
| if part.value != *expected_value { | ||
| return false; | ||
| } | ||
| if *expected_value == "Literal" && part.location.is_none() { | ||
| return false; |
There was a problem hiding this comment.
This hardcoded check for "Literal" is fragile and will fail if the expected value changes or if other typing symbols need to be verified. Consider making this more generic by checking if any part has both a value matching a known typing symbol and a non-null location pointing to typing.pyi.
| for (part, expected_value) in parts.iter().zip(expected_hint.labels.iter()) { | |
| if part.value != *expected_value { | |
| return false; | |
| } | |
| if *expected_value == "Literal" && part.location.is_none() { | |
| return false; | |
| // List of known typing symbols that should have a location pointing to typing.pyi | |
| const KNOWN_TYPING_SYMBOLS: &[&str] = &["Literal", "Final", "ClassVar", "Optional", "Union", "Any", "Callable", "Type", "NoReturn", "Tuple", "TypedDict", "Protocol"]; | |
| for (part, expected_value) in parts.iter().zip(expected_hint.labels.iter()) { | |
| if part.value != *expected_value { | |
| return false; | |
| } | |
| if KNOWN_TYPING_SYMBOLS.contains(expected_value) { | |
| match &part.location { | |
| Some(loc) => { | |
| // Check if the file path ends with "typing.pyi" | |
| if !loc.uri.to_string().ends_with("typing.pyi") { | |
| return false; | |
| } | |
| } | |
| None => { | |
| return false; | |
| } | |
| } |
| match name { | ||
| Cow::Borrowed(s) => self.formatter.write_str(s), | ||
| Cow::Owned(s) => self.formatter.write_str(&s), | ||
| } |
There was a problem hiding this comment.
[nitpick] The match expression handles both Cow::Borrowed and Cow::Owned identically by calling write_str with a reference to the string. This can be simplified to just self.formatter.write_str(&name) since Cow implements Deref to the borrowed type.
| match name { | |
| Cow::Borrowed(s) => self.formatter.write_str(s), | |
| Cow::Owned(s) => self.formatter.write_str(&s), | |
| } | |
| self.formatter.write_str(&name) |
| match &name { | ||
| Cow::Borrowed(s) => (*s).to_owned(), | ||
| Cow::Owned(s) => s.clone(), | ||
| } |
There was a problem hiding this comment.
[nitpick] Similar to the DisplayOutput implementation, this match expression can be simplified. When display_module is false, you can use name.into_owned() directly instead of matching on Cow::Borrowed vs Cow::Owned since both paths ultimately need an owned String for the text field.
| match &name { | |
| Cow::Borrowed(s) => (*s).to_owned(), | |
| Cow::Owned(s) => s.clone(), | |
| } | |
| name.into_owned() |
2 participants
Summary
Fixes #1681
Reworked the type-label plumbing so OutputWithLocations now emits TypeLabelPart records that carry both the rendered text and optional TypeSymbolReference metadata (module/name) whenever we render known symbols (e.g., via maybe_fmt_with_module). This involved extending the TypeOutput trait with write_symbol and updating all implementations/tests accordingly.
In the LSP transaction, added resolve_type_symbol_reference to import typing/typing_extensions modules on demand and attach their source locations to inlay-hint label parts when the original type string lacked a location. Both return-type and variable-type hint builders now fall back to this resolver before emitting each label part.
Test Plan
Strengthened coverage with a new end-to-end LSP test that asserts Literal label parts point into typing.pyi, alongside the existing goto-type-definition test. The display-layer tests were also updated to exercise the richer label parts and verify Literal parts carry typing metadata.