fix Odd warning on shutil.rmtree and ExitStack.callback #2105#2340
fix Odd warning on shutil.rmtree and ExitStack.callback #2105#2340asukaminato0721 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
Fixes pyrefly’s ParamSpec inference when forwarding arguments through callback-style helpers (e.g., ExitStack.callback), ensuring the selected callable overload matches the forwarded arguments (including for bound __call__ callables), addressing issue #2105.
Changes:
- Update ParamSpec inference in call analysis to prefer the callable overload that matches forwarded
*args/**kwargs. - Add a regression test covering
ExitStack.callback(shutil.rmtree, ..., ignore_errors=True).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pyrefly/lib/alt/callable.rs |
Adds logic to select an overload for a forwarded callable to constrain the ParamSpec based on forwarded arguments. |
pyrefly/lib/test/paramspec.rs |
Adds a regression test reproducing the ExitStack.callback(shutil.rmtree, ..., ignore_errors=True) scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let call_errors_local = self.error_collector(); | ||
| let _ = self.callable_infer( | ||
| sig.clone(), | ||
| None, | ||
| None, |
There was a problem hiding this comment.
Overload selection here uses a “first signature with zero call errors wins” approach by iterating sigs and breaking on the first match. This can diverge from the project’s overload call evaluation logic (see pyrefly/lib/alt/overload.rs, which applies additional disambiguation steps when multiple overloads match), and could pin the ParamSpec to the wrong parameter list when multiple signatures accept the forwarded args. Consider reusing the existing overload-selection machinery (or at least its disambiguation heuristic) to choose the same overload that a normal call would select, before constraining paramspec_var.
| if sigs.len() > 1 { | ||
| let forwarded_args = args.get(arg_index + 1..).unwrap_or(&[]); | ||
| let mut selected = None; | ||
| for sig in sigs { | ||
| let call_errors_local = self.error_collector(); |
There was a problem hiding this comment.
This loop potentially calls callable_infer once per candidate signature, which re-infers forwarded_args/keywords each time. In hot paths with large overload sets this can become noticeably expensive. If possible, precompute argument types once (similar to CallWithTypes in call_overloads) and/or delegate to call_overloads to benefit from its argument-flattening and matching heuristics.
rchen152
left a comment
There was a problem hiding this comment.
At least conceptually, this seems like another flavor of #105 - we're matching the paramspec to the first overload rather than considering all of them.
I'd be okay with putting in a workaround to address this specific bug, but this PR makes me a bit nervous because (1) it's doing some fiddly tracking of parameters and arguments by idx, and (2) it's doing its own overload evaluation, using different logic from call_overloads.
As a more general question, I wonder how pervasive/urgent this issue is. Maybe if we wait until we've addressed some of our problems with type parameter solving more holistically, a better solution will become more obvious.
1d8ef30 to
b3c067a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: tornado (https://github.com/tornadoweb/tornado)
+ ERROR tornado/gen.py:255:62-69: Argument `Awaitable[Unknown] | Future[Unknown] | dict[Any, Awaitable[Unknown]] | list[Awaitable[Unknown]] | _T | None` is not assignable to parameter `first_yielded` with type `Awaitable[Unknown] | Future[Unknown] | dict[Any, Awaitable[Unknown]] | list[Awaitable[Unknown]] | None` in function `Runner.__init__` [bad-argument-type]
pip (https://github.com/pypa/pip)
- WARN iterative_resolve_scc: SCC CalcId(pip._vendor.msgpack.fallback, /home/runner/work/pyrefly/pyrefly/primer_base/projects/pip/src/pip/_vendor/msgpack/fallback.py, KeyClassField(class1, _unpack)) exceeded 5 iterations; committing last answers
starlette (https://github.com/encode/starlette)
+ ERROR starlette/testclient.py:112:28-39: Argument `Overload[
+ [PosArgsT, T_Retval](self: BlockingPortal, func: (**tuple[*PosArgsT]) -> Awaitable[T_Retval], *args: *PosArgsT) -> T_Retval
+ [PosArgsT, T_Retval](self: BlockingPortal, func: (**tuple[*PosArgsT]) -> T_Retval, *args: *PosArgsT) -> T_Retval
+ ]` is not assignable to parameter with type `(func: (**tuple[*PosArgsT]) -> Awaitable[T_Retval], *args: *PosArgsT) -> @_` in function `contextlib._BaseExitStack.callback` [bad-argument-type]
cloud-init (https://github.com/canonical/cloud-init)
+ WARN iterative_resolve_scc: SCC CalcId(cloudinit.sources.helpers.aliyun, /home/runner/work/pyrefly/pyrefly/primer_base/projects/cloud-init/cloudinit/sources/helpers/aliyun.py, Key::Definition(_process_dict_values 85:9-29)) exceeded 5 iterations; committing last answers
pandera (https://github.com/pandera-dev/pandera)
+ WARN iterative_resolve_scc: SCC CalcId(typing_inspect, /home/runner/work/pyrefly/pyrefly/primer_base/projects/_pandera_venv/lib/python3.13/site-packages/typing_inspect.py, Key::Definition(_union_subs_tree 779:5-21)) exceeded 5 iterations; committing last answers
|
Primer Diff Classification❌ 1 regression(s) | 1 project(s) total 1 regression(s) across tornado. error kinds:
Detailed analysis❌ Regression (1)tornado (+1)
Suggested FixSummary: The ParamSpec inference changes in callable_infer() are causing incorrect type inference for generator yield types, confusing return types with yield types. 1. In
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (1 LLM) |
Summary
Fixes #2105
Fixed ParamSpec inference for forwarded callbacks to select the overload that matches the forwarded arguments, even when the callback is a bound
__call__(protocol/class instance).Test Plan
Added a regression test for ExitStack.callback(shutil.rmtree, ..., ignore_errors=True).