◐ Shell
reader mode source ↗
Skip to content

Update test_{descr,super}.py from 3.13.9#6314

Merged
youknowone merged 3 commits into
RustPython:mainfrom
ShaharNaveh:update-test-desc-super
Nov 30, 2025
Merged

Update test_{descr,super}.py from 3.13.9#6314
youknowone merged 3 commits into
RustPython:mainfrom
ShaharNaveh:update-test-desc-super

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Nov 29, 2025

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of super() when given mismatched type/instance combinations to avoid spurious checks and preserve existing subclass/instance routing.
    • Enhanced error messages for incompatible super(type, obj) calls, providing clearer descriptions of whether obj is a type or instance and naming the offending types for easier troubleshooting.

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

@coderabbitai

coderabbitai Bot commented Nov 29, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

Reworked super_check in crates/vm/src/builtins/super.rs to add an initial typ-based path for PyType detection, preserve existing isinstance/class routing, and produce a more descriptive error when obj is not an instance or subtype of the provided type.

Changes

Cohort / File(s) Summary
Enhanced super() type checking
crates/vm/src/builtins/super.rs
Add an initial typ extraction path to detect PyType without immediate subclass check; keep existing isinstance and __class__ routing; build a clearer error message reporting whether obj is a type or an instance and include type names; no other control-flow changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect the new typ-extraction logic and its interaction with the existing subclass checks.
  • Verify correctness and safety of downcasts used to obtain type names for error messages.
  • Confirm that existing isinstance and __class__-based paths behave identically after the change.

Suggested reviewers

  • youknowone

Poem

🐰 I hopped through code where types entwine,

peered at classes, instance and sign.
Now errors chant what once was blind,
“Type or instance?” — the answer aligned.
A joyful twitch, a clearer design.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title mentions updating test files from 3.13.9, but the actual changes are in super.rs with logic modifications to super_check and error handling. Revise the title to reflect the actual changes: e.g., 'Improve super_check error reporting and type validation logic' or 'Refactor super_check downcast handling and error messages'.
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 (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

📜 Recent 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 c2ea003 and 7c86523.

📒 Files selected for processing (1)
  • crates/vm/src/builtins/super.rs (1 hunks)
🧰 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/super.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). (11)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
crates/vm/src/builtins/super.rs (1)

242-270: I'm unable to verify the review comment because the repository failed to clone. Without access to the actual file crates/vm/src/builtins/super.rs, I cannot confirm:

  1. Whether the code snippet provided matches the current state of the file
  2. Whether unsafe downcasts were actually eliminated as claimed
  3. Whether the error message construction properly reuses the stored typ value
  4. Whether previous review concerns were genuinely addressed

Additionally, the review comment requests verification using cargo fmt and cargo clippy, which cannot be executed in the sandbox environment (Rust toolchain is unavailable).

Please provide the repository or file contents directly so I can complete the verification. Alternatively, if you have access to run the verification commands in your environment, please share those results.


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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Actionable comments posted: 1

📜 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 6782fa2 and c2ea003.

⛔ Files ignored due to path filters (2)
  • Lib/test/test_descr.py is excluded by !Lib/**
  • Lib/test/test_super.py is excluded by !Lib/**
📒 Files selected for processing (1)
  • crates/vm/src/builtins/super.rs (1 hunks)
🧰 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/super.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). (11)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run tests under miri
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
crates/vm/src/builtins/super.rs (1)

269-274: LGTM! Enhanced error message provides better debugging information.

The error message now clearly distinguishes between type and instance arguments and includes their names, making it much easier to diagnose super() misuse.

@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.

Hide comment

👍

Hide details View details @youknowone youknowone merged commit b84f7c1 into RustPython:main Nov 30, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants