Skip to content

Fix RecursionError when assigning a benedict into its own nested descendant (#592) - #597

Open
dualfroz wants to merge 1 commit into
fabiocaccamo:mainfrom
dualfroz:dualfroz/fix-recursion-self-nesting
Open

Fix RecursionError when assigning a benedict into its own nested descendant (#592)#597
dualfroz wants to merge 1 commit into
fabiocaccamo:mainfrom
dualfroz:dualfroz/fix-recursion-self-nesting

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Problem

#592

Assigning a benedict into a nested descendant of itself works once, but a
second such assignment crashes with RecursionError (and, per the issue,
can crash a Jupyter kernel):

from benedict import benedict

test = benedict(keyattr_enabled=True, keyattr_dynamic=True)
test.a.b.c = test        # works
test.a.b.d.e = test      # RecursionError: maximum recursion depth exceeded

Root cause

BaseDict._get_dict_or_value() in benedict/dicts/base/base_dict.py (lines
25-34 on main) recursively unwraps nested benedict values with no cycle
detection, and it is called from every __init__/__setitem__/update/
setdefault. After the first assignment, test contains a value that
(transitively) is test itself. Any later operation that needs to unwrap
test again (eg. navigating test.a to perform the second assignment)
recurses into that self-reference and never terminates.

The actual recursion is not a simple self-call of _get_dict_or_value: it
alternates between _get_dict_or_value, _cast/__init__ (which wrap the
same underlying raw dict into a fresh benedict object on every access) and
__setitem__ -> check_keys/traverse (benedict/core/traverse.py, which
has no cycle protection of its own either). All of these frames belong to
one continuous call stack, so a cycle guard confined to
_get_dict_or_value still sees the re-entry and can stop the whole chain
before the uncontrolled recursion is ever reached elsewhere.

Fix

benedict/dicts/base/base_dict.py: _get_dict_or_value now tracks, in a
class-level _unwrapping_ids set, the id() of every mapping it is
currently in the middle of unwrapping (added on entry, removed via
finally on exit -- so it only ever reflects containers that are live on
the current call stack, not a permanent "already seen" memo, which would
incorrectly flag legitimate shared/aliased references as cycles). If a
mapping is encountered while it is already being unwrapped higher up the
stack, the structure is self-referential: instead of recursing again (which
would eventually blow the stack, potentially inside unrelated,
unprotected code such as check_keys/traverse), a ValueError is raised
immediately, unwinding the whole unwrap/cast chain in a controlled way.

Normal (non-cyclic) nested-dict unwrapping is unaffected: the guard is only
ever populated with ids of mappings still being processed, and a proper
(acyclic) tree can never revisit a node that is still being visited, so the
new code paths are simply never exercised for legitimate data.

Scope note: a complete "silently supported" self-reference (making the
second assignment succeed rather than raise) is not achievable without
also touching benedict/core/traverse.py's _traverse_dict/
_traverse_collection (used, via check_keys, on every __setitem__, and
also by several other public APIs such as merge/flatten/keypaths),
since that traversal is independently unprotected against cycles. That was
considered out of scope for a minimal, low-risk fix, so this change turns
the uncontrolled RecursionError into a clear, immediate ValueError
instead.

…#592)

BaseDict._get_dict_or_value() recursively unwraps nested benedict
values with no cycle detection, so assigning a benedict into its own
nested descendant a second time recurses forever and crashes with
RecursionError. Track ids of mappings currently being unwrapped and
raise a clear ValueError as soon as a self-reference is detected,
instead of recursing into unrelated, equally unprotected traversal
code until the interpreter stack overflows.
@dualfroz
dualfroz force-pushed the dualfroz/fix-recursion-self-nesting branch from beb3578 to 545cbf4 Compare September 5, 2026 23:10
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.39%. Comparing base (ba7e146) to head (545cbf4).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #597   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files          64       64           
  Lines        2418     2425    +7     
=======================================
+ Hits         2379     2386    +7     
  Misses         39       39           
Flag Coverage Δ
unittests 98.39% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant