diff --git a/pyrefly/lib/alt/class/class_field.rs b/pyrefly/lib/alt/class/class_field.rs index 1d2d0dd357..f9ce836025 100644 --- a/pyrefly/lib/alt/class/class_field.rs +++ b/pyrefly/lib/alt/class/class_field.rs @@ -1285,9 +1285,8 @@ fn has_any_abstract(ty: &Type) -> bool { /// 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: -/// - it's marked as a ClassVar -/// - it's assigned to a dunder name like `__add__` +/// - or, it's a Callable initialized on the class body, unless it carries an explicit +/// non-`ClassVar` annotation (see the Callable handling below) /// - or, it's a union where ANY element satisfies the above rules /// /// Note: staticmethods and union types that have at least one method type are included. @@ -1310,16 +1309,22 @@ fn is_method( return true; } - // Special cases where Callable is assumed to be a method + // A `Callable` stored on the class body behaves like a method (binding `self`), mirroring + // the runtime, where a plain function object is a descriptor. This matches mypy & pyright: + // - an inferred assignment (no explicit annotation) binds `self`, so factory-produced + // methods like `isNull = _unary_op(...)` are usable without passing `self`; + // - an explicit `x: Callable[...]` annotation is a data attribute, with no binding; + // - an explicit `x: ClassVar[Callable[...]]` is treated as a method again; + // - a dunder name (e.g. `__add__`) is always a method regardless of annotation. + // See https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 if matches!(ty, Type::Callable(_)) { 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; - } + return match annotation { + None => true, + Some(ann) => ann.is_class_var() && matches!(ann.get_type(), Type::Callable(_)), + }; } false diff --git a/pyrefly/lib/error/signature_diff.rs b/pyrefly/lib/error/signature_diff.rs index 6113dea731..92ce833e67 100644 --- a/pyrefly/lib/error/signature_diff.rs +++ b/pyrefly/lib/error/signature_diff.rs @@ -431,16 +431,16 @@ class B(A): ); assert_eq!(messages.len(), 1, "Expected one error, got {messages:?}"); let expected = r#"Class member `B.foo` overrides parent class `A` in an inconsistent manner - `B.foo` has type `(self: Unknown) -> None`, which is not consistent with `(self: B, x: int) -> int` in `A.foo` (the type of read-write attributes cannot be changed) + `B.foo` has type `(self: Unknown) -> None`, which is not assignable to `(self: B, x: int) -> int`, the type of `A.foo` Signature mismatch: expected: def foo(self: B, x: int) -> int: ... ^^^^^^^^^ ^^^ return type | parameters - found: (self: Unknown) -> None - ^^^^^^^ ^^^^ return type - | - parameters"#; + found: def foo(self: Unknown) -> None: ... + ^^^^^^^ ^^^^ return type + | + parameters"#; assert_eq!(messages[0], expected); } diff --git a/pyrefly/lib/test/attributes.rs b/pyrefly/lib/test/attributes.rs index 1435fc99f4..f8908b1a42 100644 --- a/pyrefly/lib/test/attributes.rs +++ b/pyrefly/lib/test/attributes.rs @@ -560,27 +560,43 @@ assert_type(C().f(1), int) "#, ); -// Mypy and Pyright treat `f` as not a method here; its actual behavior -// is ambiguous even if we assume the values are always functions or lambdas -// because the default value can be overridden by instance assignment. -// -// Our behavior is compatible, but the underlying implementation is not, we are -// behaving this way based on how we treat the Callable type rather than based -// on the absence of `ClassVar`. +// A `Callable` assigned in the class body without an explicit annotation binds `self`, +// matching mypy & pyright: instance access drops the first parameter, class access does not, +// and the (read-only) method cannot be shadowed by an instance assignment. // // See https://discuss.python.org/t/when-should-we-assume-callable-types-are-method-descriptors/92938 testcase!( - test_callable_with_ambiguous_binding, + test_callable_inferred_binding, r#" from typing import assert_type, Callable def get_callback() -> Callable[[object, int], int]: ... class C: f = get_callback() assert_type(C.f(None, 1), int) -assert_type(C().f(None, 1), int) -# This is why the behavior is ambiguous - at runtime, the default `C.f` is a -# method but the instance-level shadow is not. -C().f = lambda _, x: x +assert_type(C().f(1), int) +C().f = get_callback() # E: not assignable to attribute `f` +"#, +); + +// Regression test for https://github.com/facebook/pyrefly/issues/3465: methods produced by a +// factory helper (as PySpark's `pyspark.sql.Column` does for `isNull`, `asc`, etc.) are +// `Callable`-typed class attributes and must bind `self` when called on an instance. +testcase!( + test_callable_factory_method_binding, + r#" +from typing import assert_type, Callable +def _unary_op() -> Callable[["Column"], "Column"]: + def _(self: "Column") -> "Column": ... + return _ +def _bin_op() -> Callable[["Column", "Column"], "Column"]: + def _(self: "Column", other: "Column") -> "Column": ... + return _ +class Column: + isNull = _unary_op() + eqNullSafe = _bin_op() +c = Column() +assert_type(c.isNull(), Column) +assert_type(c.eqNullSafe(c), Column) "#, );