From 7758b70c59e6de5b56b06c84cf3fda5ecdef5e9f Mon Sep 17 00:00:00 2001 From: Aryan Bagade Date: Tue, 30 Dec 2025 15:22:03 -0800 Subject: [PATCH 1/2] Fix: Skip bad-return validation for Protocol methods (#1916) Signed-off-by: Aryan Bagade --- pyrefly/lib/alt/solve.rs | 14 ++++++++++++-- pyrefly/lib/binding/binding.rs | 2 ++ pyrefly/lib/binding/function.rs | 5 +++++ pyrefly/lib/test/protocol.rs | 30 ++++++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index dc2cae3a5b..08a6369c54 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3155,6 +3155,7 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { annotation, stub_or_impl, decorators, + class_key, implicit_return, is_generator, has_explicit_return, @@ -3163,9 +3164,18 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { // It will result in an implicit Any type, which is reasonable, but we should // at least error here. let ty = self.get_idx(*annotation).annotation.get_type().clone(); - // If the function body is stubbed out or if the function is decorated with - // `@abstractmethod`, we blindly accept the return type annotation. + // Check if this method is in a protocol class + let is_in_protocol = class_key.is_some_and(|key| { + self.get_idx(key) + .0 + .as_ref() + .is_some_and(|cls| self.get_metadata_for_class(cls).is_protocol()) + }); + // If the function body is stubbed out, if the function is decorated with + // `@abstractmethod`, or if the function is a method in a protocol class, + // we blindly accept the return type annotation. if *stub_or_impl != FunctionStubOrImpl::Stub + && !is_in_protocol && !decorators.iter().any(|k| { let decorator = self.get_idx(*k); match decorator.ty.callee_kind() { diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index 3eaa23ccaf..b6521a81f7 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -1174,6 +1174,8 @@ pub enum ReturnTypeKind { /// We keep this just so we can scan for `@abstractmethod` and use the info to decide /// whether to skip the validation. decorators: Box<[Idx]>, + /// The class this function belongs to, if any. Used to skip validation for protocol methods. + class_key: Option>, implicit_return: Idx, is_generator: bool, has_explicit_return: bool, diff --git a/pyrefly/lib/binding/function.rs b/pyrefly/lib/binding/function.rs index 4f7281e15e..432f294efd 100644 --- a/pyrefly/lib/binding/function.rs +++ b/pyrefly/lib/binding/function.rs @@ -371,6 +371,7 @@ impl<'a> BindingsBuilder<'a> { should_infer_return_type: bool, stub_or_impl: FunctionStubOrImpl, decorators: Box<[Idx]>, + class_key: Option>, ) { let is_generator = !(yields_and_returns.yields.is_empty() && yields_and_returns.yield_froms.is_empty()); @@ -431,6 +432,7 @@ impl<'a> BindingsBuilder<'a> { annotation, stub_or_impl, decorators, + class_key, implicit_return, is_generator: !(yield_keys.is_empty() && yield_from_keys.is_empty()), has_explicit_return: !return_keys.is_empty(), @@ -613,6 +615,7 @@ impl<'a> BindingsBuilder<'a> { false, // this disables return type inference stub_or_impl, decorators.decorators.clone(), + class_key, ); self_assignments } @@ -643,6 +646,7 @@ impl<'a> BindingsBuilder<'a> { false, // this disables return type inference stub_or_impl, decorators.decorators.clone(), + class_key, ); self_assignments } @@ -673,6 +677,7 @@ impl<'a> BindingsBuilder<'a> { true, stub_or_impl, decorators.decorators.clone(), + class_key, ); self_assignments } diff --git a/pyrefly/lib/test/protocol.rs b/pyrefly/lib/test/protocol.rs index d1f43b4d33..29e87779f8 100644 --- a/pyrefly/lib/test/protocol.rs +++ b/pyrefly/lib/test/protocol.rs @@ -847,3 +847,33 @@ def test(): p: TrickyProtocol[int] = t # E: "#, ); + +// Regression test for https://github.com/facebook/pyrefly/issues/1916 +// Protocol methods with only a docstring should not emit "missing explicit return" errors +testcase!( + test_protocol_method_with_docstring, + r#" +from typing import Protocol + +class SortState: + pass + +class View(Protocol): + """A protocol with methods that have docstrings but no body.""" + + @property + def sort_state(self) -> SortState: + """Return the current sorting/grouping settings.""" + + def get_value(self) -> int: + """Get a value.""" + + def method_with_ellipsis(self) -> str: + """This one uses ellipsis.""" + ... + + def method_with_pass(self) -> str: + """This one uses pass - should still be allowed in protocol.""" + pass +"#, +); From 14a2bc0efb1a8af21576724120c49d3b6bfdd3e7 Mon Sep 17 00:00:00 2001 From: Aryan Bagade Date: Wed, 7 Jan 2026 23:19:55 -0800 Subject: [PATCH 2/2] Simplify Protocol bad-return fix per review feedback Signed-off-by: Aryan Bagade --- pyrefly/lib/alt/solve.rs | 14 ++------------ pyrefly/lib/binding/binding.rs | 2 -- pyrefly/lib/binding/class.rs | 15 ++++++++++++++- pyrefly/lib/binding/function.rs | 28 +++++++++++++++++++++++----- pyrefly/lib/binding/scope.rs | 12 ++++++++++++ 5 files changed, 51 insertions(+), 20 deletions(-) diff --git a/pyrefly/lib/alt/solve.rs b/pyrefly/lib/alt/solve.rs index 08a6369c54..dc2cae3a5b 100644 --- a/pyrefly/lib/alt/solve.rs +++ b/pyrefly/lib/alt/solve.rs @@ -3155,7 +3155,6 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { annotation, stub_or_impl, decorators, - class_key, implicit_return, is_generator, has_explicit_return, @@ -3164,18 +3163,9 @@ impl<'a, Ans: LookupAnswer> AnswersSolver<'a, Ans> { // It will result in an implicit Any type, which is reasonable, but we should // at least error here. let ty = self.get_idx(*annotation).annotation.get_type().clone(); - // Check if this method is in a protocol class - let is_in_protocol = class_key.is_some_and(|key| { - self.get_idx(key) - .0 - .as_ref() - .is_some_and(|cls| self.get_metadata_for_class(cls).is_protocol()) - }); - // If the function body is stubbed out, if the function is decorated with - // `@abstractmethod`, or if the function is a method in a protocol class, - // we blindly accept the return type annotation. + // If the function body is stubbed out or if the function is decorated with + // `@abstractmethod`, we blindly accept the return type annotation. if *stub_or_impl != FunctionStubOrImpl::Stub - && !is_in_protocol && !decorators.iter().any(|k| { let decorator = self.get_idx(*k); match decorator.ty.callee_kind() { diff --git a/pyrefly/lib/binding/binding.rs b/pyrefly/lib/binding/binding.rs index b6521a81f7..3eaa23ccaf 100644 --- a/pyrefly/lib/binding/binding.rs +++ b/pyrefly/lib/binding/binding.rs @@ -1174,8 +1174,6 @@ pub enum ReturnTypeKind { /// We keep this just so we can scan for `@abstractmethod` and use the info to decide /// whether to skip the validation. decorators: Box<[Idx]>, - /// The class this function belongs to, if any. Used to skip validation for protocol methods. - class_key: Option>, implicit_return: Idx, is_generator: bool, has_explicit_return: bool, diff --git a/pyrefly/lib/binding/class.rs b/pyrefly/lib/binding/class.rs index 7daa0b33d8..3b557d828d 100644 --- a/pyrefly/lib/binding/class.rs +++ b/pyrefly/lib/binding/class.rs @@ -33,6 +33,7 @@ use starlark_map::small_map::SmallMap; use crate::binding::base_class::BaseClass; use crate::binding::base_class::BaseClassGeneric; +use crate::binding::base_class::BaseClassGenericKind; use crate::binding::binding::AnnotationTarget; use crate::binding::binding::Binding; use crate::binding::binding::BindingAbstractClassCheck; @@ -111,6 +112,7 @@ impl<'a> BindingsBuilder<'a> { consistent_override_check_idx: self .idx_for_promise(KeyConsistentOverrideCheck(def_index)), abstract_class_check_idx: self.idx_for_promise(KeyAbstractClassCheck(def_index)), + is_protocol: false, // Will be set after processing bases }; // The user - used for first-usage tracking of any expressions we analyze in a class definition - // is the `Idx` of the class object bound to the class name. @@ -120,7 +122,7 @@ impl<'a> BindingsBuilder<'a> { } pub fn class_def(&mut self, mut x: StmtClassDef, parent: &NestingContext) { - let (mut class_object, class_indices) = self.class_object_and_indices(&x.name); + let (mut class_object, mut class_indices) = self.class_object_and_indices(&x.name); let mut pydantic_config_dict = PydanticConfigDict::default(); let docstring_range = Docstring::range_from_stmts(x.body.as_slice()); let body = mem::take(&mut x.body); @@ -197,6 +199,17 @@ impl<'a> BindingsBuilder<'a> { base_class }); + // Check if this class directly inherits from Protocol + class_indices.is_protocol = bases.iter().any(|base| { + matches!( + base, + BaseClass::Generic(BaseClassGeneric { + kind: BaseClassGenericKind::Protocol, + .. + }) + ) + }); + let mut keywords = Vec::new(); if let Some(args) = &mut x.arguments { args.keywords.iter_mut().for_each(|keyword| { diff --git a/pyrefly/lib/binding/function.rs b/pyrefly/lib/binding/function.rs index 432f294efd..f1096a3303 100644 --- a/pyrefly/lib/binding/function.rs +++ b/pyrefly/lib/binding/function.rs @@ -371,7 +371,6 @@ impl<'a> BindingsBuilder<'a> { should_infer_return_type: bool, stub_or_impl: FunctionStubOrImpl, decorators: Box<[Idx]>, - class_key: Option>, ) { let is_generator = !(yields_and_returns.yields.is_empty() && yields_and_returns.yield_froms.is_empty()); @@ -432,7 +431,6 @@ impl<'a> BindingsBuilder<'a> { annotation, stub_or_impl, decorators, - class_key, implicit_return, is_generator: !(yield_keys.is_empty() && yield_from_keys.is_empty()), has_explicit_return: !return_keys.is_empty(), @@ -528,10 +526,33 @@ impl<'a> BindingsBuilder<'a> { undecorated_idx: Idx, class_key: Option>, ) -> (FunctionStubOrImpl, Option) { + // A method in a Protocol with a stub-like body (docstring-only, docstring + pass, + // docstring + ellipsis) is treated as a stub. This matches Python typing spec + // behavior where Protocol methods don't need implementations. + let is_protocol_stub_body = if self.scopes.is_in_protocol_class() { + match body.as_slice() { + // Docstring only + [stmt] if is_docstring(stmt) => true, + // Docstring + pass + [first, Stmt::Pass(_)] if is_docstring(first) => true, + // Docstring + ellipsis + [first, Stmt::Expr(expr_stmt)] + if is_docstring(first) + && matches!(expr_stmt.value.as_ref(), Expr::EllipsisLiteral(_)) => + { + true + } + _ => false, + } + } else { + false + }; + let stub_or_impl = if (body.first().is_some_and(is_docstring) && decorators.is_abstract_method) || is_ellipse(&body) || (body.first().is_some_and(is_docstring) && decorators.is_overload) + || is_protocol_stub_body { FunctionStubOrImpl::Stub } else { @@ -615,7 +636,6 @@ impl<'a> BindingsBuilder<'a> { false, // this disables return type inference stub_or_impl, decorators.decorators.clone(), - class_key, ); self_assignments } @@ -646,7 +666,6 @@ impl<'a> BindingsBuilder<'a> { false, // this disables return type inference stub_or_impl, decorators.decorators.clone(), - class_key, ); self_assignments } @@ -677,7 +696,6 @@ impl<'a> BindingsBuilder<'a> { true, stub_or_impl, decorators.decorators.clone(), - class_key, ); self_assignments } diff --git a/pyrefly/lib/binding/scope.rs b/pyrefly/lib/binding/scope.rs index e60cbe4ede..eea665acde 100644 --- a/pyrefly/lib/binding/scope.rs +++ b/pyrefly/lib/binding/scope.rs @@ -690,6 +690,8 @@ pub struct ClassIndices { pub variance_idx: Idx, pub consistent_override_check_idx: Idx, pub abstract_class_check_idx: Idx, + /// Whether this class directly inherits from Protocol. + pub is_protocol: bool, } #[derive(Clone, Debug)] @@ -1231,6 +1233,16 @@ impl Scopes { None } + /// Check if we're currently inside a Protocol class body. + pub fn is_in_protocol_class(&self) -> bool { + for scope in self.iter_rev() { + if let ScopeKind::Class(class_scope) = &scope.kind { + return class_scope.indices.is_protocol; + } + } + false + } + /// Are we inside an async function or method? pub fn is_in_async_def(&self) -> bool { for scope in self.iter_rev() {