fix sqlalchemy func.current_timestamp missing doc #2889#2938
fix sqlalchemy func.current_timestamp missing doc #2889#2938asukaminato0721 wants to merge 1 commit into
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.
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