fix sqlalchemy func.current_timestamp missing doc #2889#2938
fix sqlalchemy func.current_timestamp missing doc #2889#2938asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Improves LSP hover documentation so that when hovering a symbol whose definition lacks a docstring (e.g., a @property returning Type[SomeClass]), the hover can fall back to showing the docstring of the referenced class object type—addressing the missing-doc scenario in issue #2889.
Changes:
- Add a hover fallback to retrieve docstrings from class-object types when the resolved symbol has no docstring.
- Add an LSP hover regression test covering
@property -> Type[current_timestamp]docstring fallback.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pyrefly/lib/test/lsp/hover.rs |
Adds a regression test ensuring hover falls back to the returned class’s docstring for a property returning Type[T]. |
pyrefly/lib/lsp/wasm/hover.rs |
Implements fallback docstring lookup for class-object types when the primary definition has no docstring. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let definition_handle = Handle::new( | ||
| qname.module_name(), | ||
| qname.module_path().dupe(), | ||
| handle.sys_info().dupe(), | ||
| ); | ||
| let definition = transaction | ||
| .find_definition( | ||
| &definition_handle, | ||
| qname.range().start(), | ||
| FindPreference { | ||
| resolve_call_dunders: false, | ||
| ..Default::default() | ||
| }, | ||
| ) | ||
| .into_iter() | ||
| .find(|item| item.definition_range == qname.range())?; | ||
| Some(Docstring(definition.docstring_range?, definition.module)) | ||
| } |
There was a problem hiding this comment.
docstring_for_class_object_type relies on transaction.find_definition, which hard-depends on get_ast(handle) being present. ASTs can be evicted after computing answers (ModuleState::evict_ast stores None), so this fallback can silently stop working for many library modules even if their exports/types are available.
To make this robust, avoid the find_definition path here (or re-parse the module when get_ast is None). A practical alternative is to fetch the class symbol’s docstring_range directly from the target module’s exports (and follow ExportLocation::OtherModule if needed), which doesn’t require retaining the AST. Also note FindPreference::default() prefers .pyi; for docstrings you likely want to prefer executable .py when available.
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary
Fixes #2889
hover now falls back to the docstring of the class object represented by the hover type when the resolved symbol definition itself has no docstring. That covers the
@property -> Type[current_timestamp]case from the issue.Test Plan
add test