-
Notifications
You must be signed in to change notification settings - Fork 213
fix numpy function hover missing name #1418 #1618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
improve #1416
|
|
fix #1348
|
kinto0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! A few questions
it's unclear why these names are missing from your summary. are all of these bugs the same situation?
do we need all of these fallbacks? which ones are necessary in which situations?
I would like to see all this logic localized to one section. can this entire PR be a change to pyrefly/crates/pyrefly_types/src/display.rs?
|
Resolver didn’t keep a textual name when the definition expression was empty (e.g., stub attributes, keyword arguments, or type-only nodes). Fix: FindDefinitionItemWithDocstring now always carries a display_name, so when we do have definition metadata, we reuse that string. Type-only lookup path – when we fail to find any definition (third-party binary stubs), we still need something readable. That’s why we fall back to metadata from Type::Function/Callable/BoundMethod/Forall. Literal identifier under the cursor – in the degenerate case where neither of the above provides anything (e.g., a failing resolver on a local scratch buffer), we lex the identifier so we can at least echo the token. |
1ae78da to
ab31d30
Compare
593a75b to
d15191b
Compare
kinto0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still a few unresolved comments about API changes: this diff adds a few layers of indirection. can we instead have as_hover_string take a fallback name and contain the logic there?
d15191b to
9066433
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes issue #1418 where numpy function hovers were missing function names. The fix implements a comprehensive multi-layered fallback mechanism to ensure hover tooltips always display identifiable names, even for third-party symbols defined in stub files.
Key Changes:
- Added
display_namefield toFindDefinitionItemWithDocstringto thread identifier names through definition lookups - Implemented multi-tier fallback for hover names: definition snippet → metadata name → type-derived name → lexical identifier
- Enhanced callable type rendering with
def name(...)syntax for improved readability and syntax highlighting
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyrefly/lib/test/lsp/lsp_interaction/hover.rs | Added integration test verifying third-party function names appear in hover tooltips |
| pyrefly/lib/test/lsp/hover.rs | Updated test expectation to include def __matmul__( syntax in method hover display |
| pyrefly/lib/state/lsp.rs | Added display_name field to FindDefinitionItemWithDocstring struct and populated it in all instantiation sites; updated destructuring patterns to handle new field |
| pyrefly/lib/lsp/wasm/hover.rs | Implemented fallback mechanisms (fallback_hover_name_from_type, identifier_text_at) and integrated multi-layered name resolution into hover logic; added unit tests |
| pyrefly/lib/lsp/non_wasm/server.rs | Updated destructuring patterns to ignore new display_name field with .. |
| crates/pyrefly_types/src/display.rs | Added as_hover_string_with_fallback_name to format callable types with def name(...) syntax when name is provided |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


fix #1418
Added metadata-backed hover names by threading a display_name through every FindDefinitionItemWithDocstring so hover headers can still show identifiers even when we land in stub files or empty ranges.
Updated the hover formatter to try definition snippets, the metadata name, a type-derived fallback, and finally a lexical identifier; this restored headers for third-party symbols and ensured both WASM and native LSP paths behave identically
Introduced rich function rendering: callable tooltips now show (function) name plus a syntactically valid def name(...): ..., improving syntax highlighting in VS Code for cases like np.array.