Repository navigation
Conversation
|
✅ This PR meets all requirements from our pull request checklist. Optional recommendations:
Note: this comment updates automatically after changes to the PR. Updates may take up to half an hour. |
Libraries that predate type annotations often declare parameter types in their docstrings, such as `:param flask.Flask|flask.Blueprint app:`. Add a parser that extracts these declarations from `:param <type> <name>:` and `:type <name>: <type>` fields, with the byte offset of each type so that it can later be mapped back to a range in the source file. The `:type` field wins when a parameter has both.
Using the types declared in docstrings changes the inferred type of every unannotated parameter that has one, which can surface new errors in existing projects. Gate that behavior on an opt-in option, set in the config file or with `--infer-param-types-from-docstring`, and make it available to the binding step. It does nothing yet.
Older libraries often document parameter types in Sphinx fields instead of annotating them, so Pyrefly treats those parameters as `Any`. When `infer-param-types-from-docstring` is on, the type from a `:param <type> <name>:` or `:type <name>: <type>` field becomes the type of an unannotated parameter. For `__init__`, the class docstring is read too, since that is where Sphinx documents constructor parameters. A docstring type is only a hint. It is bound in the scope of the function's annotations, but it is never a declared type, is not checked against the default value, and is dropped without an error when it cannot be understood, for example prose like `list of str` or an unknown name. A parameter that defaults to `None` becomes optional.
061b477 to
a1afca1
Compare
|
This pull request has been imported. If you are a Meta employee, you can view this in D129028362. (Because this pull request was imported automatically, there will not be any future comments.) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
rchen152
left a comment
There was a problem hiding this comment.
Thanks for the PR! From the typing side, this looks very good; I found one minor typing issue (see in-line comment). IMO it doesn't necessarily need to be addressed before merging, but it looks like a relatively simple fix.
How this interacts with the language server is where things get a little hairy. I asked claude to test a number of lsp scenarios by using a mini hand-written client, and it seems like with the new flag turned on, find-references and renames will see/update docstring types across files but not within the same file. I don't have a strong preference for whether we fix this by always excluding docstring types or always including them, but we should be consistent. I'll include claude's notes below on how to implement either approach.
As a side note, thank you for organizing this into easy-to-review commits :)
AI notes on an LSP fix:
Option 1: Ignore docstring types in IDE navigation
- Record the range of each docstring type expression on
Bindings.- Add a
docstring_type_ranges: Vec<TextRange>field next toannotated_metadata_ranges/unused_imports(pyrefly/lib/binding/bindings.rs:238) and onBindingsBuilder(:340). - Initialize it in both constructors (
:468,:861), move it across inBindings::new(:1018), and add anis_in_docstring_type(range)accessor next tounused_imports()(:557). - Push
expr.range()afterensure_typeinbind_docstring_param_types(pyrefly/lib/binding/function.rs:429).
- Add a
- Skip those ranges when building the index of names defined in other modules (
pyrefly/lib/alt/answers.rs:1453), by adding&& !bindings.is_in_docstring_type(reference_range)to the existing check on the reference text. - Return early from
record_attribute_definition_index(pyrefly/lib/alt/answers.rs:1767) whenattribute_reference_rangeis in a docstring type. This covers attribute accesses likem.Foo, whichalt/expr.rs:2765records when the docstring annotation is solved.
The syntax-tree scans for same-module references and for the star-import fix already ignore docstrings, so nothing else changes. One difference remains: lookup_name (pyrefly/lib/binding/bindings.rs:2029, :2064) still counts a docstring name as a use of its import. I'd keep that, because otherwise removing an "unused" import would silently break the docstring type.
Option 2: Include docstring types everywhere
- Same-module references to a name: in
local_variable_references_from_local_definition, after the syntax-tree walk (pyrefly/lib/state/lsp.rs:4971), also run the visitor over everyBindingAnnotation::DocstringParamexpression in the bindings. Theis_valid_usecheck works unchanged, becauseensure_typealready created name-use keys for these names. - Same-module references to an attribute: do the same in
local_attribute_references_from_local_definition(pyrefly/lib/state/lsp.rs:4794). This also needs a recorded type for the expression before the dot (get_type_trace(attribute.value.range()),:4805), and I'm not sure solving the docstring annotation records one. - Star-import conversion: also visit docstring expressions in
collect_star_imported_names(pyrefly/lib/state/lsp/quick_fixes/convert_star_import.rs:159), so names that only docstrings use are kept. - Starting from inside a docstring:
identifier_at(pyrefly/lib/state/lsp.rs:1079) only scans the syntax tree (:1274). So rename, find-references, hover, and go-to-definition do nothing when the cursor is in a docstring. Full consistency would mean also checking the docstring expressions there.
Unused-import tracking already counts docstring uses, so it's consistent with this option as is.
| { | ||
| continue; | ||
| } | ||
| self.ensure_type(&mut expr, None); |
There was a problem hiding this comment.
The second argument to ensure_type is a collector for legacy type variables that makes them resolve correctly in signatures. Passing in None means that if a docstring references a legacy type variable, we'll infer the type as TypeVar[T] (essentially, the definition of the legacy type variable) rather than T (the type parameter).
Concretely, with the new flag:
from typing import TypeVar, reveal_type
T = TypeVar("T")
def ident(x):
"""
:param T x: the x
"""
return x
ident(1) # error!A possible fix is to bind docstring types inside function_header, with the same LegacyTParamCollector as real annotations.
Summary
Older libraries often document parameter types in Sphinx fields such as
:param flask.Flask|flask.Blueprint app:instead of annotating them, so Pyrefly treats those parameters asAny.This adds an opt-in
infer-param-types-from-docstringoption (also--infer-param-types-from-docstring, defaultfalse). When it is on, the type from a:param <type> <name>:or:type <name>: <type>field becomes the type of an unannotated parameter.:typewins if a parameter has both. For__init__the class docstring is read as well, since that is where Sphinx documents constructor parameters, and the method's own docstring takes priority.A docstring type is only a hint:
NonebecomesT | None.list of str, unknown names, and types that are not valid for a parameter.Google and NumPy docstrings are not handled here and can be added on top of this.
Fixes #1384
Test Plan
Added type checker tests in
pyrefly/lib/test/docstring_param_types.rs(off by default,:paramand:type,Nonedefaults, unions and attributes, local classes,__init__with a class docstring, annotations winning, and types that are ignored), parser tests indocstring.rs, and a config test. Thepyrefly,pyrefly_pythonandpyrefly_configtest suites pass, except forcheck_writes_cinderx_and_glean_reports_togetheranddeep_phi_chain_term_expr, which also fail on an unmodifiedmainin my environment. Formatting and lint, conformance, and the jsonschema check are clean.