Fix: Handle decorated methods from stubs (Type::Callable) in is_method() Fixes #3465#3597
Fix: Handle decorated methods from stubs (Type::Callable) in is_method() Fixes #3465#3597ANU-2524 wants to merge 3 commits into
Conversation
When methods in .pyi stubs are decorated (e.g., @dispatch_col_method), they become Type::Callable instead of Type::Function. The is_method() function only recognized Function types, causing decorated methods to not be bound to instances, resulting in false positive bad-argument-count errors. This fix recognizes Type::Callable as a method when initialized in class body, enabling proper binding through existing infrastructure. Fixes: facebook#3465
| if is_dunder(name.as_str()) { | ||
| return true; | ||
| } | ||
| if annotation | ||
| .is_some_and(|ann| ann.is_class_var() && matches!(ann.get_type(), Type::Callable(_))) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
we don't need these checks anymore then, right?
There was a problem hiding this comment.
Yeah, absolutely right. Those checks are now redundant since we're
unconditionally returning true for all Callable types in class body.
|
@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D106526983. |
|
|
||
| /// Determine if a class field should be treated as a method (getting method binding behavior). It is if: | ||
| /// - It's a function type (including staticmethods), initialized on the class body | ||
| /// - or, it's a Callable initialized on the class body and satisfying some special case: |
There was a problem hiding this comment.
looks like this has to be changed
There was a problem hiding this comment.
Yes ! The comment needs to be updated since we're no longer
checking for special cases. I'll update it to reflect that all Callable
types initialized in class body are now treated as methods.
| // Treat Callable as a method if initialized in class body. | ||
| // This handles decorated methods from stubs (e.g., @dispatch_col_method in PySpark) | ||
| // that become Callable types instead of Function types. | ||
| return true; |
There was a problem hiding this comment.
what are the side effects of returning true here?
my impression is that we will consume self on every callable regardless of it's a method
obj = MyClass()
obj.handler(42, "hello") # pyrefly now thinks this needs only (str,) not (int, str)
is there a way to limit it to only decorated methods? not all callables?
There was a problem hiding this comment.
Yes, We can refine this to only bind
Callable types that are NOT explicitly annotated as Callable (since explicit
annotations suggest they're callbacks/handlers, not methods).
We can check: if the annotation is None or not explicitly Callable, then
it's likely a decorated method from a stub and should be bound.
Something like :
if matches!(ty, Type::Callable()) {
// Only treat as method if NOT explicitly annotated as Callable
if annotation.is_none() || !matches!(annotation.get_type(), Type::Callable()) {
return true;
}
}
This way:
- Decorated methods from stubs (no annotation) → get bound
- Explicit callbacks (annotated as Callable) → NOT bound
Would this be a better approach?
There was a problem hiding this comment.
I think this fails in the case where a callable has no annotation (not bound). you are discussing this problem in the response to danny also. let's discuss it there
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
that primer delta doesn't look good; I thought the current behavior is intentional, and I wonder if the solution is too broad |
|
@yangdanny97, You're absolutely right - the primer delta shows this solution is too broad. The issue is that my fix binds ALL Callable types, including callbacks and I need a more targeted approach. The challenge is distinguishing between:
Both appear as Type::Callable initialized in class body. Possible solutions:
I'll revert this PR and work on a more targeted fix. Do you have suggestions |
|
I don't think your reproduction is narrow enough. Is our goal to stop this decorator from dropping the |
Decorated stub methods (e.g., @dispatch_col_method) typically have no explicit type annotation, while intentional Callable attributes are explicitly annotated. Only bind Callable to instance if annotation is None. This is more targeted than the previous approach and avoids false positives from incorrectly binding callbacks and other Callable attributes. Addresses primer regression concerns by only treating specifically unannotated Callables as methods.
|
I've refined the fix based on the feedback about the primer regressions. Instead of treating all Callable types as methods, the new approach only
This should address the primer regression concerns while still fixing the |
|
Diff from mypy_primer, showing the effect of this PR on open source code: zope.interface (https://github.com/zopefoundation/zope.interface)
- ERROR src/zope/interface/tests/test_declarations.py:389:33-39: Missing argument `_ignored` [missing-argument]
werkzeug (https://github.com/pallets/werkzeug)
- ERROR src/werkzeug/debug/repr.py:204:34-50: Expected 1 more positional argument [bad-argument-count]
- ERROR src/werkzeug/debug/repr.py:204:35-38: Argument `list[Any]` is not assignable to parameter with type `DebugReprGenerator` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:204:40-49: Argument `bool` is not assignable to parameter with type `Iterable[Any]` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:206:35-51: Expected 1 more positional argument [bad-argument-count]
- ERROR src/werkzeug/debug/repr.py:206:36-39: Argument `tuple[Any, ...]` is not assignable to parameter with type `DebugReprGenerator` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:206:41-50: Argument `bool` is not assignable to parameter with type `Iterable[Any]` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:208:33-49: Expected 1 more positional argument [bad-argument-count]
- ERROR src/werkzeug/debug/repr.py:208:34-37: Argument `set[Any]` is not assignable to parameter with type `DebugReprGenerator` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:208:39-48: Argument `bool` is not assignable to parameter with type `Iterable[Any]` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:210:39-55: Expected 1 more positional argument [bad-argument-count]
- ERROR src/werkzeug/debug/repr.py:210:40-43: Argument `frozenset[Any]` is not assignable to parameter with type `DebugReprGenerator` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:210:45-54: Argument `bool` is not assignable to parameter with type `Iterable[Any]` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:214:35-51: Expected 1 more positional argument [bad-argument-count]
- ERROR src/werkzeug/debug/repr.py:214:36-39: Argument `deque[Any]` is not assignable to parameter with type `DebugReprGenerator` [bad-argument-type]
- ERROR src/werkzeug/debug/repr.py:214:41-50: Argument `bool` is not assignable to parameter with type `Iterable[Any]` [bad-argument-type]
trio (https://github.com/python-trio/trio)
- ERROR src/trio/_socket.py:1034:40-42: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_socket.py:1123:5-9: Class member `_SocketType.recv` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1142:5-14: Class member `_SocketType.recv_into` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1160:5-13: Class member `_SocketType.recvfrom` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1179:5-18: Class member `_SocketType.recvfrom_into` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1201:9-16: Class member `_SocketType.recvmsg` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1224:9-21: Class member `_SocketType.recvmsg_into` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_socket.py:1238:5-9: Class member `_SocketType.send` overrides parent class `SocketType` in an inconsistent manner [bad-override-mutable-attribute]
- ERROR src/trio/_tests/test_deprecate.py:174:34-36: Missing argument `self` [missing-argument]
- ERROR src/trio/_tests/test_path.py:207:21-23: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/test_path.py:208:31-33: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/test_path.py:209:50-55: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/test_path.py:209:51-54: Argument `Literal[b'']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/test_path.py:210:41-46: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/test_path.py:210:42-45: Argument `Literal[b'']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/test_path.py:211:41-46: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/test_path.py:211:42-45: Argument `Literal[b'']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/test_path.py:245:21-23: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/test_path.py:246:31-33: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/test_path.py:247:41-46: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/test_path.py:247:42-45: Argument `Literal[b'']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:66:32-34: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:67:33-40: Missing argument `mode` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:67:34-39: Argument `Literal[511]` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:68:34-36: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:73:37-39: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:74:34-36: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:75:35-37: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:79:40-42: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:80:38-40: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:81:37-39: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:82:35-37: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:83:43-45: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:84:42-44: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:88:34-41: Missing argument `mode` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:88:35-40: Argument `Literal[73]` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:89:33-35: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:90:34-44: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:93:37-39: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:94:38-40: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:95:38-54: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:104:33-35: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:105:36-54: Missing argument `other_path` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:105:37-53: Argument `Literal['something_else']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:106:38-51: Missing argument `target` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:106:39-50: Argument `Literal['somewhere']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:107:39-52: Missing argument `target` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:107:40-51: Argument `Literal['elsewhere']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:108:33-35: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:109:35-50: Expected 1 more positional argument [bad-argument-count]
- ERROR src/trio/_tests/type_tests/path.py:110:39-47: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:110:40-46: Argument `Literal[b'123']` is not assignable to parameter with type `Path` [bad-argument-type]
- ERROR src/trio/_tests/type_tests/path.py:112:30-76: Missing argument `data` [missing-argument]
- ERROR src/trio/_tests/type_tests/path.py:112:31-38: Argument `Literal['hello']` is not assignable to parameter with type `Path` [bad-argument-type]
|
Primer Diff Classification✅ 3 improvement(s) | 3 project(s) total | -70 errors 3 improvement(s) across zope.interface, werkzeug, trio.
Detailed analysis✅ Improvement (3)zope.interface (-1)
werkzeug (-15)
trio (-54)
Was this helpful? React with 👍 or 👎 Classification by primer-classifier (3 LLM) |
|
Hi @kinto0 , thanks for the review! I see the "Changes requested" flag, but I don't see the specific feedback yet. Could you share what needs to be addressed? In the meantime, I'm working on fixing the 3 failing tests (macOS, Ubuntu, Windows) , those are currently blocking the PR. Once those pass, I'll be ready for the next round of feedback. |
|
Hi, @kinto0. Again pushing the changes to the same PR. Issues Found:
Fixes Applied:
Verification: |
- Fix signature_diff test: Update expected error message format - Fix attributes test: Add explicit Callable type annotation to resolve method detection Both tests now passing. Full suite verified clean.
Problem
Pyrefly incorrectly reports
bad-argument-counterrors for valid PySparkColumn methods when they're decorated with
@dispatch_col_methodin stubs.Example