PyStructSequence Compatibility #6327
Conversation
WalkthroughMigrate PyStructSequence from derive-based macros to attribute-based macros and a data-driven model: introduce Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Potential hotspots to focus on:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
f713c47 to
3c1d284
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/derive-impl/src/pymodule.rs (1)
270-324: Allow#[pyattr]to precede#[pystruct_sequence]like other type itemsIn
attrs_to_module_items, whenpy_attrsis non-empty you only acceptClass | Function | Exception:match attr_name { AttrName::Class | AttrName::Function | AttrName::Exception => { ... } _ => bail!("...[pyclass], #[pyfunction], or #[pyexception] can follow #[pyattr]"), }This rejects patterns like:
#[pyattr] #[pystruct_sequence(name = "stat_result", module = "os", data = "StatResultData")] struct PyStatResult;even though
StructSequenceItemis designed to useself.py_attrsfor custom export names.Consider treating
StructSequencelike the other type-defining attributes here:- match attr_name { - AttrName::Class | AttrName::Function | AttrName::Exception => { + match attr_name { + AttrName::Class | AttrName::Function | AttrName::Exception + | AttrName::StructSequence => { result.push(item_new(i, attr_name, py_attrs.clone())); }This preserves the existing requirement that only one grouped
#[pyattr]applies while enabling#[pyattr]+#[pystruct_sequence]combinations.
🧹 Nitpick comments (5)
crates/derive-impl/src/util.rs (1)
11-25: Remove duplicate"pystruct_sequence"fromALL_ALLOWED_NAMES
ALL_ALLOWED_NAMESlists"pystruct_sequence"twice, which is redundant and may confuse future readers, even thoughcontains()semantics are unchanged.You can simplify the constant like this:
pub(crate) const ALL_ALLOWED_NAMES: &[&str] = &[ @@ - "pystruct_sequence", - "pystruct_sequence", + "pystruct_sequence", "pyattr",crates/derive-impl/src/pymodule.rs (1)
412-417: Re-use existingpystruct_sequencemeta parsing to avoid duplication
StructSequenceItem::gen_module_itemmanually re-parses#[pystruct_sequence(...)]to extractname,module, andno_attr, while similar logic already exists in thePyStructSequenceMetamachinery inpystructseq.rs.To keep behavior in sync and reduce maintenance risk, consider refactoring
gen_module_itemto delegate to the shared meta type (or a small helper built onItemMetaInner) rather than duplicating parsing here. This would also ensure any future options added to#[pystruct_sequence]are handled consistently in both the macro expansion and module wiring.Also applies to: 626-729
crates/derive-impl/src/pystructseq.rs (3)
10-65: Field parsing and classification are clear; consider validating conflicting pystruct options
FieldKind/FieldInfoplusparse_fieldsgive a nice, centralized view over named/unnamed/skipped fields, and stripping#[pystruct(..)]off the synthesized input avoids leaking helper attrs into downstream derives.One edge case you might want to guard against is a field marked with both
#[pystruct(skip)]and#[pystruct(unnamed)]. Currently that would silently prioritiseskipvia:let kind = if skip { FieldKind::Skipped } else if unnamed { FieldKind::Unnamed } else { FieldKind::Named };If such a combination is considered invalid, explicitly bailing with a helpful error (rather than silently picking one) would make misuse easier to diagnose.
Also applies to: 67-144
166-285: Strengthen #[pystruct_sequence_data] codegen for generics and TryFromObject semanticsThe overall structure of
impl_pystruct_sequence_datalooks good, but there are a few details worth tightening:
Generics on data structs are currently ignored
DeriveInputcarriesinput.generics, but the generated impls use a bare#data_ident:impl #data_ident { ... } impl ::rustpython_vm::types::PyStructSequenceData for #data_ident { ... }For a generic data struct (even though you don’t have one today), this will fail to compile. It’d be more robust to thread generics through:
let generics = input.generics.clone(); let (impl_generics, ty_generics, where_clause) = generics.split_for_impl(); impl #impl_generics #data_ident #ty_generics #where_clause { ... } impl #impl_generics ::rustpython_vm::types::PyStructSequenceData for #data_ident #ty_generics #where_clause { ... }
TryFromObjectusesvisible_fields.len()as the minimum lengthThe current check:
let n_required = visible_fields.len(); if seq.len() < #n_required { ... }treats all visible fields (named + unnamed) as mandatory. Conceptually, only the named fields are truly “required”; unnamed fields are typically the tail-only, sequence-only extras. If you ever combine
#[pystruct(unnamed)]withtry_from_object, usingnamed_fields.len()as the lower bound would more closely match the separation encoded inFieldInfoand theREQUIRED_FIELD_NAMESconstant.Avoid extra cloning in
try_from_elementsInside the generated
try_from_elementsbody you do:#(#visible_fields: iter.next().unwrap().clone().try_into_value(vm)?,)* #(#skipped_fields: match iter.next() { Some(v) => v.clone().try_into_value(vm)?, None => vm.ctx.none(), },)*Because
iterowns the vector,iter.next()already yields an ownedPyObjectRef; the.clone()is redundant. You can drop the clones to avoid extra Arc bumps:#(#visible_fields: iter.next().unwrap().try_into_value(vm)?,)* #(#skipped_fields: match iter.next() { Some(v) => v.try_into_value(vm)?, None => vm.ctx.none(), },)*None of these are blockers, but addressing them would make the macro path more future-proof.
303-394: PyStructSequenceMeta and impl_pystruct_sequence wiring look soundThe meta parser correctly restricts
#[pystruct_sequence(...)]to known keys (name,module,data,no_attr) and enforces string values forname/module/data, with a clear error ifdatais missing. The generatedPyClassDef/StaticType/MaybeTraverse/PyStructSequence/ToPyObjectimpls form a clean, minimal wrapper over the data struct and basePyTuple.Given current usage, constraining
datato a simple identifier viaformat_ident!is fine, but if you ever need to support a non-local path (e.g.,data = "time::StructTimeData"), this will need to be extended to parse a path instead of a bare ident. For now, the approach is straightforward and keeps the attribute surface tight.Also applies to: 406-494
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/datetimetester.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_structseq.pyis excluded by!Lib/**
📒 Files selected for processing (17)
crates/derive-impl/src/lib.rs(1 hunks)crates/derive-impl/src/pymodule.rs(7 hunks)crates/derive-impl/src/pystructseq.rs(3 hunks)crates/derive-impl/src/util.rs(1 hunks)crates/derive/src/lib.rs(1 hunks)crates/stdlib/src/grp.rs(5 hunks)crates/stdlib/src/resource.rs(4 hunks)crates/vm/src/stdlib/nt.rs(2 hunks)crates/vm/src/stdlib/os.rs(9 hunks)crates/vm/src/stdlib/posix.rs(3 hunks)crates/vm/src/stdlib/pwd.rs(7 hunks)crates/vm/src/stdlib/sys.rs(21 hunks)crates/vm/src/stdlib/time.rs(7 hunks)crates/vm/src/types/mod.rs(1 hunks)crates/vm/src/types/structseq.rs(4 hunks)crates/vm/src/vm/context.rs(1 hunks)crates/vm/src/vm/mod.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 runningcargo fmtto 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/types/mod.rscrates/vm/src/stdlib/posix.rscrates/derive-impl/src/util.rscrates/vm/src/vm/context.rscrates/vm/src/stdlib/nt.rscrates/derive-impl/src/pymodule.rscrates/derive-impl/src/lib.rscrates/vm/src/vm/mod.rscrates/stdlib/src/resource.rscrates/derive/src/lib.rscrates/stdlib/src/grp.rscrates/vm/src/stdlib/pwd.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/time.rscrates/derive-impl/src/pystructseq.rscrates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Applied to files:
crates/vm/src/types/mod.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Applied to files:
crates/vm/src/types/mod.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/derive/src/lib.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
🧬 Code graph analysis (8)
crates/vm/src/types/mod.rs (1)
crates/vm/src/types/structseq.rs (1)
struct_sequence_new(20-69)
crates/vm/src/vm/context.rs (1)
crates/derive-impl/src/pystructseq.rs (1)
n_unnamed_fields(58-63)
crates/derive-impl/src/pymodule.rs (1)
crates/derive-impl/src/pystructseq.rs (3)
inner(314-316)class_name(320-342)module(344-366)
crates/derive-impl/src/lib.rs (2)
crates/derive/src/lib.rs (2)
pystruct_sequence(235-239)pystruct_sequence_data(264-268)crates/derive-impl/src/pystructseq.rs (2)
impl_pystruct_sequence(406-494)impl_pystruct_sequence_data(166-299)
crates/stdlib/src/resource.rs (3)
crates/derive/src/lib.rs (2)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)crates/derive-impl/src/pystructseq.rs (1)
module(344-366)crates/derive-impl/src/util.rs (2)
module(429-458)with(335-347)
crates/derive/src/lib.rs (1)
crates/derive-impl/src/util.rs (1)
with(335-347)
crates/vm/src/stdlib/time.rs (2)
crates/vm/src/types/structseq.rs (2)
struct_sequence_new(20-69)Self(280-283)crates/vm/src/vm/mod.rs (1)
new(116-219)
crates/vm/src/stdlib/sys.rs (6)
crates/vm/src/types/structseq.rs (2)
from_data(186-191)Self(280-283)crates/derive-impl/src/lib.rs (2)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)crates/derive/src/lib.rs (2)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)crates/vm/src/stdlib/os.rs (3)
name(480-482)try_from_object(127-130)try_from_object(134-137)crates/derive-impl/src/pystructseq.rs (1)
module(344-366)crates/vm/src/stdlib/posix.rs (7)
try_from_object(282-290)try_from_object(294-302)try_from_object(783-787)try_from_object(1268-1270)try_from_object(1274-1276)try_from_object(1912-1924)try_from_object(2306-2321)
🔇 Additional comments (43)
crates/stdlib/src/grp.rs (4)
16-28: Clean separation of data and Python wrapper.The new pattern correctly separates the data struct (
GroupData) from the Python-exposed wrapper (PyGroup), with proper linkage via thedata = "GroupData"attribute. This aligns well with the macro system approach for implementing Python functionality.
30-45: Correct conversion logic with graceful UTF-8 handling.The
cstr_lossyhelper properly handles potential UTF-8 conversion failures with a lossy fallback. The field mappings correctly translate from the nixGroupstructure toGroupData.
47-83: Proper error handling and return type adaptation.Both functions correctly return
GroupDatadirectly, with the macro-generated traits handling the conversion to Python objects. Error handling is appropriate:
KeyErrorfor missing group entries with descriptive messages- Null byte validation in
getgrnamprevents C string issues- Nix errors properly converted via
IntoPyException
85-101: Thread-safe enumeration with proper unsafe handling.Good implementation:
- Static mutex correctly protects non-thread-safe
setgrent/getgrent/endgrentcallsNonNull::newproperly handles null pointer checks for iteration termination- Explicit
.to_pyobject(vm)is correctly used here since the return type isVec<PyObjectRef>rather than a data struct that gets auto-convertedcrates/vm/src/vm/context.rs (1)
248-250: LGTM!The new interned constant names (
n_fields,n_sequence_fields,n_unnamed_fields) correctly support the PyStructSequence infrastructure. These align with CPython's struct sequence attributes for tracking field counts and visibility.crates/vm/src/vm/mod.rs (1)
468-475: LGTM!The switch from
UnraisableHookArgstoUnraisableHookArgsDataaligns with the data-centric refactoring pattern. The data struct will be automatically converted to a Python object when passed tounraisablehook.call()via theToPyObjecttrait implementation generated by#[pystruct_sequence_data].crates/vm/src/stdlib/pwd.rs (5)
19-34: LGTM!The data-centric refactoring is well-structured:
PasswdDataholds the raw field data with#[pystruct_sequence_data]PyPasswdserves as the Python-facing wrapper with#[pystruct_sequence(name = "struct_passwd", module = "pwd", data = "PasswdData")]- The empty
#[pyclass(with(PyStructSequence))]impl correctly delegates struct sequence behavior to the macro-generated codeThis pattern is consistent with other refactored modules (grp, resource, os, sys).
36-58: LGTM!The
From<User>implementation correctly targetsPasswdDatanow. The lossy string conversion helpers remain appropriate for handling non-UTF8 user data gracefully.
60-75: LGTM!The return type change to
PyResult<PasswdData>is correct. The macro-generatedToPyObjectimplementation handles conversion to Python objects automatically when the function result is used.
77-95: LGTM!Consistent with
getpwnam—returnsPyResult<PasswdData>with automatic Python object conversion.
99-115: LGTM!The explicit
.to_pyobject(vm)call is necessary here since the function returnsVec<PyObjectRef>, requiring manual conversion of eachPasswdDatainstance.crates/stdlib/src/resource.rs (3)
66-90: LGTM!The data-centric refactoring follows the established pattern:
RusageDatawith#[pystruct_sequence_data]contains all 16 rusage fieldsPyRusagewith#[pystruct_sequence(name = "struct_rusage", module = "resource", data = "RusageData")]provides the Python wrapper- Empty
#[pyclass(with(PyStructSequence))]impl delegates to macro-generated behaviorThe field types correctly match the libc rusage structure types (
f64for time values,libc::c_longfor counters).
92-114: LGTM!The
From<libc::rusage>implementation is clean. Thetvclosure correctly convertstimevaltof64by combining seconds and microseconds.
116-133: LGTM!Clean functional chain:
res.map(RusageData::from)converts the raw rusage to the data struct, with appropriate error mapping for invalid input.crates/vm/src/stdlib/sys.rs (16)
3-3: LGTM!The public export of
UnraisableHookArgsDataaligns with the data-centric pattern—callers (likeVirtualMachine::run_unraisable) construct the data struct directly.
452-458: LGTM!The
from_datapattern cleanly separates data construction from Python object creation.FlagsData::from_settingsandFloatInfoData::INFOprovide the data, andPyXxx::from_data()handles the conversion.
659-672: LGTM!The Windows version handling correctly constructs
WindowsVersionDatawith all required fields and usesPyWindowsVersion::from_data()for the final conversion.
674-741: LGTM!The
_unraisablehookandunraisablehookfunctions correctly acceptUnraisableHookArgsDatadirectly. The implementation logic remains unchanged—only the input type has been updated to match the data-centric pattern.
744-756: LGTM!Consistent usage of
from_datapattern forhash_infoandint_infowith their respectiveINFOconstants.
765-765: LGTM!Correctly accesses
str_digits_check_thresholdfromIntInfoData::INFOfor validation.
814-821: LGTM!Consistent pattern for
thread_infoandversion_infousing their respective Data struct constants.
901-923: LGTM!
AsyncgenHooksDataandPyAsyncgenHooksfollow the established pattern. Theget_asyncgen_hooksfunction returnsAsyncgenHooksDatadirectly with automatic Python conversion.
925-1003: LGTM!The
FlagsDatastruct withfrom_settingsmethod cleanly encapsulates flag extraction fromSettings. Theno_attroption in#[pystruct_sequence(...)]and theslot_newimplementation preventing direct instantiation are appropriate for this read-only system type.
1005-1030: LGTM!
ThreadInfoDataandPyThreadInfofollow the pattern withINFOconstant providing static thread configuration. The#[cfg(feature = "threading")]guards are correctly applied to both the data struct and wrapper.
1032-1067: LGTM!
FloatInfoDatacorrectly captures all f64 limit constants. TheINFOconstant provides compile-time values matching Python'ssys.float_info.
1069-1103: LGTM!
HashInfoDataproperly exposes hash algorithm parameters. TheINFOconstant correctly uses values fromrustpython_common::hash.
1105-1126: LGTM!
IntInfoDatacorrectly defines integer representation parameters including thestr_digits_check_thresholdused for validation inset_int_max_str_digits.
1128-1161: LGTM!
VersionInfoDataandPyVersionInfocorrectly expose version information. TheDefaultderive andslot_newpreventing instantiation are appropriate for this system type.
1163-1185: LGTM!
WindowsVersionDatacorrectly captures all Windows version fields including theplatform_versiontuple. The#[cfg(windows)]guards are properly applied.
1187-1201: LGTM!
UnraisableHookArgsDatawithtry_from_objectattribute enables conversion from Python objects, which is needed when Python code callsunraisablehookdirectly. The fields correctly match CPython'sUnraisableHookArgs.crates/vm/src/types/mod.rs (1)
6-6: Struct sequence exports are consistent with new runtime APIRe-exporting
PyStructSequence,PyStructSequenceData, andstruct_sequence_newfromstructseqcleanly exposes the new struct-sequence runtime surface without changing behavior in this module.crates/vm/src/stdlib/nt.rs (1)
178-210: get_terminal_size migration toTerminalSizeDatalooks correctThe function now returns
_os::TerminalSizeData { columns, lines }instead of aPyTerminalSizewrapper, which matches the new data-backed struct-sequence pattern; the Windows logic and field values are otherwise unchanged.crates/vm/src/stdlib/posix.rs (2)
1332-1341:unamereturningUnameResultDataaligns with data-struct patternSwitching the return type to
_os::UnameResultDataand constructing it field-for-field fromuname::uname()preserves behavior while fitting the new data-backed struct-sequence scheme.
1730-1747: POSIXget_terminal_sizenow returningTerminalSizeDatais consistentThe function now returns
_os::TerminalSizeData { columns, lines }instead of aPyTerminalSizewrapper, with identical ioctl and field handling. This matches the NT-side and the new struct-sequence Data pattern.crates/derive-impl/src/lib.rs (1)
61-67: Newpystruct_sequence*entry points are wired correctlyBoth
pystruct_sequenceandpystruct_sequence_datamirror the existingpyclass/pymodulepattern, forwarding(attr, item)topystructseq::{impl_pystruct_sequence, impl_pystruct_sequence_data}viaresult_to_tokens, so the derive-impl surface stays consistent.crates/derive/src/lib.rs (1)
217-268:#[pystruct_sequence]/#[pystruct_sequence_data]macros and docs look goodThe new attribute macros correctly parse
attras aPunctuatedlist and delegate toderive_impl::pystruct_sequence/pystruct_sequence_data, matching the style ofpyclass/pymodule. The examples accurately reflect the intended “Data + Py type” usage and thetry_from_objectoption.crates/vm/src/types/structseq.rs (1)
16-69: Struct-sequence runtime design matches the intended Data + PyType model
struct_sequence_new,PyStructSequenceData, andPyStructSequence(withextend_pyclass) nicely encapsulate the CPython-style structseq behavior: visible vs hidden fields,n_sequence_fields/n_fields/n_unnamed_fields, class-level getters, and slot overrides for sequence/mapping/iter/hash/repr. The ToPyObject impl generated for Data structs will integrate cleanly with this API.Also applies to: 151-325
crates/vm/src/stdlib/time.rs (3)
298-305: StructTimeData argument handling for asctime/ctime/strftime looks consistentUsing
OptionalArg<StructTimeData>::naive_or_localand reusing it inasctime,ctime, andstrftimecleanly centralizes “treat struct_time as local time” semantics and preserves thet-optional behaviour. The conversion viaStructTimeData::to_date_timeis straightforward and matches the existing float/seconds handling.Also applies to: 343-374
329-338: mktime() ignores tm_gmtoff/tm_zone and relies solely on local time rules
mktime()now convertsStructTimeDataback to aNaiveDateTimeviato_date_time()(using only the integer Y/M/D/h/m/s fields) and then feeds that intochrono::Local.from_local_datetime(..).single(). This matches CPython’s documented “interpret as local time” behaviour and correctly mapsOverflowErrorwhen the local conversion fails.One behavioural edge case is ambiguous/nonexistent local times around DST transitions:
Local::from_local_datetime(...).single()will turn “ambiguous” or “nonexistent” intoOverflowError. If we ever want closer parity with CPython’s use of Cmktime()(which typically picks one of the candidates rather than erroring), we may need additional handling, but that’s likely out of scope for this refactor and can be revisited based on test results.Please run the existing
time.mktimetests (especially any DST-boundary cases) to confirm this still matches RustPython’s previous behaviour.Also applies to: 524-540
543-552: PyStructTime wrapper and slot_new wiring look correctThe new
PyStructTimewrapper is minimal and delegates construction tostruct_sequence_new(cls, seq, vm), which respectsn_sequence_fields/n_fieldson the type and preservescls(better for subclassing / future-proofing than the previous_cls-ignoring slot). The combination of#[pystruct_sequence(.., data = "StructTimeData")]and#[pyclass(with(PyStructSequence))]matches the intended macro pattern.crates/vm/src/stdlib/os.rs (3)
738-835: StatResultData struct and stat()/PyStatResult wiring look internally consistentThe new
StatResultDatacleanly separates:
- Core integer fields (
st_mode,st_ino, ...,st_size) as named fields.- Integer seconds (
st_atime_int,st_mtime_int,st_ctime_int) as#[pystruct(unnamed)]with FromArgs defaults.- Derived float times / nanosecond fields /
st_reparse_tagas#[pystruct(skip)].
from_statcovers all supported platforms, normalises(sec, nsec)into both float seconds and nanoseconds, and gatesst_reparse_tagbehind#[cfg(windows)]. The updatedstat()just returnsStatResultData::from_stat(...).to_pyobject(vm), which will go through the newPyStatResultstruct-sequence wrapper, andPyStatResult::slot_newpreserves CPython-likeos.stat_result((...))flattening before binding viaFromArgs.Given the subtlety of os.stat_result’s sequence vs attribute fields, it’s worth running the existing os.stat/os.fstat tests (including indexing, named attributes, and platform-specific fields like *_ns and st_reparse_tag) to confirm layout parity, but structurally this refactor looks sound.
Also applies to: 928-940
1233-1317: TimesResultData and times() refactor are straightforwardOn both Windows and Unix,
times()now builds a plainTimesResultDatawith the expected five floats (user, system, children_user, children_system, elapsed) and returns it viato_pyobject(vm), relying on the#[pystruct_sequence(name = "times_result", module = "os", data = "TimesResultData")]+#[pyclass(with(PyStructSequence))]wiring to materialise the Pythonos.times_resultstruct sequence.The platform-specific computations (FILETIME scaling on Windows;
tms/sysconf(_SC_CLK_TCK)on Unix) remain unchanged and only the wrapping changed, so behaviour should be identical aside from the internal representation.Please run the os.times-related tests to confirm repr/indexing and field names still match CPython.
1480-1508: TerminalSizeData and UnameResultData wrappers match the new struct-sequence pattern
TerminalSizeDataandUnameResultDataare now simple Rust data structs annotated with#[pystruct_sequence_data], paired with correspondingPyTerminalSizeandPyUnameResultwrappers via#[pystruct_sequence(name = "...", module = "os", data = "...")]and#[pyclass(with(PyStructSequence))]. This aligns with the new macro model and keeps the Python-visible types (os.terminal_size,os.uname_result) thin while centralising their field layouts in the data structs.Barring any pre-existing quirks in how these types are exposed from the module, the refactor looks good and should be functionally neutral.
| item: atomic_func!(|seq, i, vm| { | ||
| let n_seq = get_visible_len(seq.obj, vm)?; | ||
| let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap(); | ||
| let idx = if i < 0 { | ||
| let pos_i = n_seq as isize + i; | ||
| if pos_i < 0 { | ||
| return Err(vm.new_index_error("tuple index out of range")); | ||
| } | ||
| pos_i as usize | ||
| } else { | ||
| i as usize | ||
| }; | ||
| if idx >= n_seq { | ||
| return Err(vm.new_index_error("tuple index out of range")); | ||
| } | ||
| Ok(tuple[idx].clone()) | ||
| }), |
There was a problem hiding this comment.
Clamp visible length to avoid panics if n_sequence_fields is mutated from Python
Several operations assume n_sequence_fields <= tuple.len() and index/slice using it directly:
STRUCT_SEQUENCE_AS_SEQUENCE.item: indexestuple[idx]after only checkingidx < n_seq.STRUCT_SEQUENCE_AS_MAPPING.subscript: doestuple.as_slice()[..n_seq].PyStructSequence::__getitem__: also sliceszelf.as_slice()[..n_seq].struct_sequence_richcompare: slices bothzelf_tupleandother_tupleup to lengths derived fromn_sequence_fields.
Because n_sequence_fields is exposed as a writable Python attribute, user code can do:
time.struct_time.n_sequence_fields = 10**9
time.gmtime()[0] # or slicing / subscriptingand trigger an out-of-bounds slice or index, causing the Rust runtime to panic.
To harden this, treat n_sequence_fields as an upper bound and clamp it to the actual tuple length before slicing or indexing. For example:
// in STRUCT_SEQUENCE_AS_SEQUENCE.item
- item: atomic_func!(|seq, i, vm| {
- let n_seq = get_visible_len(seq.obj, vm)?;
- let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap();
+ item: atomic_func!(|seq, i, vm| {
+ let n_seq = get_visible_len(seq.obj, vm)?;
+ let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap();
+ let visible_len = n_seq.min(tuple.len());
let idx = if i < 0 {
- let pos_i = n_seq as isize + i;
+ let pos_i = visible_len as isize + i;
if pos_i < 0 {
return Err(vm.new_index_error("tuple index out of range"));
}
pos_i as usize
} else {
i as usize
};
- if idx >= n_seq {
+ if idx >= visible_len {
return Err(vm.new_index_error("tuple index out of range"));
}
Ok(tuple[idx].clone())
}), // in STRUCT_SEQUENCE_AS_MAPPING.subscript
- subscript: atomic_func!(|mapping, needle, vm| {
- let n_seq = get_visible_len(mapping.obj, vm)?;
- let tuple = mapping.obj.downcast_ref::<PyTuple>().unwrap();
- let visible_elements = &tuple.as_slice()[..n_seq];
+ subscript: atomic_func!(|mapping, needle, vm| {
+ let n_seq = get_visible_len(mapping.obj, vm)?;
+ let tuple = mapping.obj.downcast_ref::<PyTuple>().unwrap();
+ let visible_len = n_seq.min(tuple.len());
+ let visible_elements = &tuple.as_slice()[..visible_len]; // in __getitem__
- let n_seq = get_visible_len(zelf.as_ref(), vm)?;
- let visible_elements = &zelf.as_slice()[..n_seq];
+ let n_seq = get_visible_len(zelf.as_ref(), vm)?;
+ let visible_len = n_seq.min(zelf.len());
+ let visible_elements = &zelf.as_slice()[..visible_len]; // in struct_sequence_richcompare
- let zelf_len = get_visible_len(zelf, vm)?;
- // For other, try to get visible len; if it fails (not a struct sequence), use full length
- let other_len = get_visible_len(other, vm).unwrap_or(other_tuple.len());
-
- let zelf_visible = &zelf_tuple.as_slice()[..zelf_len];
- let other_visible = &other_tuple.as_slice()[..other_len];
+ let zelf_len = get_visible_len(zelf, vm)?.min(zelf_tuple.len());
+ // For other, try to get visible len; if it fails (not a struct sequence), use full length
+ let other_len = match get_visible_len(other, vm) {
+ Ok(n) => n.min(other_tuple.len()),
+ Err(_) => other_tuple.len(),
+ };
+
+ let zelf_visible = &zelf_tuple.as_slice()[..zelf_len];
+ let other_visible = &other_tuple.as_slice()[..other_len];This keeps semantics the same under normal invariants, but prevents user-modified n_sequence_fields from crashing the interpreter.
Also applies to: 134-147, 235-245, 359-385
🤖 Prompt for AI Agents
In crates/vm/src/types/structseq.rs around lines 101 to 117, the code uses
n_sequence_fields (via get_visible_len) directly for indexing/slicing which can
panic if Python mutates that value larger than the underlying tuple; clamp n_seq
to the tuple length before using it. Replace usages like let n_seq =
get_visible_len(...)? with let n_seq = get_visible_len(...)?; let n_seq =
n_seq.min(tuple.len()); (convert types as needed) and then use this clamped
n_seq for the idx bounds check and any slice ranges. Apply the same clamping fix
at the other mentioned ranges (134-147, 235-245, 359-385) so all indexing and
slicing use min(get_visible_len, tuple.len()) to avoid out-of-bounds panics.
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin structseq |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/derive-impl/src/pymodule.rs (1)
283-321: #[pyattr] cannot currently be combined with #[pystruct_sequence]In
attrs_to_module_items, onlyClass | Function | Exceptionare allowed to follow#[pyattr]. With the newAttrName::StructSequence, patterns like:#[pyattr] #[pystruct_sequence(...)] struct PyStructTime;will hit the “can follow #[pyattr]” error even though
StructSequenceItem::gen_module_itemsupportspy_attrsfor custom names.Consider allowing
AttrName::StructSequencein that match arm and updating the error message accordingly:- match attr_name { - AttrName::Class | AttrName::Function | AttrName::Exception => { + match attr_name { + AttrName::Class | AttrName::Function | AttrName::Exception | AttrName::StructSequence => { result.push(item_new(i, attr_name, py_attrs.clone())); } _ => { bail_span!( attr, - "#[pyclass], #[pyfunction], or #[pyexception] can follow #[pyattr]", + "#[pyclass], #[pyfunction], #[pyexception], or #[pystruct_sequence] can follow #[pyattr]", ) } }
♻️ Duplicate comments (2)
crates/vm/src/stdlib/time.rs (1)
502-523: gmtime() still uses local timezone for tm_gmtoff/tm_zone instead of UTC
StructTimeData::newcurrently derivestm_gmtoffandtm_zonefromchrono::Localfor all callers:fn new(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); let offset_seconds = local_time.offset().local_minus_utc() + if isdst == 1 { 3600 } else { 0 }; let tz_abbr = local_time.format("%Z").to_string(); // ... tm_isdst: vm.ctx.new_int(isdst).into(), tm_gmtoff: vm.ctx.new_int(offset_seconds).into(), tm_zone: vm.ctx.new_str(tz_abbr).into(), }But
gmtime()now callsStructTimeData::new(vm, instant, 0), so itsstruct_timereports the local offset/name instead of UTC. CPython’stime.gmtime()must return a UTC struct_time with:
tm_gmtoff == 0tm_zone == "UTC"(or the platform’s canonical UTC name)tm_isdst == 0This is the same issue previously flagged and remains unresolved.
A minimal fix is to make
StructTimeData::newdistinguish UTC vs local based onisdst(or split into explicit constructors), e.g.:impl StructTimeData { - fn new(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { - let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); - let offset_seconds = - local_time.offset().local_minus_utc() + if isdst == 1 { 3600 } else { 0 }; - let tz_abbr = local_time.format("%Z").to_string(); - - Self { - tm_year: vm.ctx.new_int(tm.year()).into(), - tm_mon: vm.ctx.new_int(tm.month()).into(), - tm_mday: vm.ctx.new_int(tm.day()).into(), - tm_hour: vm.ctx.new_int(tm.hour()).into(), - tm_min: vm.ctx.new_int(tm.minute()).into(), - tm_sec: vm.ctx.new_int(tm.second()).into(), - tm_wday: vm.ctx.new_int(tm.weekday().num_days_from_monday()).into(), - tm_yday: vm.ctx.new_int(tm.ordinal()).into(), - tm_isdst: vm.ctx.new_int(isdst).into(), - tm_gmtoff: vm.ctx.new_int(offset_seconds).into(), - tm_zone: vm.ctx.new_str(tz_abbr).into(), - } - } + fn new(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { + // For gmtime() we pass isdst = 0 and must report UTC. + let (offset_seconds, tz_abbr, tm_isdst) = if isdst == 0 { + (0, "UTC".to_owned(), 0) + } else { + let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); + let offset = local_time.offset().local_minus_utc() + + if isdst == 1 { 3600 } else { 0 }; + let tz = local_time.format("%Z").to_string(); + (offset, tz, isdst) + }; + + Self { + tm_year: vm.ctx.new_int(tm.year()).into(), + tm_mon: vm.ctx.new_int(tm.month()).into(), + tm_mday: vm.ctx.new_int(tm.day()).into(), + tm_hour: vm.ctx.new_int(tm.hour()).into(), + tm_min: vm.ctx.new_int(tm.minute()).into(), + tm_sec: vm.ctx.new_int(tm.second()).into(), + tm_wday: vm.ctx.new_int(tm.weekday().num_days_from_monday()).into(), + tm_yday: vm.ctx.new_int(tm.ordinal()).into(), + tm_isdst: vm.ctx.new_int(tm_isdst).into(), + tm_gmtoff: vm.ctx.new_int(offset_seconds).into(), + tm_zone: vm.ctx.new_str(tz_abbr).into(), + } + }With this change:
gmtime()(which passesisdst = 0) will report UTC offset/name.localtime()(currently passingisdst = -1) continues to use the local timezone path as before.Also applies to: 309-315
crates/vm/src/types/structseq.rs (1)
101-117: Clamp visible length to tuple length to avoid panics whenn_sequence_fieldsis mutated
get_visible_lenreadsn_sequence_fieldsfrom the Python type, and several places use it directly to index/slice the underlyingPyTuple:
STRUCT_SEQUENCE_AS_SEQUENCE.itemindexestuple[idx]after checkingidx < n_seq.STRUCT_SEQUENCE_AS_MAPPING.subscriptslicestuple.as_slice()[..n_seq].PyStructSequence::__getitem__sliceszelf.as_slice()[..n_seq].struct_sequence_richcompareslices both tuples up to lengths derived fromn_sequence_fields.Because
n_sequence_fieldsis a normal, writable Python attribute set inextend_pyclass, user code can do:time.struct_time.n_sequence_fields = 10**9 time.gmtime(0)[0] # or subscripting/slicingand cause an out‑of‑bounds slice/index panic in Rust when
n_sequence_fields > tuple.len().Please clamp the “visible length” to the actual tuple length before using it for indexing/slicing. For example:
- item: atomic_func!(|seq, i, vm| { - let n_seq = get_visible_len(seq.obj, vm)?; - let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap(); + item: atomic_func!(|seq, i, vm| { + let n_seq = get_visible_len(seq.obj, vm)?; + let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap(); + let visible_len = n_seq.min(tuple.len()); let idx = if i < 0 { - let pos_i = n_seq as isize + i; + let pos_i = visible_len as isize + i; if pos_i < 0 { return Err(vm.new_index_error("tuple index out of range")); } pos_i as usize } else { i as usize }; - if idx >= n_seq { + if idx >= visible_len { return Err(vm.new_index_error("tuple index out of range")); } Ok(tuple[idx].clone()) }),- subscript: atomic_func!(|mapping, needle, vm| { - let n_seq = get_visible_len(mapping.obj, vm)?; - let tuple = mapping.obj.downcast_ref::<PyTuple>().unwrap(); - let visible_elements = &tuple.as_slice()[..n_seq]; + subscript: atomic_func!(|mapping, needle, vm| { + let n_seq = get_visible_len(mapping.obj, vm)?; + let tuple = mapping.obj.downcast_ref::<PyTuple>().unwrap(); + let visible_len = n_seq.min(tuple.len()); + let visible_elements = &tuple.as_slice()[..visible_len];- fn __getitem__(zelf: PyRef<PyTuple>, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { - let n_seq = get_visible_len(zelf.as_ref(), vm)?; - let visible_elements = &zelf.as_slice()[..n_seq]; + fn __getitem__(zelf: PyRef<PyTuple>, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { + let n_seq = get_visible_len(zelf.as_ref(), vm)?; + let visible_len = n_seq.min(zelf.len()); + let visible_elements = &zelf.as_slice()[..visible_len];- let zelf_len = get_visible_len(zelf, vm)?; - // For other, try to get visible len; if it fails (not a struct sequence), use full length - let other_len = get_visible_len(other, vm).unwrap_or(other_tuple.len()); - - let zelf_visible = &zelf_tuple.as_slice()[..zelf_len]; - let other_visible = &other_tuple.as_slice()[..other_len]; + let zelf_len = get_visible_len(zelf, vm)?.min(zelf_tuple.len()); + // For other, try to get visible len; if it fails (not a struct sequence), use full length + let other_len = match get_visible_len(other, vm) { + Ok(n) => n.min(other_tuple.len()), + Err(_) => other_tuple.len(), + }; + + let zelf_visible = &zelf_tuple.as_slice()[..zelf_len]; + let other_visible = &other_tuple.as_slice()[..other_len];This preserves existing behavior under normal invariants while preventing user‑level mutations of
n_sequence_fieldsfrom crashing the VM.Also applies to: 137-147, 235-245, 373-385
🧹 Nitpick comments (2)
crates/derive-impl/src/pymodule.rs (1)
626-730: Minor: duplicate parsing of #[pystruct_sequence] and missingmodule = falsehandling
StructSequenceItem::gen_module_itemreparses#[pystruct_sequence]by hand to extractname,module, andno_attr, even thoughPyStructSequenceMetainpystructseq.rsalready encapsulates this logic. It also ignores themodule = falsecase (treated as “no module” byPyStructSequenceMeta), defaultingmodule_nameto the surrounding module instead.To avoid drift between the macro and pymodule views of the same attribute, consider:
- Reusing
PyStructSequenceMeta(or a shared helper) here rather than open‑coding the parsing.- Honoring
module = falseby allowingmodule_nameto beNoneand skipping the__module__set, if that form is intended to represent builtin/module‑less types.crates/derive-impl/src/pystructseq.rs (1)
81-139: Tighten validation for per‑field #[pystruct_sequence(...)] usageTwo edge cases could be worth guarding against at macro time:
Mixed
Named→Unnamed→NamedorderBecause
into_tupleemits all visible fields in declaration order andPyStructSequence::slot_reprzips the firstREQUIRED_FIELD_NAMES.len()tuple elements withREQUIRED_FIELD_NAMES, placing an#[pystruct_sequence(unnamed)]field before a later named field would misalign names and values in__repr__and getters.Conflicting per‑field options
A field marked with both
skipandunnamedsilently becomesSkipped(skipwins), which is surprising.You could reject these at compile time, e.g.:
fn parse_fields(input: &mut DeriveInput) -> Result<FieldInfo> { // ... - for field in &mut fields.named { + let mut seen_unnamed = false; + for field in &mut fields.named { let mut skip = false; let mut unnamed = false; // ... for ident in idents { match ident.to_string().as_str() { "skip" => { skip = true; } "unnamed" => { unnamed = true; } _ => { bail_span!(ident, "Unknown item for #[pystruct_sequence(...)]") } } } // ... - let kind = if skip { - FieldKind::Skipped - } else if unnamed { - FieldKind::Unnamed - } else { - FieldKind::Named - }; + if skip && unnamed { + bail_span!(field, "field cannot be both `skip` and `unnamed` in #[pystruct_sequence(...)]"); + } + let kind = if skip { + FieldKind::Skipped + } else if unnamed { + seen_unnamed = true; + FieldKind::Unnamed + } else { + if seen_unnamed { + bail_span!(field, "named fields must not follow `#[pystruct_sequence(unnamed)]` fields"); + } + FieldKind::Named + };This keeps the macro behavior predictable and aligned with how repr/getters interpret the tuple layout.
Also applies to: 210-253
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/datetimetester.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_structseq.pyis excluded by!Lib/**
📒 Files selected for processing (16)
crates/derive-impl/src/lib.rs(1 hunks)crates/derive-impl/src/pymodule.rs(7 hunks)crates/derive-impl/src/pystructseq.rs(3 hunks)crates/derive/src/lib.rs(1 hunks)crates/stdlib/src/grp.rs(5 hunks)crates/stdlib/src/resource.rs(4 hunks)crates/vm/src/stdlib/nt.rs(2 hunks)crates/vm/src/stdlib/os.rs(9 hunks)crates/vm/src/stdlib/posix.rs(3 hunks)crates/vm/src/stdlib/pwd.rs(7 hunks)crates/vm/src/stdlib/sys.rs(21 hunks)crates/vm/src/stdlib/time.rs(7 hunks)crates/vm/src/types/mod.rs(1 hunks)crates/vm/src/types/structseq.rs(4 hunks)crates/vm/src/vm/context.rs(1 hunks)crates/vm/src/vm/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/vm/context.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/derive-impl/src/pymodule.rscrates/vm/src/stdlib/pwd.rscrates/vm/src/stdlib/nt.rscrates/derive-impl/src/lib.rscrates/stdlib/src/grp.rscrates/vm/src/types/mod.rscrates/derive/src/lib.rscrates/stdlib/src/resource.rscrates/vm/src/stdlib/posix.rscrates/derive-impl/src/pystructseq.rscrates/vm/src/types/structseq.rscrates/vm/src/vm/mod.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/sys.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Applied to files:
crates/vm/src/types/mod.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Applied to files:
crates/vm/src/types/mod.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/derive/src/lib.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/os.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/os.rscrates/vm/src/stdlib/sys.rs
🧬 Code graph analysis (7)
crates/derive-impl/src/pymodule.rs (1)
crates/derive-impl/src/pystructseq.rs (3)
inner(316-318)class_name(322-344)module(346-368)
crates/vm/src/stdlib/pwd.rs (3)
crates/derive-impl/src/lib.rs (3)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)pyclass(41-47)crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/stdlib/src/grp.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/types/mod.rs (1)
crates/vm/src/types/structseq.rs (1)
struct_sequence_new(20-69)
crates/derive/src/lib.rs (1)
crates/derive-impl/src/lib.rs (2)
pystruct_sequence(61-63)pystruct_sequence_data(65-67)
crates/stdlib/src/resource.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/stdlib/time.rs (1)
crates/vm/src/types/structseq.rs (1)
struct_sequence_new(20-69)
⏰ 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). (6)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (42)
crates/vm/src/vm/mod.rs (1)
468-475: LGTM!The migration to
UnraisableHookArgsDatacorrectly follows the new data-driven architecture. The struct is constructed with all required fields and passed to the unraisable hook, which will convert it to a Python object via theToPyObjectimplementation provided by the#[pystruct_sequence_data]macro.crates/stdlib/src/grp.rs (5)
16-28: LGTM! Clean separation of data and wrapper types.The new pattern correctly separates the data struct (
GroupDatawith#[pystruct_sequence_data]) from the Python-visible wrapper (PyGroupwith#[pystruct_sequence]). The attribute configurationdata = "GroupData"properly links them.
30-45: LGTM!The
from_unistd_groupmethod is correctly placed onGroupDataand produces the data struct directly.
47-66: LGTM!Return type change to
PyResult<GroupData>is correct. The framework will automatically convert the data struct to a Python object via the generatedToPyObjectimplementation.
68-83: LGTM!Consistent with
getgrgid- returnsPyResult<GroupData>with proper conversion.
85-101: LGTM!For
getgrall, the explicit.to_pyobject(vm)call is correctly used since the function returnsVec<PyObjectRef>rather than a single data struct.crates/stdlib/src/resource.rs (3)
66-90: LGTM! Correct data-driven struct sequence implementation.The
RUsageDataandPyRUsagepattern follows the same architecture as other migrated types. UsingFrom<libc::rusage>is idiomatic and clean.
92-114: LGTM!The
Fromimplementation correctly maps all 16 rusage fields and handles the timeval-to-f64 conversion inline with the helper closure.
116-133: LGTM!The
getrusagefunction cleanly uses.map(RUsageData::from)to convert the raw rusage struct, maintaining the same error handling logic.crates/vm/src/types/mod.rs (1)
6-6: LGTM!The expanded exports correctly expose
PyStructSequenceData(the trait for data structs) andstruct_sequence_new(the constructor function) which are required for the new attribute-based macro system.crates/vm/src/stdlib/pwd.rs (5)
19-34: LGTM! Consistent with the data-driven architecture.The
PasswdDataandPyPasswdpattern matches the structure used ingrp.rsandresource.rs, maintaining consistency across the codebase.
36-58: LGTM!The
From<User>implementation handles CString and PathBuf conversions gracefully with lossy fallbacks, which is appropriate for system data that may contain non-UTF8 sequences.
60-75: LGTM!Return type correctly changed to
PyResult<PasswdData>.
77-95: LGTM!Consistent with
getpwnam- returnsPyResult<PasswdData>.
97-115: LGTM!For
getpwall, the explicit.to_pyobject(vm)is correctly used since the function returnsVec<PyObjectRef>.crates/vm/src/stdlib/posix.rs (2)
1333-1341: UnameResult now cleanly forwards all uname fields into data structThe change to return
_os::UnameResultDataand populate all five fields (includingversionandmachine) looks correct and keeps error handling intact.
1730-1747: TerminalSizeData return type matches new data‑driven structseq patternSwitching
get_terminal_sizeto return_os::TerminalSizeData { columns, lines }preserves behavior while aligning with the new data‑backed struct sequence design.crates/vm/src/stdlib/nt.rs (1)
178-210: NT get_terminal_size correctly migrated to TerminalSizeDataThe function now returns
_os::TerminalSizeData { columns, lines }with unchanged Windows console logic, which is consistent with the new struct sequence data model.crates/derive-impl/src/pymodule.rs (1)
15-22: StructSequence integration into pymodule looks consistent with existing item handling
AttrName::StructSequence, its Display/FromStr wiring,module_item_newdispatch, andStructSequenceItem’sContentItemimpl all follow the existing patterns for functions, attrs, and classes and should integrate cleanly with the module item pipeline.Also applies to: 24-32, 37-47, 222-245, 412-448
crates/derive-impl/src/lib.rs (1)
61-67: New pystruct_sequence_ forwarding functions align with existing macro plumbing*Both
pystruct_sequenceandpystruct_sequence_datamirror the pattern used bypyclass/pyexception, cleanly delegating topystructseq::impl_pystruct_sequence{,_data}viaresult_to_tokens.crates/derive/src/lib.rs (2)
217-239: Attribute macropystruct_sequenceis wired correctly and documentation matches usageThe new
#[proc_macro_attribute] pub fn pystruct_sequence(attr, item)mirrorspyclass/pymoduleparsing (Punctuated meta + Item) and forwards toderive_impl::pystruct_sequence, with doc examples that align with the data + wrapper pattern.
241-268: pystruct_sequence_data macro and options are consistent with impl sideThe
pystruct_sequence_dataattribute macro correctly parses its meta (try_from_object) and item, then delegates toderive_impl::pystruct_sequence_data. The docstring describes field‑level#[pystruct_sequence(skip)]and thetry_from_objectoption in a way that matches the implementation inpystructseq.rs.crates/derive-impl/src/pystructseq.rs (2)
10-32: Data‑oriented pystruct_sequence_data implementation looks solidThe new
FieldKind/FieldInfomodel,parse_fields, andimpl_pystruct_sequence_datatogether provide a clear separation of:
- visible named fields (
REQUIRED_FIELD_NAMES)- visible unnamed fields (
UNNAMED_FIELDS_LEN)- hidden/skipped fields (
OPTIONAL_FIELD_NAMES),and generate appropriate index constants,
into_tuple, and an optionalTryFromObject/try_from_elementspath whentry_from_objectis requested. This matches the intended struct‑sequence data semantics and composes cleanly with the runtimePyStructSequenceDatatrait.Also applies to: 33-65, 166-233, 255-301
305-396: Wrapper generation for #[pystruct_sequence] matches runtime expectations
impl_pystruct_sequencecorrectly:
- enforces use on a
structand requiresnameanddata = "DataType"in the attribute,- maps
name/moduleintoPyClassDef::NAME,MODULE_NAME, andTP_NAME,- makes the Python type a
PyTuplesubclass viatype Base = PyTuple,- implements
StaticType,MaybeTraverse, andPyStructSequence<Data = #data_ident>,- and provides a
ToPyObjectimpl for the data struct that callsPyStructSequence::from_data.This lines up with the new runtime APIs in
types::structseqand gives a clean, attribute‑driven way to bind data structs to Python-visible struct sequence types.Also applies to: 409-495
crates/vm/src/stdlib/time.rs (1)
34-41: StructTimeData + PyStructTime integration with the new struct sequence runtime looks good
StructTimeDatais annotated with#[pystruct_sequence_data(try_from_object)], matching the new derive‑impl; fields and skips (tm_gmtoff,tm_zone) align with_STRUCT_TM_ITEMS = 11.gmtime,localtime,mktime,asctime, andstrftimenow take/returnStructTimeDataand rely onToPyObjectto produce/consume the Pythonstruct_time, which is wrapped byPyStructTime.PyStructTimeis declared with#[pystruct_sequence(name = "struct_time", module = "time", data = "StructTimeData")]and#[pyclass(with(PyStructSequence))], andslot_newdelegates tostruct_sequence_new, so construction and length validation reuse the common runtime path.This is a clean application of the new data‑driven struct sequence infrastructure.
Also applies to: 298-305, 309-338, 343-374, 478-553
crates/vm/src/stdlib/sys.rs (10)
451-454: LGTM!The migration to
PyFlags::from_data(FlagsData::from_settings(...), vm)pattern cleanly separates data construction from Python object creation.
674-741: LGTM!The
_unraisablehookandunraisablehookfunctions now correctly acceptUnraisableHookArgsDatadirectly. The function body accesses fields consistently with the data struct pattern.
901-923: LGTM!Clean implementation of
AsyncgenHooksDatawith#[pystruct_sequence_data]and the correspondingPyAsyncgenHookswrapper. Theget_asyncgen_hooksfunction correctly returnsAsyncgenHooksData.
925-1003: LGTM!The
FlagsDatastruct andPyFlagswrapper follow the new data-driven pattern correctly. Thefrom_settingsmethod is properly placed on the data struct, and theslot_newimplementation correctly prevents instantiation.
1005-1031: LGTM!Consistent pattern applied to
ThreadInfoDataandPyThreadInfowith proper#[cfg(feature = "threading")]gating on both the data struct and wrapper.
1032-1068: LGTM!
FloatInfoDataandPyFloatInfocorrectly implement the data-driven pattern with theINFOconstant properly placed on the data struct.
1069-1127: LGTM!Both
HashInfoData/PyHashInfoandIntInfoData/PyIntInfofollow the established pattern consistently. TheINFOconstants are correctly placed on the data structs.
1128-1161: Visibility consistency:VersionInfoDataandPyVersionInfoarepubwhile others arepub(super).These types are
pub(lines 1130, 1149) while similar types likeFlagsData,ThreadInfoData, etc., usepub(super). This appears intentional sinceVersionInfoDatamay need broader exposure, but verify this is the intended visibility.
1163-1186: LGTM!Windows-specific
WindowsVersionDataandPyWindowsVersioncorrectly implement the data-driven pattern with proper#[cfg(windows)]gating.
1187-1201: Verifytry_from_objectattribute behavior.The
#[pystruct_sequence_data(try_from_object)]attribute onUnraisableHookArgsData(line 1188) differs from other data structs. Ensure this generates the expectedTryFromObjectimplementation for use in function parameter binding.crates/vm/src/stdlib/os.rs (7)
837-867: LGTM!
PyStatResultwrapper correctly referencesStatResultDataand theslot_newimplementation properly binds args toStatResultDatabefore conversion. The flattening logic for nested tuples is preserved.
936-940: LGTM!The
statfunction correctly usesStatResultData::from_stat(&stat, vm).to_pyobject(vm)to return the result, following the data-driven pattern.
1233-1251: LGTM!
TimesResultDataandPyTimesResultcorrectly implement the data-driven pattern with proper#[cfg(all(any(unix, windows), not(target_os = "redox")))]gating applied consistently to both the data struct and wrapper.
1279-1316: LGTM!The
timesfunction correctly constructsTimesResultDataon both Windows and Unix paths and converts toPyObjectviato_pyobject(vm).
1480-1491: LGTM!
TerminalSizeDataandPyTerminalSizefollow the pattern correctly. Thepub(crate)visibility is appropriate for cross-module usage within the crate.
1493-1507: LGTM!
UnameResultDataandPyUnameResultcorrectly implement the data-driven pattern withpub(crate)visibility for cross-module access.
738-835: Verify macro compatibility forStatResultDataattribute combination.
StatResultDatacombines#[derive(Debug, FromArgs)]with#[pystruct_sequence_data]and uses field-level attributes#[pystruct_sequence(unnamed)]and#[pystruct_sequence(skip)]. Theunnamedfields (lines 750-757) participate in the tuple representation without named accessors, whileskipfields (lines 760-780) are excluded entirely. Verify that thepystruct_sequence_dataderive macro correctly handles this mixed-attribute configuration alongsideFromArgs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/vm/src/types/structseq.rs (1)
101-117: Clamp visible length to prevent panics ifn_sequence_fieldsis mutated from PythonThe
n_sequence_fieldsattribute is accessible from Python and could be modified to a value larger than the actual tuple length. Several operations usen_seqdirectly for indexing/slicing without clamping:
STRUCT_SEQUENCE_AS_SEQUENCE.item(line 116):tuple[idx]STRUCT_SEQUENCE_AS_MAPPING.subscript(line 139):tuple.as_slice()[..n_seq]__getitem__(line 238):zelf.as_slice()[..n_seq]struct_sequence_richcompare(lines 377-378): slices both tuplesApply clamping like
n_seq.min(tuple.len())before using the value:// Example fix for item: item: atomic_func!(|seq, i, vm| { let n_seq = get_visible_len(seq.obj, vm)?; let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap(); + let n_seq = n_seq.min(tuple.len()); let idx = if i < 0 {Also applies to: 134-147, 235-245, 359-385
crates/vm/src/stdlib/time.rs (1)
309-315:gmtime()uses local timezone info instead of UTC for tm_gmtoff/tm_zone
StructTimeData::new()derivestm_gmtoffandtm_zonefromchrono::Localfor all cases. Forgmtime(), CPython returnstm_gmtoff = 0andtm_zone = "UTC".Split into separate constructors:
+impl StructTimeData { + fn new_utc(vm: &VirtualMachine, tm: NaiveDateTime) -> Self { + Self { + tm_year: vm.ctx.new_int(tm.year()).into(), + // ... other fields ... + tm_isdst: vm.ctx.new_int(0).into(), + tm_gmtoff: vm.ctx.new_int(0).into(), + tm_zone: vm.ctx.new_str("UTC").into(), + } + } + + fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { + // existing implementation + } +}Then use
new_utc()ingmtime()andnew_local()inlocaltime().
🧹 Nitpick comments (9)
crates/vm/src/stdlib/sys.rs (3)
451-459: Struct-sequence data/wrapper pattern forsysmetadata looks consistentThe new
*_Data+Py*pairs forflags,float_info,hash_info,int_info,thread_info,version_info, andasyncgen_hooks, together with the updated accessors (e.g.PyFlags::from_data(FlagsData::from_settings(...), vm)), form a clean, uniform pattern. Field layouts and types look appropriate for the corresponding CPython struct sequences, and centralizing constants in*_Data::INFO/FlagsData::from_settingswill make future changes less error‑prone. Nice use of the#[pystruct_sequence]/#[pystruct_sequence_data]macros as per the macro-system guidelines.Also applies to: 743-756, 812-821, 863-924, 929-1128
624-672:getwindowsversion()struct-sequence wiring looks correct; name nit (optional)Populating
WindowsVersionDatafrom bothOSVERSIONINFOEXWandget_kernel32_version()and returning it viaPyWindowsVersion::from_data(winver, vm)matches the expected(major, minor, build, platform, service_pack, ..., platform_version)layout, and using(u32, u32, u32)forplatform_versionwill convert cleanly to a Python tuple. If you’re aiming for exact CPython parity, you might consider naming the struct-sequence typegetwindowsversion_resultinstead ofgetwindowsversion, but that’s cosmetic and non‑blocking.Also applies to: 1164-1187
674-731:UnraisableHookArgsData+__unraisablehook__: minor type/traceback nitsThe move to
UnraisableHookArgsData/PyUnraisableHookArgswithtry_from_objectand a default__unraisablehook__implemented in Rust fits well with the new struct-sequence model and keeps VM ↔ sys coupling clean. A couple of small follow‑ups you might consider:
UnraisableHookArgsData.exc_typeis aPyTypeRef, but_unraisablehookstill checksvm.is_none(unraisable.exc_type.as_object()). With the current type, that branch is effectively dead; either drop theNonecheck or relax the field type (e.g.PyObjectReforOption<PyTypeRef>) if you want to acceptexc_typebeingNonelike CPython sometimes allows.- The TODO around traceback printing currently calls
traceback.print_stack()and ignoresunraisable.exc_traceback. When you revisit this, switching totraceback.print_exception(unraisable.exc_type, unraisable.exc_value, unraisable.exc_traceback)would be closer to CPython’s behavior while still usingPyStderr.Functionally this is fine as-is, so these are non‑blocking cleanups.
Also applies to: 1188-1203
crates/vm/src/vm/mod.rs (1)
460-477:run_unraisablenow usingUnraisableHookArgsDatais correct; robustness nitsConstructing
stdlib::sys::UnraisableHookArgsDatahere with(exc_type, exc_value, exc_traceback, err_msg, object)matches the new struct-sequence definition and keeps the VM side data‑oriented, which is nice.Two non‑blocking robustness tweaks you might consider:
- Both
import("sys", 0).unwrap()andget_attr("unraisablehook", ...) .unwrap()will panic ifsysor the hook are missing (e.g. during odd teardown paths). Falling back to a minimal stderr print instead of panicking could make this path more resilient.- On hook failure you currently
println!the exception repr to stdout; usingsys::PyStderr(self)instead would align with how other unraisable/exit paths report errors.Behavior is otherwise unchanged and looks good.
crates/derive/src/lib.rs (1)
241-268:#[pystruct_sequence_data]macro wiring matches its documentationThe
pystruct_sequence_dataattribute macro correctly parses arguments and delegates toderive_impl::pystruct_sequence_data. The doc comments about generating field index constants,into_tuple(), and optionaltry_from_objectare consistent with the intended behavior in the derive-impl crate (modulo the tuple/skip detail noted in pystructseq.rs).crates/derive-impl/src/pystructseq.rs (4)
10-144: Field parsing and classification are solid; consider tightening edge casesThe
FieldKind/ParsedField/FieldInfomodel andparse_fieldslogic correctly:
- Restrict
#[pystruct_sequence_data]to named-field structs.- Parse per-field
#[pystruct_sequence(skip)]and#[pystruct_sequence(unnamed)]markers.- Strip those attributes from the emitted struct while retaining other attributes.
Two small follow‑ups you might consider:
- Right now, a field with both
skipandunnamedsilently becomesSkipped. That’s probably not a meaningful combination; explicitly rejecting it with a diagnostic would make misuse more obvious.attr.parse_meta()failures are silently ignored. If that can ever happen for well‑formed attributes, surfacing an error instead ofcontinuewould make debugging easier.These are not blockers but would tighten the developer experience.
209-253: Guard against panics intry_from_elementswhen called directlyThe generated
try_from_elementsimplementation relies on the caller ensuring there are at leastvisible_fields.len()elements:let mut iter = elements.into_iter(); Ok(Self { #(#visible_fields: iter.next().unwrap().clone().try_into_value(vm)?,)* #(#skipped_fields: match iter.next() { Some(v) => v.clone().try_into_value(vm)?, None => vm.ctx.none(), },)* })When used via the generated
TryFromObjectimpl, you do checkseq.len() < n_requiredfirst, so in that pathunwrap()is safe. However,try_from_elementsis part of the publicPyStructSequenceDatatrait and can be called directly; in that case a too-shortelementsvector will panic.If you want this to be more robust, consider turning those
unwrap()s into PythonTypeErrors instead of panicking, e.g. by matching and returning an error wheniter.next()isNone. Not strictly required for this PR to function, but it would make the trait safer to use in other call sites.
305-400:PyStructSequenceMetawiring is consistent; consider future-proofing thedataparameterThe
PyStructSequenceMeta+ItemMetaimplementation:
- Restricts allowed keys to
"name","module","data", and"no_attr".- Parses
nameandmodulestrictly as string literals.- Parses
dataas a string literal and turns it into anIdentviaformat_ident!, with a clear error ifdatais missing.That’s sufficient for the current API, where the example uses
data = "StructTimeData"in the same module. If you later find you need cross-module data types, you might want to allow a path expression fordatainstead of only a bare string, but that can be an incremental enhancement.
402-500: Type-levelimpl_pystruct_sequencegeneration looks correct and matches the new macro docs
impl_pystruct_sequence:
- Enforces that the attribute is on a
struct.- Re-parses the attribute via
PyStructSequenceMeta, requiringnameanddatawhile makingmoduleoptional.- Re-emits the Python type as an empty
structwith the original visibility (as per the macro docs, the data lives in a separateDatastruct).- Implements
PyClassDefwithBase = PyTuple,StaticType, and a trivialMaybeTraverse, which is appropriate for an empty wrapper.- Implements the
PyStructSequencetrait binding the wrapper type to the generatedDatatype.- Implements
ToPyObjectfor theDatatype in terms ofPyStructSequence::from_data, so data instances naturally convert to the corresponding Python struct-sequence objects.This design cleanly matches the attribute macro documentation in
crates/derive/src/lib.rsand should integrate with the existing VM-sidePyStructSequence/PyStructSequenceDatatraits.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/datetimetester.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_structseq.pyis excluded by!Lib/**
📒 Files selected for processing (16)
crates/derive-impl/src/lib.rs(1 hunks)crates/derive-impl/src/pymodule.rs(9 hunks)crates/derive-impl/src/pystructseq.rs(3 hunks)crates/derive/src/lib.rs(1 hunks)crates/stdlib/src/grp.rs(5 hunks)crates/stdlib/src/resource.rs(4 hunks)crates/vm/src/stdlib/nt.rs(2 hunks)crates/vm/src/stdlib/os.rs(9 hunks)crates/vm/src/stdlib/posix.rs(3 hunks)crates/vm/src/stdlib/pwd.rs(7 hunks)crates/vm/src/stdlib/sys.rs(21 hunks)crates/vm/src/stdlib/time.rs(7 hunks)crates/vm/src/types/mod.rs(1 hunks)crates/vm/src/types/structseq.rs(4 hunks)crates/vm/src/vm/context.rs(1 hunks)crates/vm/src/vm/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/types/mod.rs
- crates/vm/src/vm/context.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/stdlib/src/grp.rscrates/derive-impl/src/pymodule.rscrates/derive/src/lib.rscrates/stdlib/src/resource.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/pwd.rscrates/derive-impl/src/lib.rscrates/derive-impl/src/pystructseq.rscrates/vm/src/stdlib/sys.rscrates/vm/src/vm/mod.rscrates/vm/src/stdlib/os.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/time.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/derive/src/lib.rscrates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/types/structseq.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/types/structseq.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/os.rs
🧬 Code graph analysis (8)
crates/derive-impl/src/pymodule.rs (1)
crates/derive-impl/src/pystructseq.rs (4)
inner(316-318)class_name(322-344)no_attr(397-399)module(346-368)
crates/derive/src/lib.rs (1)
crates/derive-impl/src/lib.rs (2)
pystruct_sequence(61-63)pystruct_sequence_data(65-67)
crates/stdlib/src/resource.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/stdlib/pwd.rs (3)
crates/derive-impl/src/lib.rs (3)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)pyclass(41-47)crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/derive-impl/src/lib.rs (2)
crates/derive/src/lib.rs (2)
pystruct_sequence(235-239)pystruct_sequence_data(264-268)crates/derive-impl/src/pystructseq.rs (2)
impl_pystruct_sequence(412-500)impl_pystruct_sequence_data(166-301)
crates/derive-impl/src/pystructseq.rs (1)
crates/vm/src/types/structseq.rs (4)
Self(280-283)try_from_elements(171-173)into_tuple(166-166)from_data(186-191)
crates/vm/src/stdlib/os.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/types/structseq.rs (1)
crates/vm/src/vm/context.rs (1)
new_tuple(462-464)
⏰ 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 (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust 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: Ensure compilation on various targets
- GitHub Check: Run tests under miri
🔇 Additional comments (44)
crates/vm/src/stdlib/pwd.rs (4)
19-28: LGTM!The
PasswdDatastruct correctly uses the new#[pystruct_sequence_data]attribute macro, cleanly separating the data representation from the Python binding. Field definitions match the expected Unix passwd structure.
30-35: LGTM!The
PyPasswdwrapper struct correctly uses the new attribute-based macro pattern with proper module and name bindings. The#[pyattr]makes the type accessible from Python, and the#[pyclass(with(PyStructSequence))]correctly applies the struct sequence implementation. Based on coding guidelines, this follows the macro system pattern for implementing Python functionality in Rust.
37-59: LGTM!The
From<User>implementation correctly targetsPasswdData, and the function signatures forgetpwnamandgetpwuidproperly returnPyResult<PasswdData>. The data-centric pattern allows the macro-generated traits to handle Python object conversion automatically.Also applies to: 61-76, 78-96
98-116: LGTM!The
getpwallfunction correctly uses the explicit.to_pyobject(vm)conversion pattern since it returns aVec<PyObjectRef>rather than relying on automatic conversion. The thread-safety mechanism withparking_lot::Mutexis properly maintained.crates/vm/src/stdlib/posix.rs (2)
1333-1342: LGTM!The
unamefunction correctly returnsUnameResultDatawith all fields properly populated. This aligns with the new data-driven struct sequence pattern.
1729-1747: LGTM!The
get_terminal_sizefunction correctly returnsTerminalSizeDatawith the columns and lines fields. This is consistent with the NT module implementation and the new data-centric pattern.crates/vm/src/stdlib/nt.rs (1)
178-210: LGTM!The
get_terminal_sizefunction correctly migrates toTerminalSizeData, maintaining consistency with the POSIX implementation and the new data-driven pattern.crates/vm/src/types/structseq.rs (5)
16-69: LGTM!The
struct_sequence_newfunction correctly validates sequence length againstn_sequence_fields(minimum) andn_fields(maximum), and pads withNonevalues. Error messages are clear and follow CPython patterns.
71-76: LGTM!The
get_visible_lenhelper correctly retrievesn_sequence_fieldsfrom the class.
151-174: LGTM!The
PyStructSequenceDatatrait provides a clean interface for data structs with required/optional field separation and conversion methods.
176-325: LGTM!The
PyStructSequencetrait implementation correctly:
- Wires up field getters for both required and optional fields
- Sets up
__match_args__,n_sequence_fields,n_fields, andn_unnamed_fields- Overrides
as_sequence,as_mapping,iter,hash, andrichcompareslotsThe
from_datamethod correctly converts data to a typed tuple reference.
327-355: LGTM!The
struct_sequence_iterandstruct_sequence_hashfunctions correctly operate only on visible elements by taking the firstn_seqitems.crates/vm/src/stdlib/os.rs (8)
738-781: LGTM!The
StatResultDatastruct correctly separates:
- Required visible fields (st_mode through st_size)
- Unnamed visible fields (st_atime_int, st_mtime_int, st_ctime_int) with
#[pystruct_sequence(unnamed)]- Hidden/skip fields (float times, ns times, reparse_tag) with
#[pystruct_sequence(skip)]This matches CPython's stat_result structure where indices 0-6 are named, 7-9 are integer timestamps, and the rest are attribute-only.
783-835: LGTM!The
from_statmethod correctly computes all time-related fields from the platform-specific stat struct, handling the various timestamp representations across platforms (standard unix, netbsd, wasi, windows).
837-868: LGTM!The
PyStatResultwrapper andslot_newimplementation correctly:
- Flattens nested tuple arguments (matching CPython behavior)
- Binds to
StatResultDataand converts viato_pyobject
940-941: LGTM!The
statfunction correctly returnsStatResultData::from_stat(&stat, vm).to_pyobject(vm).
1235-1252: LGTM!The
TimesResultDataandPyTimesResultare properly defined with the five standard fields (user, system, children_user, children_system, elapsed).
1281-1319: LGTM!The
timesfunction correctly constructsTimesResultDataon both Windows and Unix platforms and returns it viato_pyobject(vm).
1482-1494: LGTM!The
TerminalSizeDataandPyTerminalSizeare properly defined with columns and lines fields.
1496-1511: LGTM!The
UnameResultDataandPyUnameResultare properly defined with sysname, nodename, release, version, and machine fields.crates/vm/src/stdlib/time.rs (7)
36-41: LGTM!Import changes correctly add
struct_sequence_newandPyStructSequencewhile removingTryFromObject(now handled via macro-generated code).
279-305: LGTM!Both
OptionalArgimplementations correctly handle conversion toNaiveDateTimefor local time.
317-326: LGTM!The
localtimefunction correctly usesStructTimeData::newwith the local instant and isdst=-1 (unknown).
328-338: LGTM!The
mktimefunction correctly:
- Converts
StructTimeDatatoNaiveDateTime- Interprets it as local time using
chrono::Local.from_local_datetime()- Handles ambiguous/invalid local times with proper error
- Returns epoch timestamp as f64
342-374: LGTM!Both
asctimeandstrftimecorrectly acceptOptionalArg<StructTimeData>and usenaive_or_localfor default handling.
478-541: LGTM!The
StructTimeDatastruct properly defines all 11 struct_time fields with:
- 9 visible required fields (tm_year through tm_isdst)
- 2 hidden skip fields (tm_gmtoff, tm_zone)
The
to_date_timemethod correctly reconstructsNaiveDateTimefrom the fields.
543-554: LGTM!The
PyStructTimewrapper correctly usesstruct_sequence_newinslot_newto construct instances from sequences, delegating validation to the shared implementation.crates/stdlib/src/resource.rs (4)
66-84: LGTM: Data struct correctly defined.The
RUsageDatastruct properly uses the new#[pystruct_sequence_data]macro with all fields correctly typed. The field layout matches the originallibc::rusagestructure.
86-91: LGTM: Wrapper struct correctly configured.The
PyRUsagewrapper properly uses#[pystruct_sequence]with all required parameters (name,module,data) and is correctly registered with#[pyattr]for module visibility.
93-115: LGTM: Conversion implementation is correct.The
From<libc::rusage>trait implementation correctly maps all 16 fields with proper timeval-to-f64 conversion for timing fields.
118-118: Verify that the data struct return type integrates correctly with the macro system using RustPython's conversion APIs.The function now returns
PyResult<RUsageData>(the data struct) rather than the Python wrapper type. Ensure that the appropriate RustPython conversion macro (such asPyStructSequenceorTryIntoPyStructSequencefromrustpython-derive) correctly convertsRUsageDatainto a Python struct sequence when returned from a#[pyfunction]. Note: RustPython uses its own conversion system viarustpython-deriveandrustpython-vm, not PyO3'sToPyObjecttrait.crates/stdlib/src/grp.rs (2)
16-46: PyStructSequence data type and wrapper wiring look correctThe
GroupDatalayout (gr_name,gr_passwd,gr_gid,gr_mem) matches CPython’sstruct_groupfields, and the combination of#[pystruct_sequence_data]onGroupDatawith#[pyattr]+#[pystruct_sequence(name = "struct_group", module = "grp", data = "GroupData")]and#[pyclass(with(PyStructSequence))] impl PyGroup {}is consistent with the new data‑backed struct‑sequence pattern. Thefrom_unistd_groupconstructor cleanly encapsulates thenix::unistd::Group→ Python conversion, including robust handling of non‑UTF‑8 passwords viacstr_lossy.
49-84: Function returns and getgrall conversion align with the new data‑driven struct‑sequence modelHaving
getgrgidandgetgrnamreturnPyResult<GroupData>(rather than the wrapper type directly) fits the attribute‑basedpystruct_sequence_dataapproach, relying on the generatedToPyObjectforGroupDatato materializegrp.struct_groupinstances at the Python level. The error handling and lookup logic (range check vialibc::gid_t::try_from,Group::from_gid/from_name, andKeyErrormessages) remain intact.In
getgrall, converting viaGroupData::from_unistd_group(group, vm).to_pyobject(vm)ensures all three APIs go through the same data constructor, so field semantics stay consistent acrossgetgrgid,getgrnam, andgetgrall.Also applies to: 93-101
crates/vm/src/stdlib/sys.rs (2)
1-3: Re-exportingUnraisableHookArgsDatafor VM integration is soundRe-exporting
UnraisableHookArgsDatafromsyskeeps the VM side using the new data-centric API without depending on the concrete struct-sequence wrapper. No issues here.
763-773: Single-sourcing the int max‑digits threshold viaIntInfoDatais a good changeUsing
IntInfoData::INFO.str_digits_check_thresholdinsideset_int_max_str_digitskeeps the validation rule (0or>= threshold) in one place and aligned with whatsys.int_inforeports. The logic and error path look correct.crates/derive-impl/src/pymodule.rs (5)
16-53: AttrName extension forpystruct_sequencelooks consistentThe new
StructSequencevariant is correctly threaded throughDisplayandFromStr, so#[pystruct_sequence]will be recognized and formatted like existing#[py*]attributes. No issues here.
223-247: Factory wiring forStructSequenceItemmatches existing patternsThe new
AttrName::StructSequencearm inmodule_item_newmirrors the existing Function/Class/Attr cases and correctly fillsContentItemInner { index, attr_name }pluspy_attrs. This will integrate struct sequences cleanly into the module item pipeline.
308-325: Allowing#[pystruct_sequence]after#[pyattr]is aligned with other#[py*]itemsIncluding
AttrName::StructSequencein the list of items that may follow#[pyattr]and updating the error message keeps the module attribute surface coherent with classes, functions, and exceptions.
416-452:StructSequenceItemas aContentItemis minimal and correctThe new
StructSequenceItemwrapper and itsContentItemimpl are straightforward copies of the existing patterns forFunctionItem/ClassItem/AttributeItem, so they integrate properly with shared infrastructure.
630-705: Struct sequence module wiring mirrors#[pyclass]behavior; semantics look sound
StructSequenceItem::gen_module_item:
- Validates the item is a
struct, usingbail_span!on misuse.- Uses
PyStructSequenceMeta::from_attrso the attribute parsing is centralized with the rest of the struct-sequence machinery.- Enforces
nameandmodulethe same way every time, defaultingmoduleto thepymodulename.- Requires either
#[pyattr]orno_attr, matching#[pyclass]semantics for whether the type is exported as a module attribute.- Handles multiple
#[pyattr]names and generates either a single or cloned registration.Assuming the existing blanket
PyClassImplimplementation still applies to types that implementPyClassDef + StaticType + MaybeTraverse, this should behave correctly at runtime. Please just ensure existing tests around#[pyclass]+#[pyattr]-style module wiring are extended to cover#[pystruct_sequence]as well.crates/derive/src/lib.rs (1)
217-239: Attribute macro surface for#[pystruct_sequence]is wired correctlyThe new
pystruct_sequenceproc-macro attribute mirrors patterns used bypyclass,pyexception, andpymodule(parse attr viaPunctuated::parse_terminated, parse item asItem, then delegate toderive_impl). The documentation and example match the implementation contract (empty struct type wrapping a data struct).crates/derive-impl/src/lib.rs (1)
61-67: New derive-impl entry points for struct sequences are consistent with existing API
pystruct_sequenceandpystruct_sequence_datafollow the same pattern aspyclass/pymodule: they accept(PunctuatedNestedMeta, Item)and delegate to the correspondingimpl_functions viaresult_to_tokens. This keeps the public derive-impl surface coherent.crates/derive-impl/src/pystructseq.rs (2)
146-153:has_try_from_objecthelper is straightforwardScanning the nested meta list for an ident equal to
try_from_objectis a simple and robust way to gate generation of the extra conversion impls. No functional concerns here.
255-301: Data struct codegen and attribute stripping look correctThe generated output for
#[pystruct_sequence_data]:
- Adds an inherent
implwith field index constants derived from field idents.- Implements
PyStructSequenceDatawithREQUIRED_FIELD_NAMES,OPTIONAL_FIELD_NAMES, andUNNAMED_FIELDS_LENall based on the parsedFieldInfo.- Appends the optional
try_from_elementsoverride andTryFromObjectimpl only whentry_from_objectis requested.- Re-emits a cleaned version of the original struct with only
#[pystruct_sequence(...)]field attributes removed.This matches the documented behavior and keeps user-visible struct definitions free of macro-only attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
crates/vm/src/types/structseq.rs (4)
101-117: Clamp visible length to prevent panics ifn_sequence_fieldsis modified from Python.The
itemfunction usesn_seqfromget_visible_lendirectly to bounds-check and index into the tuple without clamping to the actual tuple length. If Python code mutatesn_sequence_fieldsto exceed the tuple's length, this will panic when accessingtuple[idx].Apply this defensive fix:
item: atomic_func!(|seq, i, vm| { let n_seq = get_visible_len(seq.obj, vm)?; let tuple = seq.obj.downcast_ref::<PyTuple>().unwrap(); + let visible_len = n_seq.min(tuple.len()); let idx = if i < 0 { - let pos_i = n_seq as isize + i; + let pos_i = visible_len as isize + i; if pos_i < 0 { return Err(vm.new_index_error("tuple index out of range")); } pos_i as usize } else { i as usize }; - if idx >= n_seq { + if idx >= visible_len { return Err(vm.new_index_error("tuple index out of range")); } Ok(tuple[idx].clone()) }),
136-147: Clamp visible length before slicing to prevent panics.The
subscriptfunction slicestuple.as_slice()[..n_seq]without clampingn_seqto the tuple's actual length. A mutatedn_sequence_fieldslarger than the tuple will cause an out-of-bounds panic.Apply this fix:
subscript: atomic_func!(|mapping, needle, vm| { let n_seq = get_visible_len(mapping.obj, vm)?; let tuple = mapping.obj.downcast_ref::<PyTuple>().unwrap(); - let visible_elements = &tuple.as_slice()[..n_seq]; + let visible_len = n_seq.min(tuple.len()); + let visible_elements = &tuple.as_slice()[..visible_len]; match SequenceIndex::try_from_borrowed_object(vm, needle, "tuple")? { SequenceIndex::Int(i) => visible_elements.getitem_by_index(vm, i), SequenceIndex::Slice(slice) => visible_elements .getitem_by_slice(vm, slice) .map(|x| vm.ctx.new_tuple(x).into()), } }),
235-246: Clamp visible length in__getitem__to prevent panics.The method slices
zelf.as_slice()[..n_seq]without clamping. Ifn_sequence_fieldsis modified to exceed the tuple length, this will panic.Apply this fix:
#[pymethod] fn __getitem__(zelf: PyRef<PyTuple>, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { let n_seq = get_visible_len(zelf.as_ref(), vm)?; - let visible_elements = &zelf.as_slice()[..n_seq]; + let visible_len = n_seq.min(zelf.len()); + let visible_elements = &zelf.as_slice()[..visible_len]; match SequenceIndex::try_from_borrowed_object(vm, &needle, "tuple")? { SequenceIndex::Int(i) => visible_elements.getitem_by_index(vm, i), SequenceIndex::Slice(slice) => visible_elements .getitem_by_slice(vm, slice) .map(|x| vm.ctx.new_tuple(x).into()), } }
359-385: Clamp visible lengths in richcompare to prevent panics.Both
zelf_lenandother_lenare used to slice tuples without clamping to actual tuple lengths. Mutatedn_sequence_fieldsvalues will cause out-of-bounds panics.Apply this fix:
fn struct_sequence_richcompare( zelf: &PyObject, other: &PyObject, op: PyComparisonOp, vm: &VirtualMachine, ) -> PyResult<Either<PyObjectRef, PyComparisonValue>> { let zelf_tuple = zelf .downcast_ref::<PyTuple>() .ok_or_else(|| vm.new_type_error("expected tuple"))?; // If other is not a tuple, return NotImplemented let Some(other_tuple) = other.downcast_ref::<PyTuple>() else { return Ok(Either::B(PyComparisonValue::NotImplemented)); }; - let zelf_len = get_visible_len(zelf, vm)?; + let zelf_len = get_visible_len(zelf, vm)?.min(zelf_tuple.len()); // For other, try to get visible len; if it fails (not a struct sequence), use full length - let other_len = get_visible_len(other, vm).unwrap_or(other_tuple.len()); + let other_len = match get_visible_len(other, vm) { + Ok(n) => n.min(other_tuple.len()), + Err(_) => other_tuple.len(), + }; let zelf_visible = &zelf_tuple.as_slice()[..zelf_len]; let other_visible = &other_tuple.as_slice()[..other_len]; // Use the same comparison logic as regular tuples zelf_visible .iter() .richcompare(other_visible.iter(), op, vm) .map(|v| Either::B(PyComparisonValue::Implemented(v))) }crates/vm/src/stdlib/time.rs (1)
309-326:gmtime()must return UTC offset and zone, not local timezone.Both
gmtime()(line 314) andlocaltime()(line 325) callStructTimeData::new(), which always derivestm_gmtoffandtm_zonefromchrono::Local. This is incorrect: per CPython behavior,gmtime()must returntm_gmtoff = 0andtm_zone = "UTC".Split
StructTimeData::newinto UTC and local variants:impl StructTimeData { + fn new_utc(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { + Self { + tm_year: vm.ctx.new_int(tm.year()).into(), + tm_mon: vm.ctx.new_int(tm.month()).into(), + tm_mday: vm.ctx.new_int(tm.day()).into(), + tm_hour: vm.ctx.new_int(tm.hour()).into(), + tm_min: vm.ctx.new_int(tm.minute()).into(), + tm_sec: vm.ctx.new_int(tm.second()).into(), + tm_wday: vm.ctx.new_int(tm.weekday().num_days_from_monday()).into(), + tm_yday: vm.ctx.new_int(tm.ordinal()).into(), + tm_isdst: vm.ctx.new_int(isdst).into(), + tm_gmtoff: vm.ctx.new_int(0).into(), + tm_zone: vm.ctx.new_str("UTC").into(), + } + } + - fn new(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { + fn new_local(vm: &VirtualMachine, tm: NaiveDateTime, isdst: i32) -> Self { let local_time = chrono::Local.from_local_datetime(&tm).unwrap(); let offset_seconds = local_time.offset().local_minus_utc() + if isdst == 1 { 3600 } else { 0 }; let tz_abbr = local_time.format("%Z").to_string(); Self { tm_year: vm.ctx.new_int(tm.year()).into(), tm_mon: vm.ctx.new_int(tm.month()).into(), tm_mday: vm.ctx.new_int(tm.day()).into(), tm_hour: vm.ctx.new_int(tm.hour()).into(), tm_min: vm.ctx.new_int(tm.minute()).into(), tm_sec: vm.ctx.new_int(tm.second()).into(), tm_wday: vm.ctx.new_int(tm.weekday().num_days_from_monday()).into(), tm_yday: vm.ctx.new_int(tm.ordinal()).into(), tm_isdst: vm.ctx.new_int(isdst).into(), tm_gmtoff: vm.ctx.new_int(offset_seconds).into(), tm_zone: vm.ctx.new_str(tz_abbr).into(), } } } #[pyfunction] fn gmtime( secs: OptionalArg<Either<f64, i64>>, vm: &VirtualMachine, ) -> PyResult<StructTimeData> { let instant = secs.naive_or_utc(vm)?; - Ok(StructTimeData::new(vm, instant, 0)) + Ok(StructTimeData::new_utc(vm, instant, 0)) } #[pyfunction] fn localtime( secs: OptionalArg<Either<f64, i64>>, vm: &VirtualMachine, ) -> PyResult<StructTimeData> { let instant = secs.naive_or_local(vm)?; - Ok(StructTimeData::new(vm, instant, -1)) + Ok(StructTimeData::new_local(vm, instant, -1)) }crates/derive-impl/src/pystructseq.rs (1)
255-278: Skipped-field semantics vs into_tuple and docs still seem misaligned
FieldKind::Skippedis documented as a “hidden/skipped field (stored in tuple, but hidden from repr/len/index)”, and the higher-level docs (e.g. the#[pystruct_sequence_data]example incrates/derive/src/lib.rs) still describe#[pystruct_sequence(skip)]as “optional field, not included in tuple”. However,into_tuplecurrently appends bothvisible_fieldsandskipped_fields:let items = vec![ // visible_fields... #(::rustpython_vm::convert::ToPyObject::to_pyobject(self.#skipped_fields, vm),)* ];So the underlying tuple length always includes skipped fields, which conflicts with the “not included in tuple” wording and makes it non-obvious whether
len(), indexing, andtuple(obj)should see those optional fields or not. This was already pointed out in an earlier review; the core concern still seems unresolved.I’d recommend deciding on a single contract and aligning both code and docs with it—for example:
- If “skipped” truly means “not part of the sequence”, change
into_tupleto emit onlyvisible_fields(as previously suggested) and keep skipped data purely as construction-time extras; or- If the intent is “stored but logically hidden”, update the public docs to say so, and ensure the
PyStructSequenceimplementation invmactually hides those tail elements from__len__/__getitem__/ repr so behavior matches the description.Right now it’s hard for users (and for tests) to know what to expect.
🧹 Nitpick comments (3)
crates/vm/src/stdlib/sys.rs (1)
914-924: Verify return type handling: inconsistent with other struct sequence functions.This function returns
AsyncgenHooksDatadirectly, while all other struct sequence functions (lines 453, 458, 745, 755, 815, 820) explicitly callfrom_dataand returnPyTupleRef.For consistency and clarity, consider updating this to match the pattern:
#[pyfunction] -fn get_asyncgen_hooks(vm: &VirtualMachine) -> AsyncgenHooksData { - AsyncgenHooksData { +fn get_asyncgen_hooks(vm: &VirtualMachine) -> PyTupleRef { + PyAsyncgenHooks::from_data( + AsyncgenHooksData { + firstiter: crate::vm::thread::ASYNC_GEN_FIRSTITER + .with_borrow(Clone::clone) + .to_pyobject(vm), + finalizer: crate::vm::thread::ASYNC_GEN_FINALIZER + .with_borrow(Clone::clone) + .to_pyobject(vm), + }, + vm, + ) +}If returning
Datadirectly is intentional and supported by the macro system, please verify it works correctly and consider adding a comment explaining why this function differs from the others.crates/derive-impl/src/pymodule.rs (1)
416-705: StructSequenceItem module emission is correct but could share more with ClassItem
StructSequenceItem::gen_module_itemvalidates that#[pystruct_sequence]is on a struct, usesPyStructSequenceMetato retrievename/module/no_attr, enforces the “either #[pyattr] or no_attr” rule, and then generatesmake_class+ module attribute wiring in parallel toClassItem. Semantically this matches how other type-defining py* items behave.If you find future evolution is duplicating more logic between
ClassItemandStructSequenceItem(e.g., defaulting the module name into the attribute meta, or tweaking no_attr behavior), consider extracting a small helper that takes{ ident, meta, py_attrs, cfgs }and returns{ class_new, py_names, tokens }so both paths stay in lockstep. Right now it’s fine; this is just a maintainability suggestion.crates/derive-impl/src/pystructseq.rs (1)
412-497: impl_pystruct_sequence discards the original struct body and generics; consider validating or forwardingThe
impl_pystruct_sequencemacro currently:
- Requires the input to be a struct via
let Item::Struct(struct_item) = item, but- Ignores
struct_item’s fields, generics, and any additional attributes, and- Re-emits a fresh zero-sized
#pytype_vis struct #pytype_ident;before adding the trait impls.Given the docs state that this macro is intended for an “empty struct to create a Python type that wraps a Data struct”, this is probably fine for the intended usage, but it has a couple of sharp edges:
- If someone accidentally adds fields or generic parameters to the annotated struct, they’ll be silently dropped, and generic uses like
Foo<T>will start failing with confusing type errors because only a monomorphicFoois emitted.- Any extra attributes or doc comments on the original struct are also lost.
Two possible improvements:
- let output = quote! { - // The Python type struct (user-defined, possibly empty) - #pytype_vis struct #pytype_ident; + // Enforce the “empty, non-generic marker struct” contract up front. + if !struct_item.generics.params.is_empty() || !matches!(struct_item.fields, syn::Fields::Unit) { + bail_span!( + struct_item, + "#[pystruct_sequence] expects an empty, non-generic struct (e.g., `struct PyStructTime;`)" + ); + } + + let output = quote! { + // The Python type struct (user-defined, but treated as an empty marker) + #pytype_vis struct #pytype_ident; // trait impls... };Or, if you’d like to be more flexible, you could instead emit
#struct_itemverbatim and only add the trait impls, so any fields/generics/attributes the user wrote are preserved.Either way, making this explicit would save users from subtle surprises if they ever extend the marker struct.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/datetimetester.pyis excluded by!Lib/**Lib/test/test_logging.pyis excluded by!Lib/**Lib/test/test_structseq.pyis excluded by!Lib/**
📒 Files selected for processing (16)
crates/derive-impl/src/lib.rs(1 hunks)crates/derive-impl/src/pymodule.rs(9 hunks)crates/derive-impl/src/pystructseq.rs(3 hunks)crates/derive/src/lib.rs(1 hunks)crates/stdlib/src/grp.rs(5 hunks)crates/stdlib/src/resource.rs(4 hunks)crates/vm/src/stdlib/nt.rs(2 hunks)crates/vm/src/stdlib/os.rs(9 hunks)crates/vm/src/stdlib/posix.rs(3 hunks)crates/vm/src/stdlib/pwd.rs(7 hunks)crates/vm/src/stdlib/sys.rs(21 hunks)crates/vm/src/stdlib/time.rs(7 hunks)crates/vm/src/types/mod.rs(1 hunks)crates/vm/src/types/structseq.rs(4 hunks)crates/vm/src/vm/context.rs(1 hunks)crates/vm/src/vm/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/vm/src/stdlib/posix.rs
- crates/vm/src/vm/context.rs
- crates/vm/src/types/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/stdlib/nt.rscrates/stdlib/src/resource.rscrates/vm/src/stdlib/pwd.rscrates/stdlib/src/grp.rscrates/derive-impl/src/pystructseq.rscrates/vm/src/vm/mod.rscrates/derive/src/lib.rscrates/derive-impl/src/lib.rscrates/vm/src/types/structseq.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/sys.rscrates/derive-impl/src/pymodule.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/stdlib/src/resource.rscrates/derive/src/lib.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.
Applied to files:
crates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-08-26T05:20:54.540Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.
Applied to files:
crates/vm/src/types/structseq.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/os.rscrates/vm/src/stdlib/sys.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/os.rscrates/vm/src/stdlib/sys.rs
🧬 Code graph analysis (10)
crates/stdlib/src/resource.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/stdlib/pwd.rs (3)
crates/derive-impl/src/lib.rs (3)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)pyclass(41-47)crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/stdlib/src/grp.rs (2)
crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/derive-impl/src/pystructseq.rs (1)
crates/vm/src/types/structseq.rs (3)
Self(280-283)try_from_elements(171-173)into_tuple(166-166)
crates/derive/src/lib.rs (1)
crates/derive-impl/src/lib.rs (2)
pystruct_sequence(61-63)pystruct_sequence_data(65-67)
crates/derive-impl/src/lib.rs (2)
crates/derive/src/lib.rs (2)
pystruct_sequence(235-239)pystruct_sequence_data(264-268)crates/derive-impl/src/pystructseq.rs (2)
impl_pystruct_sequence(412-500)impl_pystruct_sequence_data(166-301)
crates/vm/src/stdlib/os.rs (2)
crates/derive-impl/src/lib.rs (3)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)pyclass(41-47)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/stdlib/time.rs (4)
crates/vm/src/types/structseq.rs (1)
struct_sequence_new(20-69)crates/derive-impl/src/lib.rs (2)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)crates/derive/src/lib.rs (2)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)crates/derive-impl/src/pystructseq.rs (1)
module(346-368)
crates/vm/src/stdlib/sys.rs (4)
crates/vm/src/types/structseq.rs (2)
from_data(186-191)Self(280-283)crates/derive-impl/src/lib.rs (3)
pystruct_sequence_data(65-67)pystruct_sequence(61-63)pyclass(41-47)crates/derive/src/lib.rs (3)
pystruct_sequence_data(264-268)pystruct_sequence(235-239)pyclass(124-128)crates/derive-impl/src/pystructseq.rs (2)
module(346-368)no_attr(397-399)
crates/derive-impl/src/pymodule.rs (3)
crates/derive-impl/src/util.rs (12)
attr_name(106-108)inner(102-102)inner(283-283)inner(310-312)inner(324-326)inner(358-360)inner(381-383)inner(485-487)class_name(387-410)class_name(491-525)syn(202-202)module(428-457)crates/derive-impl/src/pystructseq.rs (4)
inner(316-318)class_name(322-344)no_attr(397-399)module(346-368)crates/derive-impl/src/from_args.rs (1)
meta(95-95)
⏰ 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: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (18)
crates/vm/src/vm/mod.rs (1)
468-474: LGTM!The migration from
UnraisableHookArgstoUnraisableHookArgsDatais consistent with the data-backed wrapper pattern introduced across the PR. All fields are properly initialized.crates/stdlib/src/grp.rs (1)
16-96: LGTM!The migration to the data-backed pattern is well-executed:
GroupDataserves as the data struct with#[pystruct_sequence_data]PyGroupserves as the Python-facing wrapper withdata = "GroupData"- All construction and conversion logic properly updated to use
GroupData- Field structure and semantics preserved
crates/vm/src/stdlib/nt.rs (1)
182-209: LGTM!The function signature and return construction correctly updated to use
TerminalSizeData, aligning with the data-backed wrapper pattern.crates/stdlib/src/resource.rs (1)
66-134: LGTM!The refactor to the data-backed pattern is complete and correct:
RUsageDatawith#[pystruct_sequence_data]holds all rusage fieldsPyRUsagewrapper properly configured withdata = "RUsageData"From<libc::rusage>implementation correctly populates all 16 fields- Function signatures and call sites consistently updated
crates/vm/src/stdlib/pwd.rs (1)
19-116: LGTM!The migration follows the data-backed pattern correctly:
PasswdDataserves as the data struct with proper field definitionsPyPasswdwrapper configured withdata = "PasswdData"From<User>implementation properly handles all password fields with lossy conversion fallbacks- All function signatures and call sites consistently updated
crates/vm/src/stdlib/os.rs (3)
738-868: LGTM!The
StatResultrefactor is well-designed:
StatResultDataproperly separates visible fields (indices 0-6 + unnamed 7-9 for integer seconds) from skipped fields (float times, nanosecond times, reparse_tag)from_statcorrectly populates all fields including the newst_atime_int,st_mtime_int,st_ctime_intand their float/ns variantsPyStatResultwrapper properly configured withslot_newusingstruct_sequence_new- Maintains backward compatibility with Python's
stat_resultinterface
1235-1252: LGTM!The
TimesResultrefactor follows the data-backed pattern correctly:
TimesResultDatawith 5 float fields (user, system, children_user, children_system, elapsed)PyTimesResultwrapper properly configured- Both Windows and Unix implementations updated to construct and return
TimesResultData
1482-1511: LGTM!Both
TerminalSizeandUnameResultfollow the data-backed pattern consistently:
TerminalSizeDatawith columns and lines fieldsPyTerminalSizewrapper properly configuredUnameResultDatawith 5 string fields (sysname, nodename, release, version, machine)PyUnameResultwrapper properly configuredcrates/vm/src/stdlib/time.rs (1)
478-554: LGTM with required fix for gmtime().The refactor to the data-backed pattern is well-executed:
StructTimeDataproperly defines all 11 fields with appropriate#[pystruct_sequence(skip)]annotationsPyStructTimewrapper correctly implementsslot_newusingstruct_sequence_new- All time functions updated to use
StructTimeDatamktimecorrectly interprets input as local time usingchrono::Local.from_local_datetimeHowever, the
gmtime()UTC issue (flagged separately) must be addressed.crates/vm/src/stdlib/sys.rs (5)
3-3: LGTM: Public export change aligns with data-driven pattern.Exposing
UnraisableHookArgsDataallows external code to construct these data structures before wrapping them, which is consistent with the new PyStructSequence architecture.
453-453: LGTM: Consistent from_data pattern.All these functions consistently use the
Py*::from_data(Data, vm)pattern to construct struct sequence instances. The explicit wrapping makes the conversion from data to Python objects clear.Also applies to: 458-458, 745-745, 755-755, 815-815, 820-820
659-671: LGTM: Windows version construction follows the new pattern.The function correctly constructs
WindowsVersionDatafrom the Windows API results and wraps it usingPyWindowsVersion::from_data.
674-674: LGTM: Function signatures updated for data-driven model.Both
_unraisablehookandunraisablehooknow acceptUnraisableHookArgsDatadirectly. The#[pystruct_sequence_data(try_from_object)]attribute on line 1189 enables automatic conversion from Python objects to the data struct.Also applies to: 730-730
929-1202: LGTM: Well-structured data-driven struct sequence definitions.The refactoring consistently applies the new pattern:
- Data structs hold the actual data with
#[pystruct_sequence_data]- Py* wrapper structs expose to Python with
#[pystruct_sequence(...)]- All wrappers correctly use
#[pyclass(with(PyStructSequence))]- Appropriate visibility modifiers (
pubvspub(super)) based on usage- Conditional compilation attributes correctly applied for platform-specific types
slot_newoverrides onPyFlagsandPyVersionInfoproperly prevent direct instantiation from PythonThe direct field access to
IntInfoData::INFO.str_digits_check_thresholdon line 765 is correct with the new data-driven model.crates/derive/src/lib.rs (1)
217-268: pystruct_sequence / pystruct_sequence_data attribute wiring looks consistentThe new attribute macros follow the same pattern as
pyclass/pymodule(parse attributes withPunctuated::parse_terminated, parse item, then delegate toderive_impl), and the docs clearly describe thedata+ wrapper pattern. From this file’s perspective the API surface and delegation look sound.crates/derive-impl/src/lib.rs (1)
61-67: New pystruct_sequence entry points are correctly routed*Both
pystruct_sequenceandpystruct_sequence_datanow take(attr: PunctuatedNestedMeta, item: Item)and simply forward intopystructseq::impl_pystruct_sequence(_data)via the commonresult_to_tokenshelper. This matches the updated attribute-based design and keeps the public API surface minimal.crates/derive-impl/src/pymodule.rs (2)
16-53: AttrName::StructSequence integration under #[pymodule] is coherentAdding
StructSequencetoAttrName, itsDisplay/FromStrimpls, and allowing it inALL_ALLOWED_NAMES(via other file) keeps the attribute parsing model uniform withpyfunction/pyclass/pyexception. From this file’s perspective, recognition of#[pystruct_sequence]under#[pymodule]looks consistent and non-breaking.
223-335: pyattr + pystruct_sequence ordering rules mirror existing behaviorExtending
module_item_newandattrs_to_module_itemsso that#[pyattr]can be followed by#[pystruct_sequence](in addition topyclass/pyfunction/pyexception) preserves the existing “single py* item per pyattr group” invariants and reuses the same validation paths and error messages. The constraints and diagnostics look aligned with what users already expect from other#[py*]combinations.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.