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

 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.

$@ShaharNaveh$

Add regression test for RustPython#4505

91f60a9

$coderabbitai[bot]$

coderabbitai Bot reviewed Dec 12, 2025

View reviewed changes

ShaharNaveh added 2 commits December 12, 2025 13:37

$@ShaharNaveh$

Move tests to builtin_type.py

26951d2

$@ShaharNaveh$

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

ec85b45

$ShaharNaveh$

ShaharNaveh commented Dec 12, 2025

View reviewed changes

$coderabbitai[bot]$

coderabbitai Bot reviewed Dec 12, 2025

View reviewed changes

$youknowone$

youknowone approved these changes Dec 12, 2025

View reviewed changes

$@youknowone$

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

Navigation Menu

Uh oh!

Add regression test for #4505 - #6417

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

$@ShaharNaveh$ ShaharNaveh commented Dec 12, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 12, 2025 •

edited

Loading

Uh oh!

$@coderabbitai$ coderabbitai Bot left a comment

Uh oh!

youknowone commented Dec 12, 2025

Uh oh!

$@ShaharNaveh$ ShaharNaveh Dec 12, 2025

Uh oh!

$@youknowone$ youknowone Dec 12, 2025

Uh oh!

$@coderabbitai$ coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

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$ ShaharNaveh commented Dec 12, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 12, 2025 •

edited

Loading

$@coderabbitai$ coderabbitai Bot left a comment

$@ShaharNaveh$ ShaharNaveh Dec 12, 2025

$@youknowone$ youknowone Dec 12, 2025

$@coderabbitai$ coderabbitai Bot left a comment