Add regression test for #4505 by ShaharNaveh · Pull Request #6417 · RustPython/RustPython

ShaharNaveh · 2025-12-12T11:16:16Z

I cannot reproduce this on 75dcf80.
Closes #4505

Summary by CodeRabbit

Tests
- Added regression tests for builtin type handling to improve code reliability and ensure proper behavior of nested function definitions and code object introspection.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-12T11:16:28Z

Walkthrough

A regression test is added to extra_tests/snippets/builtin_type.py verifying that code.co_names does not leak __doc__ entries when nested functions are defined or when compiling module source strings, addressing issue #4505.

Changes

Cohort / File(s)	Summary
Regression test for co_names leak `extra_tests/snippets/builtin_type.py`	Adds a regression test block that verifies `code.co_names` excludes `__doc__` entries in two scenarios: nested function definitions and compiled module code. Includes import rearrangement and formatting adjustments.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

This is a straightforward test addition with minimal logic. The changes consist of a simple assertion block and formatting adjustments with no complex logic or structural modifications.

Poem

🐰 A doc was leaking where it shouldn't be,
In nested functions wild and free,
We test it now, the fix is clear,
No __doc__ shadows shall appear!
Co_names clean, hooray, hooray! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding a regression test for issue #4505, which directly reflects the content of the changeset.
Linked Issues check	✅ Passed	The PR adds test cases that directly match the two reproduction scenarios from issue #4505 (nested function and module compilation), fulfilling the objective to detect the `__doc__` leakage regression.
Out of Scope Changes check	✅ Passed	All changes are scoped to adding regression tests in builtin_type.py as requested in comments; no unrelated modifications are present.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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 and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

extra_tests/snippets/code_co_names.py (2)

5-10: Core nested-function assertion matches the reported CPython behavior. This should catch the __doc__-in-co_names regression for nested defs.
Optional: add a blank line between the def foo(): ... block and the module-level assert to avoid potential ruff/PEP8 E305 noise if these snippets are linted.

11-19: Module compile() case is well-targeted; consider hardening style/diagnostics.

Line 13: the blank line inside stmts appears to include a stray space; I’d drop it to avoid “trailing whitespace” tooling complaints.

Consider adding assertion messages (both asserts) so failures clearly show the unexpected co_names value.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75dcf80 and 91f60a9.

📒 Files selected for processing (1)

extra_tests/snippets/code_co_names.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

extra_tests/snippets/code_co_names.py

🧬 Code graph analysis (1)

extra_tests/snippets/code_co_names.py (1)

crates/vm/src/builtins/code.rs (1)

co_names (578-587)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

GitHub Check: Check the WASM package and demo
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (1)

extra_tests/snippets/code_co_names.py (1)

1-3: Good: direct regression linkage to #4505. The header docstring makes the intent/traceability clear.

youknowone · 2025-12-12T11:26:19Z

Oh, I believe this is fixed in #6390 or #6392

Could you add it to builtin_type.py? This is actually specially handled by type.

ShaharNaveh · 2025-12-12T11:39:27Z

extra_tests/snippets/builtin_type.py

 with assert_raises(TypeError):
    del int.__qualname__

-from testutils import assert_raises


Maybe we should also run ruff check --select I in our CI 🤔

how about just put it in auto-format and add some files to blocklist

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

extra_tests/snippets/builtin_type.py (1)

610-628: Regression test matches issue intent; consider making the module co_names assertion less brittle if it flakes.

The nested-function and compile() cases directly cover “__doc__ leaking into co_names”. If CI ever shows ordering/extra-name differences that aren’t __doc__, loosening to assert "__doc__" not in code.co_names plus presence checks for "blah"/"foo" would keep the test focused on #4505.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91f60a9 and ec85b45.

📒 Files selected for processing (1)

extra_tests/snippets/builtin_type.py (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

extra_tests/snippets/builtin_type.py

🧬 Code graph analysis (1)

extra_tests/snippets/builtin_type.py (2)

extra_tests/snippets/testutils.py (1)

assert_raises (5-12)

crates/vm/src/builtins/code.rs (1)

co_names (578-587)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Ensure compilation on various targets
GitHub Check: Run tests under miri
GitHub Check: Check Rust code with clippy

🔇 Additional comments (1)

extra_tests/snippets/builtin_type.py (1)

1-4: Import move is fine; keep it stable per lint conventions.

Add regression test for RustPython#4505

91f60a9

coderabbitai bot reviewed Dec 12, 2025

View reviewed changes

ShaharNaveh added 2 commits December 12, 2025 13:37

Move tests to builtin_type.py

26951d2

Merge remote-tracking branch 'upstream/main' into regr-test-doc-leak

ec85b45

ShaharNaveh commented Dec 12, 2025

View reviewed changes

coderabbitai bot reviewed Dec 12, 2025

View reviewed changes

youknowone approved these changes Dec 12, 2025

View reviewed changes

youknowone merged commit a47b328 into RustPython:main Dec 12, 2025
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add regression test for #4505#6417

Add regression test for #4505#6417
youknowone merged 3 commits intoRustPython:mainfrom
ShaharNaveh:regr-test-doc-leak

ShaharNaveh commented Dec 12, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

youknowone commented Dec 12, 2025

Uh oh!

ShaharNaveh Dec 12, 2025

Uh oh!

youknowone Dec 12, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ShaharNaveh commented Dec 12, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone commented Dec 12, 2025

Uh oh!

ShaharNaveh Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

youknowone Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Dec 12, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 12, 2025 •

edited

Loading