doc handing in the right way #6390

youknowone · 2025-12-10T03:03:14Z

Summary by CodeRabbit

Bug Fixes
- Avoid creating unnecessary doc entries for classes without explicit docstrings.
Improvements
- Reduced generated bytecode for classes that have no docstring.
- Slot-driven members now always register as attributes, ensuring consistent overriding across inheritance.
- doc lookup now favors a type's own dictionary for clearer resolution; types missing doc get one set during creation.
Tests
- Tightened class docstring test to use identity check (is None).

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

coderabbitai · 2025-12-10T03:03:20Z

Walkthrough

Class codegen no longer emits a __doc__ entry when no docstring is provided; the load_docstring helper was removed. PyType construction now always creates member descriptors for __slots__ and prefers the type's own dict for __doc__ resolution, setting __doc__ in the type dict when absent.

Changes

Cohort / File(s)	Summary
Class compilation / docstring `crates/codegen/src/compile.rs`	Removed `fn load_docstring(&mut self, doc_str: Option<String>)`; emit `__doc__` only when `Some(doc)` is present (omit write when no docstring)
Type initialization & lookup `crates/vm/src/builtins/type.rs`	Always create and assign `PyMemberDescriptor` entries for `__slots__` members (overriding inherited attrs); set `__doc__` in the type dict if absent; change `__doc__` read to use the type's own dict (`get_direct_attr`) and use descriptor `__get__` when present
Test update `extra_tests/snippets/syntax_class.py`	Change assertion from `A.__doc__ == None` to `A.__doc__ is None` (identity check, clearer failure message)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pay attention to:
- crates/vm/src/builtins/type.rs — verify overriding semantics for slot descriptors and any unintended shadowing of inherited attributes.
- crates/codegen/src/compile.rs — ensure bytecode emitted when omitting __doc__ matches VM expectations.
- Integration between codegen and VM __doc__ behavior (absent vs explicit None in type dict).

Possibly related PRs

type.__dict__ #5957 — Related edits to vm/src/builtins/type.rs affecting type attribute insertion and slot handling.
Fix __dict__ getset type #6010 — Overlapping changes around slot-based descriptors and type initialization behavior.
Fix PyFunction doc behavior #5827 — Prior adjustments to __doc__ handling and descriptor/lookup semantics.

Poem

🐇 I tuck my docstrings neat and light,
Only store them when they’re right.
Slots wear badges, plainly shown,
Own-dict truths are safely known.
Hop, compile — the garden's bright!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title contains a typo ('handing' instead of 'handling') and is vague about the specific improvements made to doc handling across the codebase.	Correct the typo and clarify the title to better describe the changes, such as 'Fix doc handling in class compilation and type initialization' or 'Make doc handling conditional and align with CPython behavior'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ 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 (1)

crates/vm/src/builtins/type.rs (1)
1181-1205: Overriding slots attributes via member descriptors makes sense; small perf nit

Always installing a PyMemberDescriptor for each __slots__ member (even when an inherited attribute exists) is consistent with the idea that defining a slot should override MRO attributes for that name, including inherited descriptors.

A small improvement: you can avoid an extra allocation when interning the attribute name by reusing the existing PyStrRef:
-                let attr_name = vm.ctx.intern_str(member.to_string());
+                let attr_name = vm.ctx.intern_str(member.as_str());
This keeps behavior the same while avoiding a temporary String.

📜 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 c71c78c and 6ce88e0.

⛔ Files ignored due to path filters (1)

Lib/test/test_typing.py is excluded by !Lib/**

📒 Files selected for processing (3)

crates/codegen/src/compile.rs (1 hunks)
crates/vm/src/builtins/type.rs (2 hunks)
extra_tests/snippets/syntax_class.py (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.rs

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

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/codegen/src/compile.rs
crates/vm/src/builtins/type.rs

**/*.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/syntax_class.py

🧬 Code graph analysis (1)

extra_tests/snippets/syntax_class.py (1)

extra_tests/snippets/builtin_type.py (5)

A (76-77)

A (159-160)

A (209-210)

A (240-241)

A (605-606)

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

🔇 Additional comments (3)

extra_tests/snippets/syntax_class.py (1)

189-189: Docstring assertion is now more idiomatic and debuggable

Using is None avoids equality quirks, and including A.__doc__ as the assertion message will make failures easier to diagnose. Looks good.

crates/codegen/src/compile.rs (1)

2667-2687: Class-body doc emission is now consistent and avoids redundant None assignments

Conditionally storing __doc__ only when split_doc finds an explicit docstring:

Reduces bytecode for classes without docstrings.

Aligns with the type-side behavior where absence of a __doc__ entry means __doc__ should read as None.

Preserves override semantics when the class body later reassigns __doc__.

Looks good.

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

1367-1393: The current implementation correctly matches CPython behavior — __doc__ is not inherited via MRO

CPython's C-API explicitly documents that PyTypeObject.tp_doc is not inherited by subtypes. A subclass must supply its own docstring if one is desired; the base class's __doc__ is not used. The code's restriction to get_direct_attr("__doc__") with no MRO traversal is correct and intentional per CPython's specification.

Likely an incorrect or invalid review comment.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/vm/src/builtins/type.rs (1)
1238-1246: LGTM! Correctly ensures every type has a __doc__ entry.

The implementation properly aligns with CPython's behavior by unconditionally adding a __doc__ entry to the type's dict during construction.

Optional micro-optimization: Consider using entry().or_insert() to avoid the check-then-set pattern:
-        {
-            let __doc__ = identifier!(vm, __doc__);
-            if !typ.attributes.read().contains_key(&__doc__) {
-                typ.attributes.write().insert(__doc__, vm.ctx.none());
-            }
-        }
+        {
+            let __doc__ = identifier!(vm, __doc__);
+            typ.attributes.write().entry(__doc__).or_insert(vm.ctx.none());
+        }
This is a minor improvement and not critical since this code runs during type construction before the type is exposed to other threads.

📜 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 1890665 and 32a51b1.

⛔ Files ignored due to path filters (2)

Lib/test/test_property.py is excluded by !Lib/**
Lib/test/test_typing.py is excluded by !Lib/**

📒 Files selected for processing (3)

crates/codegen/src/compile.rs (1 hunks)
crates/vm/src/builtins/type.rs (3 hunks)
extra_tests/snippets/syntax_class.py (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

extra_tests/snippets/syntax_class.py
crates/codegen/src/compile.rs

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

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

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/vm/src/builtins/type.rs

⏰ 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). (10)

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

🔇 Additional comments (2)

crates/vm/src/builtins/type.rs (2)

1201-1204: LGTM! Clear slot descriptor override behavior.

The implementation correctly ensures that __slots__ attributes always get a member descriptor that overrides any inherited attribute from the MRO. The explicit comment clarifies the intended behavior.

1389-1391: LGTM! Correct __doc__ resolution behavior.

The implementation correctly retrieves __doc__ from the type's own dict (not MRO) using get_direct_attr, which aligns with CPython's behavior. The updated comments clearly explain the intended behavior and the distinction from MRO-based lookup.

youknowone marked this pull request as ready for review December 10, 2025 03:03

youknowone force-pushed the __doc__ branch from 185a1ef to 6ce88e0 Compare December 10, 2025 06:01

coderabbitai bot reviewed Dec 10, 2025

View reviewed changes

youknowone force-pushed the __doc__ branch from 1890665 to a81c42d Compare December 10, 2025 10:24

youknowone added 2 commits December 10, 2025 19:26

__doc__ handing in the right way

7670655

fixit

32a51b1

youknowone force-pushed the __doc__ branch from a81c42d to 32a51b1 Compare December 10, 2025 10:26

coderabbitai bot reviewed Dec 10, 2025

View reviewed changes

youknowone merged commit 98fff96 into RustPython:main Dec 10, 2025
13 checks passed

youknowone deleted the __doc__ branch December 10, 2025 11:04

youknowone mentioned this pull request Dec 12, 2025

Add regression test for #4505 #6417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

doc handing in the right way #6390

doc handing in the right way #6390

Uh oh!

youknowone commented Dec 10, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 10, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

__doc__ handing in the right way #6390

__doc__ handing in the right way #6390

Uh oh!

Conversation

youknowone commented Dec 10, 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 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

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!

1 participant

doc handing in the right way #6390

doc handing in the right way #6390

youknowone commented Dec 10, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 10, 2025 •

edited

Loading