Skip to content

Commit 2ea872b

Browse files
arthaudmeta-codesync[bot]
arthaud
authored and
meta-codesync[bot]
committed
Add test demonstrating a bug in the override graph with properties
Summary: There is a bug in the override graph for property getters and setters. We always find the property setter when looking for the overriden member. Unfortunately, this is not trivial to fix. The problem is that we have a `ClassField` and want to find the corresponding `DecoratedFunction`. Pyrefly always point to the setter and not the getter. Reviewed By: tianhan0 Differential Revision: D91693460 fbshipit-source-id: 3dd84e731a07515b562d109589560b07d315b8c8
1 parent 81186b0 commit 2ea872b

File tree

3 files changed

+177
-14
lines changed

3 files changed

+177
-14
lines changed

‎pyrefly/lib/report/pysa/function.rs‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -728,28 +728,24 @@ impl FunctionNode {
728728
}
729729
}
730730

731-
fn is_property_getter(&self) -> bool {
731+
fn property_role(&self) -> Option<PropertyRole> {
732732
match self {
733733
FunctionNode::DecoratedFunction(function) => function
734734
.metadata()
735735
.flags
736736
.property_metadata
737737
.as_ref()
738-
.is_some_and(|meta| matches!(meta.role, PropertyRole::Getter)),
739-
FunctionNode::ClassField { .. } => false,
738+
.map(|metadata| metadata.role.clone()),
739+
FunctionNode::ClassField { .. } => None,
740740
}
741741
}
742742

743-
fn is_property_setter(&self) -> bool {
744-
match self {
745-
FunctionNode::DecoratedFunction(function) => function
746-
.metadata()
747-
.flags
748-
.property_metadata
749-
.as_ref()
750-
.is_some_and(|meta| matches!(meta.role, PropertyRole::Setter)),
751-
FunctionNode::ClassField { .. } => false,
752-
}
743+
pub fn is_property_getter(&self) -> bool {
744+
self.property_role() == Some(PropertyRole::Getter)
745+
}
746+
747+
pub fn is_property_setter(&self) -> bool {
748+
self.property_role() == Some(PropertyRole::Setter)
753749
}
754750

755751
fn is_stub(&self) -> bool {

‎pyrefly/lib/test/pysa/functions.rs‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ use crate::test::pysa::utils::get_class_ref;
3838
use crate::test::pysa::utils::get_function_ref;
3939
use crate::test::pysa::utils::get_handle_for_module_name;
4040
use crate::test::pysa::utils::get_method_ref;
41+
use crate::test::pysa::utils::get_property_setter_ref;
4142

4243
fn create_function_definition(
4344
name: &str,
@@ -1423,3 +1424,143 @@ MyTuple = collections.namedtuple("MyTuple", "x y")
14231424
]
14241425
},
14251426
);
1427+
1428+
exported_functions_testcase!(
1429+
test_export_abstract_property,
1430+
r#"
1431+
from abc import abstractmethod
1432+
1433+
class A:
1434+
@property
1435+
@abstractmethod
1436+
def my_property(self):
1437+
pass
1438+
1439+
@my_property.setter
1440+
@abstractmethod
1441+
def my_property(self, value):
1442+
pass
1443+
1444+
class B(A):
1445+
@property
1446+
def my_property(self):
1447+
pass
1448+
1449+
@my_property.setter
1450+
def my_property(self, value):
1451+
pass
1452+
"#,
1453+
&|context: &ModuleContext| {
1454+
let abstractmethod_ref = get_function_ref("abc", "abstractmethod", context);
1455+
vec![
1456+
create_function_definition(
1457+
"my_property",
1458+
ScopeParent::Class {
1459+
location: create_location(4, 7, 4, 8),
1460+
},
1461+
/* overloads */
1462+
vec![create_simple_signature(
1463+
vec![FunctionParameter::Pos {
1464+
name: "self".into(),
1465+
annotation: PysaType::from_class(&get_class("test", "A", context), context),
1466+
required: true,
1467+
}],
1468+
PysaType::none(),
1469+
)],
1470+
)
1471+
.with_is_property_getter(true)
1472+
.with_is_stub(true)
1473+
.with_defining_class(get_class_ref("test", "A", context))
1474+
.with_decorator_callees(HashMap::from([(
1475+
create_location(6, 6, 6, 20),
1476+
vec![Target::Function(abstractmethod_ref.clone())],
1477+
)])),
1478+
create_function_definition(
1479+
"my_property",
1480+
ScopeParent::Class {
1481+
location: create_location(4, 7, 4, 8),
1482+
},
1483+
/* overloads */
1484+
vec![create_simple_signature(
1485+
vec![
1486+
FunctionParameter::Pos {
1487+
name: "self".into(),
1488+
annotation: PysaType::from_class(
1489+
&get_class("test", "A", context),
1490+
context,
1491+
),
1492+
required: true,
1493+
},
1494+
FunctionParameter::Pos {
1495+
name: "value".into(),
1496+
annotation: PysaType::any_implicit(),
1497+
required: true,
1498+
},
1499+
],
1500+
PysaType::none(),
1501+
)],
1502+
)
1503+
.with_is_property_setter(true)
1504+
.with_is_stub(true)
1505+
.with_defining_class(get_class_ref("test", "A", context))
1506+
.with_decorator_callees(HashMap::from([(
1507+
create_location(11, 6, 11, 20),
1508+
vec![Target::Function(abstractmethod_ref.clone())],
1509+
)])),
1510+
create_function_definition(
1511+
"my_property",
1512+
ScopeParent::Class {
1513+
location: create_location(15, 7, 15, 8),
1514+
},
1515+
/* overloads */
1516+
vec![create_simple_signature(
1517+
vec![FunctionParameter::Pos {
1518+
name: "self".into(),
1519+
annotation: PysaType::from_class(&get_class("test", "B", context), context),
1520+
required: true,
1521+
}],
1522+
PysaType::none(),
1523+
)],
1524+
)
1525+
.with_is_property_getter(true)
1526+
.with_defining_class(get_class_ref("test", "B", context))
1527+
.with_overridden_base_method(
1528+
// TODO(T225700656): This should refer to the property getter, not the setter.
1529+
get_property_setter_ref("test", "A", "my_property", context),
1530+
),
1531+
create_function_definition(
1532+
"my_property",
1533+
ScopeParent::Class {
1534+
location: create_location(15, 7, 15, 8),
1535+
},
1536+
/* overloads */
1537+
vec![create_simple_signature(
1538+
vec![
1539+
FunctionParameter::Pos {
1540+
name: "self".into(),
1541+
annotation: PysaType::from_class(
1542+
&get_class("test", "B", context),
1543+
context,
1544+
),
1545+
required: true,
1546+
},
1547+
FunctionParameter::Pos {
1548+
name: "value".into(),
1549+
annotation: PysaType::any_implicit(),
1550+
required: true,
1551+
},
1552+
],
1553+
PysaType::none(),
1554+
)],
1555+
)
1556+
.with_is_property_setter(true)
1557+
.with_defining_class(get_class_ref("test", "B", context))
1558+
.with_overridden_base_method(get_property_setter_ref(
1559+
"test",
1560+
"A",
1561+
"my_property",
1562+
context,
1563+
)),
1564+
]
1565+
},
1566+
);

‎pyrefly/lib/test/pysa/utils.rs‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,18 +92,20 @@ pub fn get_function_ref(
9292
.as_function_ref(&context)
9393
}
9494

95-
pub fn get_method_ref(
95+
fn get_method_ref_with_predicate(
9696
module_name: &str,
9797
class_name: &str,
9898
function_name: &str,
9999
context: &ModuleContext,
100+
predicate: impl Fn(&FunctionNode) -> bool,
100101
) -> FunctionRef {
101102
let handle = get_handle_for_module_name(module_name, context.transaction);
102103
let context = ModuleContext::create(handle, context.transaction, context.module_ids).unwrap();
103104

104105
// This is slow, but we don't care in tests.
105106
get_all_functions(&context)
106107
.filter(|function| function.should_export(&context))
108+
.filter(|function| predicate(function))
107109
.find(|function| match function {
108110
FunctionNode::DecoratedFunction(decorated_function) => {
109111
function.name().as_str() == function_name
@@ -121,6 +123,30 @@ pub fn get_method_ref(
121123
.as_function_ref(&context)
122124
}
123125

126+
pub fn get_method_ref(
127+
module_name: &str,
128+
class_name: &str,
129+
function_name: &str,
130+
context: &ModuleContext,
131+
) -> FunctionRef {
132+
get_method_ref_with_predicate(module_name, class_name, function_name, context, |_| true)
133+
}
134+
135+
pub fn get_property_setter_ref(
136+
module_name: &str,
137+
class_name: &str,
138+
function_name: &str,
139+
context: &ModuleContext,
140+
) -> FunctionRef {
141+
get_method_ref_with_predicate(
142+
module_name,
143+
class_name,
144+
function_name,
145+
context,
146+
|function| function.is_property_setter(),
147+
)
148+
}
149+
124150
pub fn get_global_ref(
125151
module_name: &str,
126152
global_name: &str,

0 commit comments

Comments
 (0)