overridable validate_downcastable_from by youknowone · Pull Request #6393 · RustPython/RustPython
Walkthrough
Updates PyPayload trait to introduce a validate_downcastable_from extension point for custom downcast validation. Modifies PyUtf8Str to use this hook for UTF-8 validation without duplicate typeid checks. Updates PyNativeMethod to leverage the pyclass ctx parameter mechanism for type context.
Changes
| Cohort / File(s) | Change Summary |
|---|---|
PyPayload Trait Extension crates/vm/src/object/payload.rs |
Adds new validate_downcastable_from(&obj) -> bool default method to PyPayload trait; updates downcastable_from() to require both typeid matching and custom validation check. |
Builtin Function Type crates/vm/src/builtins/builtin_func.rs |
Adds ctx = "builtin_method_type" parameter to PyNativeMethod pyclass declaration; removes PyPayload impl block that provided custom class(ctx) lookup. |
String Type Validation crates/vm/src/builtins/str.rs |
Renames PyUtf8Str's PyPayload method from downcastable_from() to validate_downcastable_from(); removes duplicate typeid check and relies on parent trait's typeid validation. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Trait method addition and its usage across implementations: Verify that the new
validate_downcastable_fromhook is correctly applied and doesn't introduce unintended behavior. - PyUtf8Str downcast logic change: Ensure removal of typeid check is safe since parent
downcastable_from()now handles it. - PyNativeMethod ctx parameter: Confirm that the ctx mechanism properly resolves the builtin_method_type and that removing the custom PyPayload impl is safe.
Possibly related PRs
- Wtf8-compatible fixes #5985: Modifies VM downcasting logic with different approach to typeid comparison, potentially overlapping concerns.
- Refactor PyUtf8Str #6047: Directly related through changes to PyUtf8Str and PyPayload downcast hooks in the same files.
- Separate Debug from PyPayload #6320: Modifies PyPayload trait bounds and Debug requirements in the same core trait.
Suggested reviewers
- coolreader18
- ShaharNaveh
Poem
🐰 A validation hook, oh what a delight,
No typeid checks done twice in the night,
PyUtf8Str trusts and validates right,
PyNativeMethod's context shines bright,
Downcasting now flows with less fright! ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'overridable validate_downcastable_from' directly and specifically describes the main change: introducing an overridable validate_downcastable_from method as a new extension point in PyPayload. |
✨ 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.
Comment @coderabbitai help to get the list of available commands and usage tips.