Skip to content

Commit 12019c8

Browse files
AryanBagademeta-codesync[bot]
AryanBagade
authored and
meta-codesync[bot]
committed
consistently skip bad-return validation for stub methods (#1965)
Summary: We now consistently skip implicit return validations for all stub functions, which are functions that fit both these criteria: 1. are defined inside a protocol class body OR decorated w/ abstract method/overload 2. have trivial bodies like pass/return NotImplemented, raise NotImplementedError, or ... (the body may only be a docstring, or have a docstring before its first statement) We will now error for non-stub functions that have `...` as the body Fixes #1916 Fixes #2159 Pull Request resolved: #1965 Test Plan: - Added `test_protocol_method_with_docstring` test case covering: - `property` with docstring only - Regular method with docstring only - Method with `pass` - Method with ellipsis (already worked) - Verified regular (non-Protocol) classes still error correctly - Ran `python3 test.py` - all tests pass - Ran `cargo test --lib protocol::` - all 37 protocol tests pass Reviewed By: stroxler Differential Revision: D90340690 Pulled By: yangdanny97 fbshipit-source-id: c070e7e7611552ec490fb87fcca988203962ca10
1 parent cae49a9 commit 12019c8

4 files changed

Lines changed: 179 additions & 20 deletions

File tree

‎pyrefly/lib/binding/class.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ use starlark_map::small_map::SmallMap;
3333

3434
use crate::binding::base_class::BaseClass;
3535
use crate::binding::base_class::BaseClassGeneric;
36+
use crate::binding::base_class::BaseClassGenericKind;
3637
use crate::binding::binding::AnnotationTarget;
3738
use crate::binding::binding::Binding;
3839
use crate::binding::binding::BindingAbstractClassCheck;
@@ -197,6 +198,16 @@ impl<'a> BindingsBuilder<'a> {
197198
base_class
198199
});
199200

201+
let has_protocol_base = bases.iter().any(|base| {
202+
matches!(
203+
base,
204+
BaseClass::Generic(BaseClassGeneric {
205+
kind: BaseClassGenericKind::Protocol,
206+
..
207+
})
208+
)
209+
});
210+
200211
let mut keywords = Vec::new();
201212
if let Some(args) = &mut x.arguments {
202213
args.keywords.iter_mut().for_each(|keyword| {
@@ -242,6 +253,7 @@ impl<'a> BindingsBuilder<'a> {
242253
x.range,
243254
class_indices.clone(),
244255
x.name.clone(),
256+
has_protocol_base,
245257
));
246258
self.init_static_scope(&body, false);
247259
self.stmts(

‎pyrefly/lib/binding/function.rs‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -537,17 +537,16 @@ impl<'a> BindingsBuilder<'a> {
537537
class_key: Option<Idx<KeyClass>>,
538538
metadata_key: Option<Idx<KeyClassMetadata>>,
539539
) -> (FunctionStubOrImpl, Option<SelfAssignments>) {
540-
let stub_or_impl = if (body.first().is_some_and(is_docstring)
541-
&& decorators.is_abstract_method)
542-
|| is_ellipse(&body)
543-
|| (body.first().is_some_and(is_docstring) && decorators.is_overload)
540+
// If the first statement in the body is a docstring, remove it
541+
let body_no_docstring = if let Some(s) = body.first()
542+
&& is_docstring(s)
544543
{
545-
FunctionStubOrImpl::Stub
544+
&body.as_slice()[1..]
546545
} else {
547-
FunctionStubOrImpl::Impl
546+
body.as_slice()
548547
};
549-
550-
let body_is_trivial = match body.as_slice() {
548+
let body_is_trivial = match body_no_docstring {
549+
[] => true,
551550
[Stmt::Pass(_)] => true,
552551
// raise NotImplementedError(...)
553552
[
@@ -563,14 +562,28 @@ impl<'a> BindingsBuilder<'a> {
563562
..
564563
}),
565564
] if self.as_special_export(val) == Some(SpecialExport::NotImplemented) => true,
565+
[Stmt::Expr(StmtExpr { value, .. })] if value.is_ellipsis_literal_expr() => true,
566566
_ => false,
567567
};
568+
let body_is_ellipse = match body_no_docstring {
569+
[Stmt::Expr(StmtExpr { value, .. })] if value.is_ellipsis_literal_expr() => true,
570+
_ => false,
571+
};
572+
let stub_or_impl = if (self.scopes.is_in_protocol_class()
573+
|| decorators.is_abstract_method
574+
|| decorators.is_overload
575+
|| body_is_ellipse)
576+
&& body_is_trivial
577+
{
578+
FunctionStubOrImpl::Stub
579+
} else {
580+
FunctionStubOrImpl::Impl
581+
};
568582
let should_report_unused_parameters = stub_or_impl == FunctionStubOrImpl::Impl
569583
&& !body_is_trivial
570584
&& !decorators.is_overload
571585
&& !decorators.is_override
572-
&& !decorators.is_abstract_method
573-
&& !is_ellipse(&body);
586+
&& !decorators.is_abstract_method;
574587
let method_self_kind = if class_key.is_some()
575588
&& (decorators.is_classmethod
576589
|| func_name.id == dunder::INIT_SUBCLASS
@@ -896,10 +909,3 @@ fn is_docstring(x: &Stmt) -> bool {
896909
_ => false,
897910
}
898911
}
899-
900-
fn is_ellipse(x: &[Stmt]) -> bool {
901-
match x.iter().find(|x| !is_docstring(x)) {
902-
Some(Stmt::Expr(StmtExpr { value, .. })) => value.is_ellipsis_literal_expr(),
903-
_ => false,
904-
}
905-
}

‎pyrefly/lib/binding/scope.rs‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -707,15 +707,17 @@ struct ScopeClass {
707707
indices: ClassIndices,
708708
attributes_from_recognized_methods: SmallMap<Name, SmallMap<Name, InstanceAttribute>>,
709709
attributes_from_other_methods: SmallMap<Name, SmallMap<Name, InstanceAttribute>>,
710+
has_protocol_base: bool,
710711
}
711712

712713
impl ScopeClass {
713-
pub fn new(name: Identifier, indices: ClassIndices) -> Self {
714+
pub fn new(name: Identifier, indices: ClassIndices, has_protocol_base: bool) -> Self {
714715
Self {
715716
name,
716717
indices,
717718
attributes_from_recognized_methods: SmallMap::new(),
718719
attributes_from_other_methods: SmallMap::new(),
720+
has_protocol_base,
719721
}
720722
}
721723

@@ -1029,11 +1031,16 @@ impl Scope {
10291031
Self::new(range, FlowBarrier::AllowFlowChecked, ScopeKind::TypeAlias)
10301032
}
10311033

1032-
pub fn class_body(range: TextRange, indices: ClassIndices, name: Identifier) -> Self {
1034+
pub fn class_body(
1035+
range: TextRange,
1036+
indices: ClassIndices,
1037+
name: Identifier,
1038+
has_protocol_base: bool,
1039+
) -> Self {
10331040
Self::new(
10341041
range,
10351042
FlowBarrier::AllowFlowChecked,
1036-
ScopeKind::Class(ScopeClass::new(name, indices)),
1043+
ScopeKind::Class(ScopeClass::new(name, indices, has_protocol_base)),
10371044
)
10381045
}
10391046

@@ -1245,6 +1252,16 @@ impl Scopes {
12451252
None
12461253
}
12471254

1255+
/// Check if we're currently in the body of a class with `Protocol` in its base class list
1256+
pub fn is_in_protocol_class(&self) -> bool {
1257+
for scope in self.iter_rev() {
1258+
if let ScopeKind::Class(class_scope) = &scope.kind {
1259+
return class_scope.has_protocol_base;
1260+
}
1261+
}
1262+
false
1263+
}
1264+
12481265
/// Are we inside an async function or method?
12491266
pub fn is_in_async_def(&self) -> bool {
12501267
for scope in self.iter_rev() {

‎pyrefly/lib/test/returns.rs‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,3 +419,127 @@ def test():
419419
yield from [2, 3] # E: This `yield from` expression is unreachable
420420
"#,
421421
);
422+
423+
testcase!(
424+
test_no_missing_return_for_stubs,
425+
r#"
426+
from typing import Protocol, overload
427+
from abc import abstractmethod
428+
429+
class P(Protocol):
430+
def f1(self) -> int:
431+
"""a"""
432+
def f2(self) -> int:
433+
"""a"""
434+
...
435+
def f3(self) -> int:
436+
"""a"""
437+
pass
438+
def f4(self) -> int:
439+
"""a"""
440+
return NotImplemented
441+
def f5(self) -> int:
442+
"""a"""
443+
raise NotImplementedError()
444+
def f6(self) -> int:
445+
...
446+
def f7(self) -> int:
447+
pass
448+
def f8(self) -> int:
449+
return NotImplemented
450+
def f9(self) -> int:
451+
raise NotImplementedError()
452+
453+
class C:
454+
def f1(self) -> int: # E:
455+
"""a"""
456+
def f2(self) -> int:
457+
"""a"""
458+
... # OK - other type checkers do not permit this outside of stub files
459+
def f3(self) -> int: # E:
460+
"""a"""
461+
pass
462+
def f4(self) -> int:
463+
"""a"""
464+
return NotImplemented # OK
465+
def f5(self) -> int:
466+
"""a"""
467+
raise NotImplementedError() # OK
468+
def f6(self) -> int:
469+
... # OK - other type checkers do not permit this outside of stub files
470+
def f7(self) -> int: # E:
471+
pass
472+
def f8(self) -> int:
473+
return NotImplemented # OK
474+
def f9(self) -> int:
475+
raise NotImplementedError() # OK
476+
477+
class AbstractC:
478+
@abstractmethod
479+
def f1(self) -> int:
480+
"""a"""
481+
@abstractmethod
482+
def f2(self) -> int:
483+
"""a"""
484+
...
485+
@abstractmethod
486+
def f3(self) -> int:
487+
"""a"""
488+
pass
489+
@abstractmethod
490+
def f4(self) -> int:
491+
"""a"""
492+
return NotImplemented
493+
@abstractmethod
494+
def f5(self) -> int:
495+
"""a"""
496+
raise NotImplementedError()
497+
@abstractmethod
498+
def f6(self) -> int:
499+
...
500+
@abstractmethod
501+
def f7(self) -> int:
502+
pass
503+
@abstractmethod
504+
def f8(self) -> int:
505+
return NotImplemented
506+
@abstractmethod
507+
def f9(self) -> int:
508+
raise NotImplementedError()
509+
510+
class OverloadC:
511+
@overload
512+
def f(self) -> int:
513+
"""a"""
514+
@overload
515+
def f(self) -> int:
516+
"""a"""
517+
...
518+
@overload
519+
def f(self) -> int:
520+
"""a"""
521+
pass
522+
@overload
523+
def f(self) -> int:
524+
"""a"""
525+
return NotImplemented
526+
@overload
527+
def f(self) -> int:
528+
"""a"""
529+
raise NotImplementedError()
530+
@overload
531+
def f(self) -> int:
532+
...
533+
@overload
534+
def f(self) -> int:
535+
pass
536+
@overload
537+
def f(self) -> int:
538+
return NotImplemented
539+
@overload
540+
def f(self) -> int:
541+
raise NotImplementedError()
542+
def f(self) -> int:
543+
return 0
544+
"#,
545+
);

0 commit comments

Comments
 (0)