Skip to content

Fix panic in code.replace() with non-interned strings - #8471

Merged
youknowone merged 1 commit into
RustPython:mainfrom
MsfPablo:fix-code-replace-intern-panic
Aug 9, 2026
Merged

Fix panic in code.replace() with non-interned strings#8471
youknowone merged 1 commit into
RustPython:mainfrom
MsfPablo:fix-code-replace-intern-panic

Conversation

@MsfPablo

@MsfPablo MsfPablo commented Aug 8, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

code.replace() called as_interned_str().unwrap() on its string arguments, which panics when the caller passes a string that has not been interned.

modulefinder.ModuleFinder(replace_paths=...) hits this: replace_paths_in_code() builds a fresh co_filename and passes it to co.replace(), aborting the interpreter. This was the last remaining skip in test_modulefinder, part of #3846.

This interns the incoming strings instead, and raises TypeError instead of panicking when a non-string appears in co_names/co_varnames/co_cellvars/co_freevars.

def f(): pass
f.__code__.replace(co_filename='/new/path.py')  # panicked before, works now

test_modulefinder.test_replace_paths is unskipped. test_modulefinder, test_code, test_dis, test_funcattrs, test_sys_settrace all pass.

Summary by CodeRabbit

  • Bug Fixes
    • Improved validation when replacing code-object metadata.
    • Invalid non-string entries now produce a clear TypeError instead of causing failures during conversion.
    • String metadata is handled consistently for more reliable runtime behavior.

Pablo Garcia Pablo Garcia
Fix panic in code.replace() with non-interned strings
8139b4e 
code.replace() called as_interned_str().unwrap() on its string
arguments, which panics when the caller passes a string that has not
been interned. modulefinder's replace_paths_in_code() builds a fresh
co_filename, so any use of ModuleFinder(replace_paths=...) aborted the
interpreter.

Intern the incoming strings instead, and raise TypeError rather than
panicking when a non-string appears in co_names/co_varnames/
co_cellvars/co_freevars.

Unskips test_modulefinder.test_replace_paths.
@coderabbitai

coderabbitai Bot commented Aug 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1f274c5e-aecb-4e9a-b10c-1dca380fdea8

📥 Commits

Reviewing files that changed from the base of the PR and between 1819677 and 8139b4e.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_modulefinder.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/builtins/code.rs
📝 Walkthrough

Walkthrough

PyCode.replace now validates and interns string metadata through a shared helper. Invalid entries return TypeError instead of causing unchecked conversion failures. Scalar metadata fields are interned directly.

Changes

Code metadata validation

Layer / File(s) Summary
Validate and intern replacement metadata
crates/vm/src/builtins/code.rs		 PyCode.replace validates and interns cell variables, free variables, names, and variable names. It interns filename, name, and qualified name directly. Non-string sequence entries return TypeError.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: youknowone, shaharnaveh

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change: preventing a panic in code.replace() for non-interned strings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/modulefinder.py
[x] test: cpython/Lib/test/test_modulefinder.py

dependencies:

  • modulefinder

dependent tests: (2 tests)

  • modulefinder: test_importlib test_modulefinder

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@moreal
moreal self-requested a review August 9, 2026 06:04
youknowone
youknowone approved these changes Aug 9, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this makes sense. Obviously replace can introduce new strings. Thanks!

@youknowone
youknowone merged commit 5b3eb41 into RustPython:main Aug 9, 2026
28 checks passed
kyokuping pushed a commit to kyokuping/RustPython that referenced this pull request Aug 9, 2026
@MsfPablo Pablo Garcia @kyokuping
Fix panic in code.replace() with non-interned strings (RustPython#8471)
1da5b37 
code.replace() called as_interned_str().unwrap() on its string
arguments, which panics when the caller passes a string that has not
been interned. modulefinder's replace_paths_in_code() builds a fresh
co_filename, so any use of ModuleFinder(replace_paths=...) aborted the
interpreter.

Intern the incoming strings instead, and raise TypeError rather than
panicking when a non-string appears in co_names/co_varnames/
co_cellvars/co_freevars.

Unskips test_modulefinder.test_replace_paths.

Co-authored-by: Pablo Garcia <pablito@MacBook-Neo-de-Pablo.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowone youknowone youknowone approved these changes
@moreal moreal Awaiting requested review from moreal

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MsfPablo @youknowone