fix ParamSpecs should allow for overload resolution #2642#2665
fix ParamSpecs should allow for overload resolution #2642#2665asukaminato0721 wants to merge 1 commit intofacebook:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes ParamSpec-aware overload resolution when a Callable[P, ...] parameter is passed an overloaded function and additional *args/**kwargs are supplied at the call site (e.g. atexit.register(f, ...)), so overload selection uses the forwarded arguments before binding P.
Changes:
- Add a regression test covering
atexit.registerwith an overloaded callback and ParamSpec-forwarded arguments. - Add a special-case in
callable_infer_paramsto resolve an overloaded callable argument against the forwarded args, then bindPfrom the chosen overload’s parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pyrefly/lib/test/paramspec.rs |
Adds regression test for ParamSpec + overload selection through atexit.register. |
pyrefly/lib/alt/callable.rs |
Implements ParamSpec-aware overload selection/binding when the callable argument is overloaded. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| let (ret, chosen_sig) = self.call_overloads( | ||
| overloads, | ||
| &overload_for_call.metadata, | ||
| None, | ||
| &args[1..], | ||
| keywords, | ||
| arguments_range, | ||
| call_errors, | ||
| context, | ||
| None, | ||
| None, | ||
| ); |
There was a problem hiding this comment.
call_overloads always records an overload trace at arguments_range and can also emit NoMatchingOverload diagnostics. In this context we’re only using overload evaluation to bind the ParamSpec for the outer call, so recording traces / emitting errors for the inner overloaded function can produce incorrect IDE signature help and confusing error messages (it looks like the user “called” the overload). Consider using a temporary error collector (and ideally a path that doesn’t record overload traces), and only applying the result when a match is found.
| } | ||
| return; |
There was a problem hiding this comment.
This block returns unconditionally after calling call_overloads, even when ret.is_error() (no overload matched). That prevents the normal argument/ParamSpec binding logic from running and can leave only a NoMatchingOverload error for the passed-in function (or no error about the outer call argument) rather than a correctly-contextualized CallArgument/BadArgumentType diagnostic. Only early-return when overload resolution succeeds; otherwise fall through to the existing parameter matching logic.
| } | |
| return; | |
| return; | |
| } |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@yangdanny97 has imported this pull request. If you are a Meta employee, you can view this in D95416116. |
Summary
Fixes #2642
Added ParamSpec-aware overload resolution when a
Callable[P, ...]parameter is matched against an overloaded function with additional*args/**kwargs, so overload selection uses the call-site arguments before binding P. This fixes the premature overload selection inatexit.register.Test Plan
Added a regression test that passes a matching overload through atexit.register with ParamSpec args.