Fix shared Config mutation when dotted override descends into a value set in the same call#32
Fix shared Config mutation when dotted override descends into a value set in the same call#32vertix wants to merge 2 commits into
Conversation
… set in the same call A Config (or container of Configs) passed as an override value was stored by reference, so a dotted key reaching into it in the same override()/__init__ call mutated the shared instance in place (issue #31). Copy Config and container values on store in both _set_value paths, mirroring _copy_value's treatment of the base, so dotted writes always land on a private copy. Also fixes the override() docstring note that previously implied the shared pattern was safe, and adds regression tests for the override, __init__, and container-value cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3RyFrZV51MdoMjkmBmd3t
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3RyFrZV51MdoMjkmBmd3t
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d1f90cb20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Copy Config/container values on store (mirroring _copy_value for the base) so that a | ||
| # dotted override descending into a value set in the same call lands on a private copy | ||
| # rather than mutating a shared instance. See issue #31. | ||
| value = _copy_value(_resolve_value(value, default, config=self)) |
There was a problem hiding this comment.
This copy-on-store path only runs for values written through _set_value, but __init__ stores *args directly before applying keyword overrides. As a result, shared = Config(Camera, name='OpenCV'); Config(Env, shared, **{'0.name': 'New Camera'}) still descends into the shared positional Config and mutates it in place (and the same happens for a positional list/tuple containing a Config). Since numeric dotted keys are supported for positional arguments, the new same-call safety guarantee still fails for constructor calls that use positional config values.
Useful? React with 👍 / 👎.
2 participants
A Config (or container of Configs) passed as an override value was stored by
reference, so a dotted key reaching into it in the same override()/init
call mutated the shared instance in place (issue #31). Copy Config and
container values on store in both _set_value paths, mirroring _copy_value's
treatment of the base, so dotted writes always land on a private copy.
Also fixes the override() docstring note that previously implied the shared
pattern was safe, and adds regression tests for the override, init, and
container-value cases.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01W3RyFrZV51MdoMjkmBmd3t