Skip to content

Commit e840895

Browse files
committed
runtime: Fixed various small bugs
- Issues with nested object tracking for GC - Cleanup compile time type construction - Fix weakref object tracking
1 parent 13e19b2 commit e840895

19 files changed

Lines changed: 367 additions & 340 deletions

integration/tests/dict.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,3 +45,26 @@ def dict_setdefault():
4545
assert a["b"] == 10
4646

4747
dict_setdefault()
48+
49+
def dict_pop_missing_key_with_failing_repr():
50+
# Regression: PyDict::pop formatted KeyError via key.__repr__() and
51+
# unconditionally unwrapped the result, aborting if __repr__ raised.
52+
class Bad:
53+
def __hash__(self):
54+
return 0
55+
def __eq__(self, other):
56+
return False
57+
def __repr__(self):
58+
raise ValueError("bad repr")
59+
60+
d = {}
61+
raised = None
62+
try:
63+
d.pop(Bad())
64+
except KeyError:
65+
raised = "KeyError"
66+
except ValueError:
67+
raised = "ValueError"
68+
assert raised is not None, "dict.pop on missing key with failing __repr__ must not abort"
69+
70+
dict_pop_missing_key_with_failing_repr()

integration/tests/list.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,3 +23,13 @@
2323
exception_raised = True
2424
finally:
2525
assert exception_raised, "list.pop with empty list should raise an IndexError"
26+
27+
def list_recursive_repr():
28+
a = []
29+
a.append(a)
30+
assert repr(a) == "[[...]]", "recursive list repr should use [...] sentinel"
31+
# Calling repr again must drain the visited set; otherwise nested
32+
# repr() would still see `a` as visited.
33+
assert repr(a) == "[[...]]", "recursive list repr should be idempotent"
34+
35+
list_recursive_repr()

integration/tests/weakref.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import _weakref
2+
import gc
3+
4+
# A class that supports weakrefs (built-ins like int/list typically don't).
5+
class Foo:
6+
pass
7+
8+
def weakref_registers_and_counts():
9+
f = Foo()
10+
r = _weakref.ref(f)
11+
# Holding a weakref does not bump the strong-ref count, but the runtime
12+
# must record the registration so weakref_count is observable.
13+
assert _weakref.getweakrefcount(f) == 1
14+
# The weakref still resolves to the target while the target is alive.
15+
assert r() is f
16+
17+
weakref_registers_and_counts()
18+
19+
def weakref_resolves_through_gc_collect():
20+
# gc.collect() forces a full mark/sweep regardless of cadence/pause
21+
# state, so this scope can deterministically check that the weakref
22+
# keeps tracking the target across collection cycles.
23+
f = Foo()
24+
r = _weakref.ref(f)
25+
gc.collect()
26+
# `f` is still on the stack here, so it must survive the collection.
27+
assert r() is f
28+
assert _weakref.getweakrefcount(f) == 1
29+
30+
weakref_resolves_through_gc_collect()
31+
32+
def gc_collect_does_not_crash_when_disabled():
33+
# Disabling the GC must not prevent gc.collect() from running; this
34+
# mirrors CPython's gc.collect() semantics.
35+
gc.disable()
36+
try:
37+
assert gc.isenabled() is False
38+
gc.collect()
39+
finally:
40+
gc.enable()
41+
assert gc.isenabled() is True
42+
43+
gc_collect_does_not_crash_when_disabled()
44+
45+
def weakref_wrapper_unregisters_on_its_own_collection():
46+
# Regression test for B8. Whenever a weakref wrapper is collected,
47+
# the runtime must scrub the heap's m_weakrefs table — otherwise
48+
# the per-target vector accumulates dangling pointers and
49+
# getweakrefcount lies. Create N immediately-unreachable wrappers
50+
# and confirm the count is zero after gc.collect(). Pre-fix this
51+
# interpreter reported 100/100 survived; post-fix and on CPython
52+
# (which refcounts wrappers away on the spot) it reports 0/100.
53+
N = 100
54+
def churn(target):
55+
for _ in range(N):
56+
_weakref.ref(target) # result discarded
57+
t = Foo()
58+
churn(t)
59+
gc.collect()
60+
remaining = _weakref.getweakrefcount(t)
61+
assert remaining == 0, \
62+
f"expected all wrappers collected, {remaining}/{N} survived"
63+
64+
weakref_wrapper_unregisters_on_its_own_collection()

src/runtime/PyDict.cpp

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,14 @@ PyResult<PyObject *> PyDict::pop(PyObject *key, PyObject *default_value)
243243
} else if (default_value) {
244244
return Ok(default_value);
245245
}
246-
return Err(key_error("{}", key->repr().unwrap()->to_string()));
246+
auto repr = key->repr();
247+
if (repr.is_err()) {
248+
// A failing user-defined __repr__ must not abort the process. Fall back
249+
// to the type name so the caller still sees a KeyError rather than the
250+
// unrelated repr exception.
251+
return Err(key_error("<{} object>", key->type()->name()));
252+
}
253+
return Err(key_error("{}", repr.unwrap()->to_string()));
247254
}
248255

249256
PyResult<std::monostate> PyDict::merge(PyObject *other, bool override)

src/runtime/PyFrame.cpp

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,18 +141,22 @@ PyResult<std::monostate> PyFrame::put_global(const std::string &name, const Valu
141141

142142
PyObject *PyFrame::locals() const
143143
{
144+
// locals() returns PyObject* and cannot propagate failures, so any
145+
// errors from setitem/delitem on a non-PyDict m_locals are silently
146+
// swallowed here. TODO: change locals() to PyResult<PyObject*> so
147+
// __setitem__/__delitem__ exceptions surface to the caller.
144148
auto insert = [this](const Value &key, const Value &value) {
145149
if (auto l = as<PyDict>(m_locals)) {
146150
l->insert(key, value);
147151
} else {
148-
m_locals->setitem(PyObject::from(key).unwrap(), PyObject::from(value).unwrap());
152+
(void)m_locals->setitem(PyObject::from(key).unwrap(), PyObject::from(value).unwrap());
149153
}
150154
};
151155
auto remove = [this](const Value &key) {
152156
if (auto l = as<PyDict>(m_locals)) {
153157
l->remove(key);
154158
} else {
155-
m_locals->delitem(PyObject::from(key).unwrap());
159+
(void)m_locals->delitem(PyObject::from(key).unwrap());
156160
}
157161
};
158162
const auto &fast_locals = VirtualMachine::the().stack_locals();

src/runtime/PyGenericAlias.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ PyType *PyGenericAlias::static_type() const { return types::generic_alias(); }
9494

9595
void PyGenericAlias::visit_graph(Visitor &visitor)
9696
{
97+
PyObject::visit_graph(visitor);
9798
if (m_origin) visitor.visit(*m_origin);
9899
if (m_args) visitor.visit(*m_args);
99100
if (m_parameters) visitor.visit(*m_parameters);

src/runtime/PyList.cpp

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,11 @@
3333

3434
namespace py {
3535

36-
static std::unordered_set<PyObject *> visited;
36+
// Tracks lists currently being repr'd on this thread so that cyclic structures
37+
// emit "[...]" instead of recursing forever. Drained by the RAII Cleanup in
38+
// __repr__; thread_local rather than file-scope static so concurrent
39+
// interpreters/threads do not see each other's in-flight repr stack.
40+
thread_local std::unordered_set<PyObject *> visited;
3741

3842
template<> PyList *as(PyObject *obj)
3943
{
@@ -131,7 +135,9 @@ PyResult<PyObject *> PyList::extend(PyObject *iterable)
131135
auto *tmp_list = tmp_list_.unwrap();
132136
auto value = iterator.unwrap()->next();
133137
while (value.is_ok()) {
134-
tmp_list->append(value.unwrap());
138+
if (auto appended = tmp_list->append(value.unwrap()); appended.is_err()) {
139+
return Err(appended.unwrap_err());
140+
}
135141
value = iterator.unwrap()->next();
136142
}
137143

0 commit comments

Comments
 (0)