Apply clippy suggestions to switch to let chains #6126
Conversation
WalkthroughBatches of nested if/if-let conditionals across the workspace were collapsed into single guarded conditionals using combined if-let/&& patterns; the workspace MSRV in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor JS as JavaScript
participant W as wasm glue
participant VM as RustPython VM
participant IP as InnerInterperter
JS->>W: invoke wrapped function (JsValue args/kwargs)
W->>VM: stored_vm_from_wasm(wasm_id)
alt VM found
VM->>IP: interp.enter(...)
Note over IP: Args/kwargs conversion now runs inside inner VM context
IP->>IP: convert args/kwargs -> PyObjectRef
IP->>IP: call Python function
IP-->>W: return PyObjectRef / error
W->>VM: try Promise bridge (if Promise type && downcast OK)
alt Promise
W-->>JS: return Promise
else
W-->>JS: return converted result
end
else VM missing
W-->>JS: return Err(JsValue)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
derive-impl/src/compile_bytecode.rs (1)
198-209: Same unwrap risk in compile_path fallbackGuard against panics; also avoid
unwrap()onparent().- let code = compile_path(&path).or_else(|e| { - if cfg!(windows) - && let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap()) - { - let joined = path.parent().unwrap().join(real_path.trim()); - if joined.exists() { - return compile_path(&joined); - } else { - return Err(e); - } - } - Err(e) - }); + let code = compile_path(&path).or_else(|e| { + if cfg!(windows) + && let Ok(canonical) = path.canonicalize() + && let Ok(real_path) = fs::read_to_string(&canonical) + && let Some(parent) = path.parent() + { + let joined = parent.join(real_path.trim()); + if joined.exists() { + return compile_path(&joined); + } else { + return Err(e); + } + } + Err(e) + });
🧹 Nitpick comments (17)
stdlib/src/select.rs (1)
247-251: LGTM: guard-style negative-timeout checkSemantics preserved and clearer flow. Minor: consider avoiding shadowing by renaming the inner binding for readability.
- if let Some(timeout) = timeout + if let Some(t) = timeout - && timeout < 0.0 + && t < 0.0 { return Err(vm.new_value_error("timeout must be positive")); }vm/src/readline.rs (1)
98-101: Drop theexists()check;create_dir_allis idempotentSaves a syscall and avoids a TOCTOU check; behavior stays the same.
- if !path.exists() - && let Some(parent) = path.parent() - { - std::fs::create_dir_all(parent)?; - } + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + }vm/src/stdlib/itertools.rs (1)
989-991: Guard against theoretical overflow whenstopisNoneNot introduced by this PR, but
unwrap()on overflow would panic for an infinite slice after wraparound. Extremely unlikely on 64-bit, still easy to harden.- let (next, ovf) = zelf.next.load().overflowing_add(zelf.step); - zelf.next.store(if ovf { zelf.stop.unwrap() } else { next }); + match zelf.next.load().checked_add(zelf.step) { + Some(next) => zelf.next.store(next), + None => { + if let Some(stop) = zelf.stop { + zelf.next.store(stop); + } else { + // no stop: saturate to avoid panic + zelf.next.store(usize::MAX); + } + } + }stdlib/src/contextvars.rs (1)
381-385: Clarify safety forunsafe { &*cached_ptr }in let-chainSemantics look unchanged, but please document why this unsynchronized read is acceptable (soft cache; validated by
cached_idandidxguards; stale read degrades to miss).Apply this diff to add a safety comment:
- if let Some(cached) = unsafe { &*cached_ptr } + // SAFETY: cached_ptr points into AtomicCell<Option<ContextVarCache>>. + // We only read here and validate with cached_id and idx; a stale read + // merely falls through to the map lookup/update below. + if let Some(cached) = unsafe { &*cached_ptr } && zelf.cached_id.load(Ordering::SeqCst) == ctx.get_id() && cached.idx + 1 == ctxs.len() { return Some(cached.object.clone()); }vm/src/builtins/memory.rs (1)
867-874: Outdated TODO; optionally fold the two write paths into one condition.Both branches assign via pack_single(0, ...). You can drop the TODO and combine with matches! for clarity.
- if self.desc.ndim() == 0 { - // TODO: merge branches when we got conditional if let - if needle.is(&vm.ctx.ellipsis) { - return self.pack_single(0, value, vm); - } else if let Some(tuple) = needle.downcast_ref::<PyTuple>() - && tuple.is_empty() - { - return self.pack_single(0, value, vm); - } + if self.desc.ndim() == 0 { + if needle.is(&vm.ctx.ellipsis) + || matches!(needle.downcast_ref::<PyTuple>(), Some(t) if t.is_empty()) + { + return self.pack_single(0, value, vm); + }stdlib/src/unicodedata.rs (1)
141-146: Nit: redundant age check afterextract_char
extract_charalready returnsSome(c)only ifcheck_age(c)is true. You can drop the secondself.check_age(c)for a tiny simplification.Apply:
- if let Some(c) = c - && self.check_age(c) - && let Some(name) = c.to_char().and_then(unicode_names2::name) + if let Some(c) = c + && let Some(name) = c.to_char().and_then(unicode_names2::name) { return Ok(vm.ctx.new_str(name.to_string()).into()); }pylib/build.rs (1)
12-20: Let-chain refactor is correct; consider trimming and non-panicking path to string.Trim possible trailing newlines from Lib’s contents, avoid panic on non-UTF-8 paths, and make changes re-run when
Libchanges.Apply:
- if cfg!(windows) - && let Ok(real_path) = std::fs::read_to_string("Lib") - { - let canonicalized_path = std::fs::canonicalize(real_path) + if cfg!(windows) + && let Ok(real_path) = std::fs::read_to_string("Lib") + { + println!("cargo:rerun-if-changed=Lib"); + let canonicalized_path = std::fs::canonicalize(real_path.trim()) .expect("failed to resolve RUSTPYTHONPATH during build time"); println!( "cargo:rustc-env=win_lib_path={}", - canonicalized_path.to_str().unwrap() + canonicalized_path.to_string_lossy() ); }vm/src/stdlib/sys.rs (2)
186-196: Minor: avoid into_os_string/into_string churn.Use
to_string_lossy()to sidestep ownership churn and Unicode fallibility.- if let Some(exec_path) = env::args_os().next() - && let Ok(path) = which::which(exec_path) + if let Some(exec_path) = env::args_os().next() + && let Ok(path) = which::which(&exec_path) { - return ctx - .new_str( - path.into_os_string() - .into_string() - .unwrap_or_else(|p| p.to_string_lossy().into_owned()), - ) - .into(); + return ctx.new_str(path.to_string_lossy().into_owned()).into(); }
206-216: Prefer Path joining to manual “{dir}/{file}” formatting.This is more portable (Windows path separators) and avoids intermediate Unicode checks.
- if let Ok(dir) = env::current_dir() - && let Ok(dir) = dir.into_os_string().into_string() - { - return ctx - .new_str(format!( - "{}/{}", - dir, - exec_path.strip_prefix("./").unwrap_or(&exec_path) - )) - .into(); - } + if let Ok(dir) = env::current_dir() { + let joined = dir.join(exec_path.strip_prefix("./").unwrap_or(&exec_path)); + return ctx.new_str(joined.to_string_lossy().into_owned()).into(); + }vm/src/vm/vm_ops.rs (1)
302-309: slot_c resolution via chained guards is fine; consider readabilityLogic matches prior nested checks and ensures
slot_cdiffers fromslot_a/slot_b. Optionally, extract the inequality checks into locals to tame line length.- if let Some(slot_c) = class_c.slots.as_number.left_ternary_op(op_slot) - && slot_a.is_some_and(|slot_a| !std::ptr::fn_addr_eq(slot_a, slot_c)) - && slot_b.is_some_and(|slot_b| !std::ptr::fn_addr_eq(slot_b, slot_c)) + if let Some(slot_c) = class_c.slots.as_number.left_ternary_op(op_slot) + && { + let a_diff = slot_a.is_some_and(|sa| !std::ptr::fn_addr_eq(sa, slot_c)); + let b_diff = slot_b.is_some_and(|sb| !std::ptr::fn_addr_eq(sb, slot_c)); + a_diff && b_diff + } {vm/src/builtins/genericalias.rs (3)
379-395: Use ctx.none() consistently in equality checks.Elsewhere in this file you use
vm.ctx.none(). For consistency (and to avoid confusion between field vs. accessor), prefervm.ctx.none()here.- if let Ok(sub_args) = item.get_attr(identifier!(vm, __typing_unpacked_tuple_args__), vm) - && !sub_args.is(&vm.ctx.none) + if let Ok(sub_args) = item.get_attr(identifier!(vm, __typing_unpacked_tuple_args__), vm) + && !sub_args.is(&vm.ctx.none()) && let Ok(tuple) = sub_args.try_to_ref::<PyTuple>(vm)
423-434: Same nit: prefer ctx.none() accessor for clarity.- if let Ok(prepare) = param.get_attr(identifier!(vm, __typing_prepare_subst__), vm) - && !prepare.is(&vm.ctx.none) + if let Ok(prepare) = param.get_attr(identifier!(vm, __typing_prepare_subst__), vm) + && !prepare.is(&vm.ctx.none()) { // Call prepare(self, item) item = if item.try_to_ref::<PyTuple>(vm).is_ok() {
528-535: Simplify control flow: avoid return inside map-closure.This is correct but a bit awkward to read. A straightforward early-return form is clearer.
- PyType::call(&zelf.origin, args, vm).map(|obj| { - if let Err(exc) = obj.set_attr(identifier!(vm, __orig_class__), zelf.to_owned(), vm) - && !exc.fast_isinstance(vm.ctx.exceptions.attribute_error) - && !exc.fast_isinstance(vm.ctx.exceptions.type_error) - { - return Err(exc); - } - Ok(obj) - })? + let obj = PyType::call(&zelf.origin, args, vm)?; + if let Err(exc) = obj.set_attr(identifier!(vm, __orig_class__), zelf.to_owned(), vm) { + if !exc.fast_isinstance(vm.ctx.exceptions.attribute_error) + && !exc.fast_isinstance(vm.ctx.exceptions.type_error) + { + return Err(exc); + } + } + Ok(obj)vm/src/builtins/descriptor.rs (1)
370-376: LGTM: let-chain correctly collapses nested checks. Consider updating comment.Behavior matches the prior logic. The nearby comment mentions only “doc”; this path now (and previously) applies to any member with a same-named class attribute. Consider adjusting the comment for accuracy.
vm/src/vm/method.rs (1)
45-52: LGTM: data-descriptor precedence kept; minor naming nit.Shadowing
clsinside the block is a bit confusing; considerowner/cls_objto avoid shadowing the outercls.- let cls = cls.to_owned().into(); - return descr_get(descr, Some(obj), Some(cls), vm).map(Self::Attribute); + let cls_obj = cls.to_owned().into(); + return descr_get(descr, Some(obj), Some(cls_obj), vm).map(Self::Attribute);vm/src/builtins/type.rs (2)
276-283: Prefer unsigned extraction for flags to avoid negative surprises.Use
to_u64()instead ofto_i64() as u64when reading__abc_tpflags__, so negative ints can’t wrap into huge flag bitmasks.- let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0); - let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64); + let flags_val = int_obj.as_bigint().to_u64().unwrap_or(0); + let abc_flags = PyTypeFlags::from_bits_truncate(flags_val);Apply the same change in the bases scan block.
Also applies to: 287-294
1066-1070: Be robust to potential subclasses of function.Using exact
.class().is(function_type)is fine, butfast_isinstancewould also accept function-like subclasses (if ever introduced).- if let Some(f) = attributes.get_mut(identifier!(vm, __init_subclass__)) - && f.class().is(vm.ctx.types.function_type) + if let Some(f) = attributes.get_mut(identifier!(vm, __init_subclass__)) + && f.fast_isinstance(vm.ctx.types.function_type) { *f = PyClassMethod::from(f.clone()).into_pyobject(vm); } - if let Some(f) = attributes.get_mut(identifier!(vm, __class_getitem__)) - && f.class().is(vm.ctx.types.function_type) + if let Some(f) = attributes.get_mut(identifier!(vm, __class_getitem__)) + && f.fast_isinstance(vm.ctx.types.function_type) { *f = PyClassMethod::from(f.clone()).into_pyobject(vm); }Also applies to: 1072-1076
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (48)
Cargo.toml(1 hunks)compiler/codegen/src/compile.rs(4 hunks)compiler/codegen/src/symboltable.rs(3 hunks)derive-impl/src/compile_bytecode.rs(2 hunks)jit/src/instructions.rs(2 hunks)pylib/build.rs(1 hunks)src/shell.rs(1 hunks)stdlib/src/contextvars.rs(1 hunks)stdlib/src/math.rs(1 hunks)stdlib/src/select.rs(1 hunks)stdlib/src/socket.rs(1 hunks)stdlib/src/syslog.rs(1 hunks)stdlib/src/unicodedata.rs(2 hunks)stdlib/src/zlib.rs(2 hunks)vm/src/builtins/descriptor.rs(1 hunks)vm/src/builtins/enumerate.rs(1 hunks)vm/src/builtins/function.rs(1 hunks)vm/src/builtins/genericalias.rs(4 hunks)vm/src/builtins/int.rs(1 hunks)vm/src/builtins/iter.rs(1 hunks)vm/src/builtins/mappingproxy.rs(1 hunks)vm/src/builtins/memory.rs(3 hunks)vm/src/builtins/object.rs(2 hunks)vm/src/builtins/property.rs(2 hunks)vm/src/builtins/str.rs(1 hunks)vm/src/builtins/super.rs(1 hunks)vm/src/builtins/type.rs(5 hunks)vm/src/byte.rs(1 hunks)vm/src/cformat.rs(1 hunks)vm/src/exceptions.rs(2 hunks)vm/src/object/core.rs(1 hunks)vm/src/protocol/mapping.rs(1 hunks)vm/src/protocol/object.rs(2 hunks)vm/src/readline.rs(1 hunks)vm/src/signal.rs(1 hunks)vm/src/stdlib/ast/python.rs(1 hunks)vm/src/stdlib/builtins.rs(2 hunks)vm/src/stdlib/io.rs(1 hunks)vm/src/stdlib/itertools.rs(1 hunks)vm/src/stdlib/sys.rs(3 hunks)vm/src/stdlib/time.rs(1 hunks)vm/src/stdlib/warnings.rs(1 hunks)vm/src/vm/context.rs(1 hunks)vm/src/vm/method.rs(2 hunks)vm/src/vm/mod.rs(3 hunks)vm/src/vm/vm_new.rs(1 hunks)vm/src/vm/vm_ops.rs(3 hunks)wasm/lib/src/convert.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
vm/src/stdlib/io.rscompiler/codegen/src/compile.rsvm/src/object/core.rsderive-impl/src/compile_bytecode.rsvm/src/stdlib/builtins.rsvm/src/stdlib/sys.rsstdlib/src/syslog.rsvm/src/protocol/mapping.rsstdlib/src/math.rsstdlib/src/select.rsvm/src/stdlib/warnings.rsvm/src/builtins/genericalias.rsvm/src/stdlib/itertools.rsjit/src/instructions.rsvm/src/builtins/function.rsvm/src/stdlib/ast/python.rsvm/src/builtins/property.rsvm/src/cformat.rsstdlib/src/unicodedata.rsvm/src/vm/vm_ops.rsvm/src/stdlib/time.rsstdlib/src/socket.rspylib/build.rsvm/src/builtins/iter.rsvm/src/byte.rsvm/src/builtins/int.rsvm/src/builtins/str.rsstdlib/src/zlib.rsvm/src/vm/vm_new.rsvm/src/builtins/descriptor.rsvm/src/vm/context.rsvm/src/vm/method.rsvm/src/signal.rsvm/src/readline.rsvm/src/builtins/memory.rsstdlib/src/contextvars.rsvm/src/builtins/object.rsvm/src/builtins/mappingproxy.rsvm/src/builtins/type.rsvm/src/builtins/enumerate.rsvm/src/exceptions.rsvm/src/protocol/object.rswasm/lib/src/convert.rscompiler/codegen/src/symboltable.rssrc/shell.rsvm/src/vm/mod.rsvm/src/builtins/super.rs
pylib/**/*
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not modify the
pylib/directory directly - its contents are generated automatically
Files:
pylib/build.rs
🧬 Code graph analysis (12)
vm/src/stdlib/builtins.rs (4)
vm/src/builtins/genericalias.rs (1)
type_params(625-625)vm/src/builtins/type.rs (1)
__type_params__(944-954)vm/src/builtins/function.rs (1)
__type_params__(570-572)vm/src/stdlib/typing.rs (1)
__type_params__(128-130)
stdlib/src/syslog.rs (1)
vm/src/stdlib/sys.rs (1)
argv(129-136)
vm/src/protocol/mapping.rs (1)
vm/src/protocol/sequence.rs (1)
find_methods(98-101)
vm/src/builtins/genericalias.rs (2)
vm/src/builtins/union.rs (4)
arg(169-169)arg(178-178)__parameters__(95-97)obj(156-156)vm/src/stdlib/typevar.rs (4)
__typing_prepare_subst__(185-231)__typing_prepare_subst__(510-518)__typing_prepare_subst__(690-698)obj(1002-1002)
vm/src/builtins/property.rs (1)
stdlib/src/unicodedata.rs (1)
name(133-148)
stdlib/src/socket.rs (1)
stdlib/src/unicodedata.rs (1)
name(133-148)
vm/src/vm/method.rs (3)
vm/src/builtins/descriptor.rs (3)
descr_get(60-90)descr_get(356-380)cls(371-371)vm/src/object/core.rs (2)
Self(371-374)dict(699-701)vm/src/builtins/dict.rs (1)
dict(756-756)
vm/src/builtins/mappingproxy.rs (2)
vm/src/protocol/mapping.rs (1)
find_methods(101-103)vm/src/function/protocol.rs (2)
mapping(146-151)with_methods(133-135)
vm/src/builtins/type.rs (6)
vm/src/builtins/descriptor.rs (5)
qualname(240-240)__qualname__(118-120)__qualname__(257-267)class(48-50)class(233-235)vm/src/builtins/function.rs (5)
__qualname__(549-551)__qualname__(819-838)class(442-444)class(843-845)class(879-881)vm/src/builtins/builtin_func.rs (2)
__qualname__(90-110)__qualname__(163-168)vm/src/builtins/classmethod.rs (1)
__qualname__(141-143)vm/src/builtins/staticmethod.rs (1)
__qualname__(105-107)vm/src/builtins/object.rs (1)
__init_subclass__(415-415)
vm/src/protocol/object.rs (2)
vm/src/builtins/descriptor.rs (3)
descr_get(60-90)descr_get(356-380)cls(371-371)vm/src/types/slot.rs (1)
descr_get(901-906)
wasm/lib/src/convert.rs (2)
wasm/lib/src/js_module.rs (6)
args(186-186)args(201-201)args(222-222)new(96-99)new(305-341)new(396-400)wasm/lib/src/vm_class.rs (1)
stored_vm_from_wasm(99-105)
vm/src/vm/mod.rs (1)
vm/src/import.rs (1)
init_importlib_package(32-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). (9)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (63)
vm/src/stdlib/time.rs (1)
94-101: LGTM: consolidated error mapping for negative durationsBehavior matches CPython’s message and keeps non-matching errors intact.
vm/src/builtins/iter.rs (1)
154-160: LGTM: concise rev_length_hint guardEquivalent logic with tighter expression; avoids double nesting.
vm/src/stdlib/itertools.rs (1)
979-983: LGTM on the let-chain refactorSemantics match the prior nested conditionals.
vm/src/stdlib/io.rs (1)
3397-3400: LGTM on chained conditionFunctional parity with the previous nested
if; concise and clear.stdlib/src/syslog.rs (1)
29-38: LGTM on let-chainBehavior preserved; early-return remains clear.
vm/src/signal.rs (1)
38-43: LGTM: compact trigger + handler checkShort-circuiting is correct; no behavior change.
vm/src/builtins/function.rs (1)
283-288: Let-chain for kw-only defaults looks correctSemantics and error propagation are preserved; borrow lifetimes are fine. LGTM.
stdlib/src/zlib.rs (2)
196-201: Set-dictionary guard refactor is equivalentThe chained condition mirrors the prior nested checks (zdict present and wbits < 0). Short-circuiting/binding behavior is correct. LGTM.
576-581: Constructor path uses the same safe let-chainSame semantics as before; only sets the dictionary when both conditions hold. LGTM.
vm/src/protocol/mapping.rs (1)
81-87: Clean let-chain in try_protocolCombining the method presence check with the slot lookup is clear and preserves behavior; subsequent borrow_static use is safe. LGTM.
vm/src/stdlib/warnings.rs (1)
12-16: Flattened import/attr lookup is fineThe let-chain reads well; intentionally discarding call result remains unchanged. LGTM.
vm/src/stdlib/ast/python.rs (1)
36-44: Duplicate-arg detection let-chain is correctThe combined guard matches the old logic and keeps the same error path. LGTM.
vm/src/object/core.rs (1)
195-200: LGTM: let-chain preserves previous semantics under the lockThe combined guard
is_generic && let Some(generic_weakref) = inner.generic_weakrefis correct and avoids the extra nesting while short-circuiting as before. No ownership/aliasing issues sinceOption<NonNull<_>>isCopy.vm/src/cformat.rs (1)
198-201: LGTM: let-chain keeps single-codepoint string path intactThe combined
PyStrcheck andexactly_one()extraction are equivalent to the nested version and return the same error on failure.vm/src/builtins/str.rs (1)
327-333: LGTM: iterator advance logic unchanged with clearer guardThe else-if let-chain mirrors the prior nested condition and correctly updates
positionand byte offset usinglen_wtf8().stdlib/src/socket.rs (1)
2281-2284: LGTM: IPv4 fast-path readably collapsedPattern + parse guard is fine and preserves prior behavior for AF_INET/AF_UNSPEC.
src/shell.rs (1)
70-75: Use byte-based indexing for triple-quote detection
raw_location.start().to_usize()returns a byte offset; indexingsource.chars()with it misaligns on multibyte UTF-8. Replace with byte-safe checks:- let mut iter = source.chars(); - if let Some(quote) = iter.nth(loc) - && iter.next() == Some(quote) - && iter.next() == Some(quote) - { - return ShellExecResult::ContinueLine; - } + let bytes = source.as_bytes(); + if let (Some(&q), Some(&q1), Some(&q2)) = ( + bytes.get(loc), + loc.checked_add(1).and_then(|i| bytes.get(i)), + loc.checked_add(2).and_then(|i| bytes.get(i)), + ) && (q == b'\'' || q == b'"') && q1 == q && q2 == q + { + return ShellExecResult::ContinueLine; + }Add a test where the quote sequence follows a non-ASCII character (e.g.
“é'''”) to verify correct behavior.vm/src/builtins/property.rs (1)
77-81: LGTM on let-chain refactors; behavior unchanged and clearerBoth the
get_property_nameand__isabstractmethod__conversions are faithful and reduce nesting.Also applies to: 252-270
vm/src/builtins/mappingproxy.rs (1)
65-76: LGTM: consolidated constructor guard reads wellOrder-of-evaluation change is benign; exclusions for list/tuple are preserved. Unsafe borrow remains identical.
vm/src/builtins/int.rs (1)
504-531: LGTM: let-chain preserves rounding semantics (half-even) and simplifies control flowTests for cases like
round(15, -1) == 20,round(25, -1) == 20, and negatives should continue to pass.stdlib/src/math.rs (2)
521-526: Let-chain fallback is correct and preserves semantics.
is_err() && let Some(v) = x.try_float_opt(vm)short-circuits as before;v?still surfaces the float-conversion error when present. LGTM.
533-540: Mirrors ceil() change; looks good.Same short-circuiting and error behavior as prior nested conditionals. LGTM.
vm/src/builtins/memory.rs (2)
333-337: Flattened downcast+released check is fine.Behavior unchanged, now clearer. LGTM.
671-675: 0-d getitem let-chain is correct.The empty-tuple path is neatly handled; semantics match prior code. LGTM.
vm/src/builtins/enumerate.rs (1)
114-118: Guarded length-hint read is correct.Chained pattern keeps obj.length(vm)? gated by Active status; early-return remains identical. LGTM.
vm/src/vm/context.rs (2)
390-395: Int cache lookup via let-chain: OK.Short-circuits as before; no ownership pitfalls for
i. LGTM.
401-406: BigInt cache lookup via let-chain: OK.Same semantics with improved readability. LGTM.
vm/src/builtins/super.rs (2)
242-246: Downcast+issubclass combined: OK.Equivalent to prior nested checks; avoids extra nesting. LGTM.
251-256: class path guard consolidation: OK.Order and conditions preserved; behavior unchanged. LGTM.
jit/src/instructions.rs (2)
178-183: Let-chain preserves original jump-when-unterminated semantics — looks goodThe combined
if letwith guards reads well and matches the prior nested checks: only jump when there is a current block, it differs fromtarget_block, and it has no terminating inst.
211-215: Post-pass unterminated-block trap check consolidated cleanlyThe let-chain correctly traps only when a current block exists and lacks a last inst. No behavior change.
vm/src/builtins/object.rs (2)
56-62: Cleaner guard for object.init equalityUsing
if let ... &&to gate the “takes no arguments” TypeError keeps the logic identical and easier to read.
75-101: Abstract-method enforcement refactor is equivalent and tighterThe chained guards plus the single
matchon count reduce nesting while preserving error messages for 1 vs multiple abstract methods.vm/src/stdlib/builtins.rs (2)
968-979: PEP 695: namespace.type_paramsinjection guarded succinctlyThe combined checks (attr exists, is PyTuple, non-empty) before setting
.type_paramskeep semantics intact and minimize lookups.
1001-1011: PEP 695: class__type_params__/__parameters__setup matches prior flowGood use of a single guard to avoid empty/invalid cases; behavior unchanged.
vm/src/exceptions.rs (2)
1538-1542:PyOSError.__reduce__: concise filename2 inclusionThe
if let Ok(filename2) ... && !vm.is_none(...)pattern is a nice, equivalent simplification.
1661-1687:PySyntaxError.__init__: de-duplicated tuple handlingCombining the length check and tuple downcast with a single loop over known fields removes duplication; semantics are preserved.
stdlib/src/unicodedata.rs (1)
124-128:lookup: chained guard is clear and equivalentThe age check combined with the character lookup is correct and keeps control flow tight.
vm/src/stdlib/sys.rs (2)
186-196: Let-chain for which::which lookup looks good.Semantics preserved and clearer control flow.
871-887: Callable validation via let-chains LGTM.Behavior matches prior nested checks; early error paths remain clear.
Also applies to: 880-887
wasm/lib/src/convert.rs (1)
83-90: Guarded conversion for Python functions reads well.The let-chain cleanly ensures a WASM VM id and function type before wrapping.
vm/src/vm/vm_ops.rs (2)
174-182: Let-chain for right-slot dispatch looks correctShort-circuits on
slot_bbeforefast_issubclass, preserves prior behavior, and keeps theslot_b = Nonefallback. No functional change spotted.
279-288: Ternary right-slot let-chain mirrors binary pathSame semantics as before with clearer intent; only calls
slot_bbwhen appropriate and clearsslot_bon NotImplemented. Good refactor.compiler/codegen/src/symboltable.rs (4)
243-249: Let-chain for class free variable flagging is correctThe guard tightens evaluation and preserves behavior of setting
needs_class_closure.
252-258: Let-chain for classdict is consistentMatches the
__class__change; semantics unchanged.
736-744: Future import detection simplified correctlyChained conditionals accurately gate
__future__and setfuture_annotationsonly forannotations. Good.
1035-1051: Type-params disallowed-expression check via let-chain is preciseEvaluates the ban list only within
TypeParamsscope and reports with accurate location. Looks good.compiler/codegen/src/compile.rs (5)
945-950: Qualname: parent-scope GlobalExplicit detection via let-chain is OKOnly inspects
parent_tablewhen present; setsforce_globalas before. No behavior change observed.
957-963: Qualname: mangled parent lookup let-chain is cleanUses mangled name and
GlobalExplicitcheck in one guard; matches prior logic.
2811-2814: Class call: keyword-args presence check simplifiedThe single guard for
argumentsexistence and non-empty keywords is equivalent and clearer.
3531-3534: Mapping pattern: rest ‘_’ validation condensedChained guard forbids
**_succinctly; matches previous semantics.
5379-5384: ReturnConst micro-optimization via let-chain is solidTransforms trailing
LoadConstintoReturnConstin-place; minimal and efficient.vm/src/builtins/genericalias.rs (1)
272-288: LGTM: let-chain refactor preserves semantics and short-circuits correctly.vm/src/vm/vm_new.rs (1)
350-355: LGTM: chained guard preserves triple-quote detection semantics.vm/src/vm/method.rs (1)
60-64: LGTM: object dict fast-path collapsed cleanly.vm/src/vm/mod.rs (3)
366-374: LGTM: importlib init uses let-guard; short-circuits as intended.Note: using
cfg!(feature = "rustpython-compiler")keeps the call type-checked in all builds; the function is available, so this is fine.
718-722: LGTM: capacity guard is equivalent and clearer.
833-845: LGTM: exception context wiring unchanged; ordering is clear.vm/src/protocol/object.rs (2)
223-229: Data-descriptor fast path reads clean and preserves precedence.Early-return when both
descr_getanddescr_setare present matches CPython’s data-descriptor rules. Looks good.
558-562: Guarded class override check is correct.Semantics (mask AttributeError, accept real types, avoid same-class) are preserved with reduced nesting.
vm/src/builtins/type.rs (3)
324-331: Strict qualname type check reads well.Early error on non-str values mirrors CPython behavior; no concerns.
948-951: Silent fallback to empty tuple — confirm intent.If
__type_params__exists but isn’t a tuple, this returns(). Is this deliberate (best-effort) or should it raise to surface misuse? The setter already enforces tuples.
1372-1378: Static-type doc fast path is solid.Combined guard avoids nesting and preserves internal-doc stripping semantics.
| @@ -138,7 +138,7 @@ members = [ | |||
| version = "0.4.0" | |||
| authors = ["RustPython Team"] | |||
| edition = "2024" | |||
| rust-version = "1.87.0" | |||
| rust-version = "1.89.0" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
MSRV bump to 1.89.0: verify CI/toolchain/docs match
Looks good for enabling let-chain guards. Please ensure CI images, rust-toolchain files, and any MSRV docs are updated accordingly across the workspace.
Run to spot divergences:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Rust toolchain files:"
fd -HI 'rust-toolchain*' -x sh -c 'echo "==> {}"; cat "{}" 2>/dev/null || true'
echo
echo "Per-crate rust-version not using workspace or mismatched:"
rg -n --type toml -C2 '^\s*rust-version\s*=\s*"(.*?)"' | sed -n '1,200p'
echo
echo "Mentions of MSRV or minimum Rust in docs:"
fd -H -t f -a README* docs --exec rg -nC1 -i 'MSRV|Minimum Rust|rustc [0-9]+\.[0-9]+'Length of output: 918
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Searching for MSRV mentions in README.md"
rg -nC2 'MSRV|Minimum Rust|rustc [0-9]+\.[0-9]+' README.md || true
echo
if [ -d docs ]; then
echo "Searching for MSRV mentions in docs/"
rg -nC2 'MSRV|Minimum Rust|rustc [0-9]+\.[0-9]+' docs || true
fiLength of output: 204
Pin the workspace toolchain to 1.89.0
rust-toolchain.toml still reads channel = "stable"—update it to channel = "1.89.0" (and ensure your CI images reference that exact version). No MSRV references were found in README.md or a docs/ directory.
🤖 Prompt for AI Agents
In Cargo.toml at line 141, the workspace rust-version is pinned to "1.89.0" but
rust-toolchain.toml still uses channel = "stable"; update rust-toolchain.toml to
set channel = "1.89.0" so the workspace toolchain matches the pinned MSRV, and
update any CI image references to use Rust 1.89.0 explicitly (replace generic
"stable" tags with "1.89.0" or the appropriate image tag) to ensure consistent
toolchain across local, CI, and docs.
| if cfg!(windows) | ||
| && let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap()) | ||
| { | ||
| return fs::read_dir(real_path.trim()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid panic in error-fallback path (unwrap in or_else)
path.canonicalize().unwrap() can panic while handling an error; keep this path panic-free.
- .or_else(|e| {
- if cfg!(windows)
- && let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap())
- {
- return fs::read_dir(real_path.trim());
- }
- Err(e)
- })
+ .or_else(|e| {
+ if cfg!(windows)
+ && let Ok(canonical) = path.canonicalize()
+ && let Ok(real_path) = fs::read_to_string(&canonical)
+ {
+ return fs::read_dir(real_path.trim());
+ }
+ Err(e)
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg!(windows) | |
| && let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap()) | |
| { | |
| return fs::read_dir(real_path.trim()); | |
| } | |
| fs::read_dir(path).or_else(|e| { | |
| if cfg!(windows) | |
| && let Ok(canonical) = path.canonicalize() | |
| && let Ok(real_path) = fs::read_to_string(&canonical) | |
| { | |
| return fs::read_dir(real_path.trim()); | |
| } | |
| Err(e) | |
| }) |
🤖 Prompt for AI Agents
In derive-impl/src/compile_bytecode.rs around lines 143-147, the error-fallback
path uses path.canonicalize().unwrap() which can panic; replace the unwrap with
safe error handling by first attempting canonicalize and only proceeding if it
succeeds (e.g., if let Ok(canonical) = path.canonicalize() { if let
Ok(real_path) = fs::read_to_string(&canonical) { return
fs::read_dir(real_path.trim()); } } ), so both canonicalize and read_to_string
are checked for Ok before calling read_dir, keeping the fallback panic-free.
| if matches!(af, c::AF_INET | c::AF_UNSPEC) | ||
| && !name.contains('%') | ||
| && let Ok(addr) = name.parse::<Ipv6Addr>() | ||
| { | ||
| return Ok(SocketAddr::V6(net::SocketAddrV6::new(addr, 0, 0, 0))); | ||
| } |
There was a problem hiding this comment.
Bug: IPv6 branch checks the wrong address family constant
This should gate on AF_INET6 | AF_UNSPEC, not AF_INET | AF_UNSPEC. As written, it may accept IPv6 literals even when af == AF_INET, and will skip the fast path when af == AF_INET6.
Apply this fix:
- if matches!(af, c::AF_INET | c::AF_UNSPEC)
+ if matches!(af, c::AF_INET6 | c::AF_UNSPEC)
&& !name.contains('%')
&& let Ok(addr) = name.parse::<Ipv6Addr>()
{
return Ok(SocketAddr::V6(net::SocketAddrV6::new(addr, 0, 0, 0)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if matches!(af, c::AF_INET | c::AF_UNSPEC) | |
| && !name.contains('%') | |
| && let Ok(addr) = name.parse::<Ipv6Addr>() | |
| { | |
| return Ok(SocketAddr::V6(net::SocketAddrV6::new(addr, 0, 0, 0))); | |
| } | |
| if matches!(af, c::AF_INET6 | c::AF_UNSPEC) | |
| && !name.contains('%') | |
| && let Ok(addr) = name.parse::<Ipv6Addr>() | |
| { | |
| return Ok(SocketAddr::V6(net::SocketAddrV6::new(addr, 0, 0, 0))); | |
| } |
🤖 Prompt for AI Agents
In stdlib/src/socket.rs around lines 2285 to 2290, the IPv6 fast-path
incorrectly checks for c::AF_INET instead of c::AF_INET6; change the matches!
predicate to match c::AF_INET6 | c::AF_UNSPEC so the branch only triggers for
IPv6 or unspecified families, keeping the other guard conditions (no '%' and
name.parse::<Ipv6Addr>()) unchanged.
| if !obj.fast_isinstance(vm.ctx.types.str_type) | ||
| && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x)) | ||
| { | ||
| return elements; |
There was a problem hiding this comment.
Bug: returning bare elements breaks the function’s return type
elements is a Vec<u8> bound from Ok(elements), but the function returns PyResult<Vec<u8>>. This should return Ok(elements); otherwise it will not compile (or would be semantically wrong if it did).
Apply this fix:
- if !obj.fast_isinstance(vm.ctx.types.str_type)
- && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x))
- {
- return elements;
- }
+ if !obj.fast_isinstance(vm.ctx.types.str_type)
+ && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x))
+ {
+ return Ok(elements);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if !obj.fast_isinstance(vm.ctx.types.str_type) | |
| && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x)) | |
| { | |
| return elements; | |
| if !obj.fast_isinstance(vm.ctx.types.str_type) | |
| && let Ok(elements) = vm.map_iterable_object(obj, |x| value_from_object(vm, &x)) | |
| { | |
| return Ok(elements); | |
| } |
🤖 Prompt for AI Agents
In vm/src/byte.rs around lines 11 to 14, the match binds elements as a Vec<u8>
from Ok(elements) but the function signature returns PyResult<Vec<u8>>; change
the return to return Ok(elements) instead of returning bare elements so the
types match and the function compiles and behaves correctly.
| if let Some(ref kwargs) = kwargs { | ||
| for pair in object_entries(kwargs) { | ||
| let (key, val) = pair?; | ||
| py_func_args | ||
| .kwargs | ||
| .insert(js_sys::JsString::from(key).into(), js_to_py(vm, val)); | ||
| } |
There was a problem hiding this comment.
Likely type mismatch inserting kwargs keys (PyStrRef expected).
FuncArgs.kwargs typically uses PyStrRef keys; inserting js_sys::JsString via .into() is unlikely to compile. Convert to Rust String then intern/new_str.
Use the VM to produce a Python string key:
- py_func_args
- .kwargs
- .insert(js_sys::JsString::from(key).into(), js_to_py(vm, val));
+ let key = String::from(js_sys::JsString::from(key));
+ py_func_args
+ .kwargs
+ .insert(vm.ctx.intern_str(&key).to_owned(), js_to_py(vm, val));If kwargs expects plain String keys in this crate version, adjust accordingly, but avoid relying on Into from JsString.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(ref kwargs) = kwargs { | |
| for pair in object_entries(kwargs) { | |
| let (key, val) = pair?; | |
| py_func_args | |
| .kwargs | |
| .insert(js_sys::JsString::from(key).into(), js_to_py(vm, val)); | |
| } | |
| if let Some(ref kwargs) = kwargs { | |
| for pair in object_entries(kwargs) { | |
| let (key, val) = pair?; | |
| // Convert the JS string key into a Rust String and intern it as a PyStrRef | |
| let key = String::from(js_sys::JsString::from(key)); | |
| py_func_args | |
| .kwargs | |
| .insert(vm.ctx.intern_str(&key).to_owned(), js_to_py(vm, val)); | |
| } | |
| } |
fd1e7ad to
1546204
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wasm/lib/src/convert.rs (1)
111-117: Kwargs key type mismatch (likely compile error) — convert keys to PyStrRef.
FuncArgs.kwargsexpectsPyStrRefkeys. InsertingJsStringvia.into()won’t type-check. Use the VM to intern a RustStringintoPyStrRef.Apply:
- py_func_args - .kwargs - .insert(js_sys::JsString::from(key).into(), js_to_py(vm, val)); + let key = String::from(js_sys::JsString::from(key)); + py_func_args + .kwargs + .insert(vm.ctx.intern_str(&key).to_owned(), js_to_py(vm, val));
🧹 Nitpick comments (7)
vm/src/readline.rs (1)
98-102: Prefer creating the parent unconditionally; drop theexists()check.
create_dir_allis idempotent and avoids a TOCTOU + extra metadata syscall. Creating the parent regardless of file existence is simpler and equally safe.- if !path.exists() - && let Some(parent) = path.parent() - { - std::fs::create_dir_all(parent)?; - } + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + }vm/src/stdlib/nt.rs (1)
366-380: Avoidunwrap()at FFI boundary; map NUL conversion error to a Python exception.
WideCString::from_os_str(parent).unwrap()can panic on interior NUL. Prefer error propagation to keep VM stability consistent with surrounding conversions.- let parent = widestring::WideCString::from_os_str(parent).unwrap(); + let parent = widestring::WideCString::from_os_str(parent) + .map_err(|e| e.to_pyexception(vm))?;stdlib/src/syslog.rs (1)
29-38: Let-chain looks good; consider robust basename extraction (handles '/' and uses last separator).Current logic uses the first backslash and includes it in the returned slice. Prefer taking the last path separator and support both Windows and Unix separators.
- return Some( - PyStr::from(match argv.find('\\') { - Some(value) => &argv[value..], - None => argv, - }) - .into_ref(&vm.ctx), - ); + return Some( + PyStr::from(argv.rsplit(['\\', '/']).next().unwrap_or(argv)) + .into_ref(&vm.ctx), + );stdlib/src/sqlite.rs (1)
2941-2956: Avoid O(n) char traversal; use byte check for preceding space.
chars().nth(bracket_pos - 1)treatsbracket_posas a char index; it’s a byte offset fromfind. Checking the preceding byte is simpler and avoids extra iteration.- let end_pos = if bracket_pos > 0 - && name_str.chars().nth(bracket_pos - 1) == Some(' ') - { + let end_pos = if bracket_pos > 0 + && name_str.as_bytes().get(bracket_pos - 1) == Some(&b' ') + { bracket_pos - 1 } else { bracket_pos };vm/src/vm/vm_new.rs (1)
350-355: Replace char-iterator triple-quote detection with byte-slicestarts_with
raw_location.start().to_usize()yields a byte offset; slicing and callingstarts_with("'''")orstarts_with("\"\"\"")is more concise, avoids multiple iterator steps, and safely respects UTF-8 boundaries.- if let Some(quote) = iter.nth(loc) - && iter.next() == Some(quote) - && iter.next() == Some(quote) - { - is_incomplete = true; - } + if let Some(rest) = source.get(loc..) { + if rest.starts_with("'''") || rest.starts_with("\"\"\"") { + is_incomplete = true; + } + }stdlib/src/contextvars.rs (1)
381-385: Short-circuit safe checks before unsafe deref.Evaluate the cheap, safe ID check first, then perform the unsafe as_ptr deref to minimize time spent borrowing potentially-mutated memory.
- if let Some(cached) = unsafe { &*cached_ptr } - && zelf.cached_id.load(Ordering::SeqCst) == ctx.get_id() - && cached.idx + 1 == ctxs.len() + if zelf.cached_id.load(Ordering::SeqCst) == ctx.get_id() + && let Some(cached) = unsafe { &*cached_ptr } + && cached.idx + 1 == ctxs.len() { return Some(cached.object.clone()); }wasm/lib/src/convert.rs (1)
87-90: Avoid double unwrap on push_held_rc; surface errors instead of panicking.
unwrap().unwrap()can panic at conversion time. Prefer propagating to JS as an error or failing closed (e.g., returning a throwing closure). Minimal improvement is to useexpect(...); better is to defer holding/upgrading into the closure and returnErron failure.Example minimal change:
- let weak_py_obj = wasm_vm.push_held_rc(py_obj).unwrap().unwrap(); + let weak_py_obj = wasm_vm + .push_held_rc(py_obj) + .expect("failed to push held PyObject into WASM VM") + .expect("failed to obtain weak ref for held PyObject");Preferable (error-returning) approach inside the closure body:
// inside the closure, before upgrading: let weak_py_obj = match wasm_vm.push_held_rc(py_obj.clone()) { Ok(weak) => weak.map_err(|e| e)?, // JsValue error Err(py_err) => return Err(py_err_to_js_err(vm, &py_err)), };If you want, I can provide a full diff moving the hold into the closure and aligning return types.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (52)
Cargo.toml(1 hunks)common/src/fileutils.rs(1 hunks)compiler/codegen/src/compile.rs(4 hunks)compiler/codegen/src/symboltable.rs(3 hunks)derive-impl/src/compile_bytecode.rs(2 hunks)jit/src/instructions.rs(2 hunks)pylib/build.rs(1 hunks)src/shell.rs(1 hunks)stdlib/src/contextvars.rs(1 hunks)stdlib/src/math.rs(1 hunks)stdlib/src/select.rs(1 hunks)stdlib/src/socket.rs(1 hunks)stdlib/src/sqlite.rs(2 hunks)stdlib/src/syslog.rs(1 hunks)stdlib/src/unicodedata.rs(2 hunks)stdlib/src/zlib.rs(2 hunks)vm/src/builtins/descriptor.rs(1 hunks)vm/src/builtins/enumerate.rs(1 hunks)vm/src/builtins/function.rs(1 hunks)vm/src/builtins/genericalias.rs(4 hunks)vm/src/builtins/int.rs(1 hunks)vm/src/builtins/iter.rs(1 hunks)vm/src/builtins/mappingproxy.rs(1 hunks)vm/src/builtins/memory.rs(3 hunks)vm/src/builtins/object.rs(2 hunks)vm/src/builtins/property.rs(2 hunks)vm/src/builtins/str.rs(1 hunks)vm/src/builtins/super.rs(1 hunks)vm/src/builtins/type.rs(5 hunks)vm/src/byte.rs(1 hunks)vm/src/cformat.rs(1 hunks)vm/src/exceptions.rs(2 hunks)vm/src/object/core.rs(1 hunks)vm/src/protocol/mapping.rs(1 hunks)vm/src/protocol/object.rs(2 hunks)vm/src/readline.rs(1 hunks)vm/src/signal.rs(1 hunks)vm/src/stdlib/ast/python.rs(1 hunks)vm/src/stdlib/builtins.rs(2 hunks)vm/src/stdlib/io.rs(1 hunks)vm/src/stdlib/itertools.rs(1 hunks)vm/src/stdlib/nt.rs(1 hunks)vm/src/stdlib/sys.rs(3 hunks)vm/src/stdlib/time.rs(1 hunks)vm/src/stdlib/warnings.rs(1 hunks)vm/src/vm/context.rs(1 hunks)vm/src/vm/method.rs(2 hunks)vm/src/vm/mod.rs(3 hunks)vm/src/vm/vm_new.rs(1 hunks)vm/src/vm/vm_ops.rs(3 hunks)vm/src/windows.rs(1 hunks)wasm/lib/src/convert.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (41)
- vm/src/stdlib/warnings.rs
- vm/src/object/core.rs
- vm/src/builtins/mappingproxy.rs
- derive-impl/src/compile_bytecode.rs
- Cargo.toml
- vm/src/stdlib/time.rs
- vm/src/builtins/iter.rs
- src/shell.rs
- vm/src/builtins/property.rs
- vm/src/exceptions.rs
- vm/src/cformat.rs
- vm/src/stdlib/sys.rs
- vm/src/stdlib/ast/python.rs
- vm/src/builtins/genericalias.rs
- vm/src/vm/mod.rs
- stdlib/src/zlib.rs
- vm/src/stdlib/builtins.rs
- vm/src/vm/method.rs
- vm/src/builtins/enumerate.rs
- stdlib/src/math.rs
- vm/src/builtins/str.rs
- vm/src/stdlib/itertools.rs
- vm/src/stdlib/io.rs
- vm/src/builtins/memory.rs
- vm/src/byte.rs
- pylib/build.rs
- stdlib/src/select.rs
- vm/src/builtins/int.rs
- vm/src/builtins/super.rs
- compiler/codegen/src/compile.rs
- vm/src/builtins/object.rs
- compiler/codegen/src/symboltable.rs
- vm/src/builtins/type.rs
- vm/src/builtins/descriptor.rs
- jit/src/instructions.rs
- stdlib/src/socket.rs
- vm/src/vm/context.rs
- stdlib/src/unicodedata.rs
- vm/src/vm/vm_ops.rs
- vm/src/protocol/object.rs
- vm/src/builtins/function.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
vm/src/signal.rsvm/src/readline.rsstdlib/src/contextvars.rsvm/src/stdlib/nt.rsvm/src/windows.rsvm/src/protocol/mapping.rsstdlib/src/syslog.rsstdlib/src/sqlite.rsvm/src/vm/vm_new.rscommon/src/fileutils.rswasm/lib/src/convert.rs
🧬 Code graph analysis (4)
vm/src/stdlib/nt.rs (1)
vm/src/stdlib/os.rs (1)
errno_err(39-41)
vm/src/protocol/mapping.rs (1)
vm/src/protocol/sequence.rs (1)
find_methods(98-101)
stdlib/src/syslog.rs (1)
vm/src/stdlib/sys.rs (1)
argv(129-136)
wasm/lib/src/convert.rs (2)
wasm/lib/src/js_module.rs (6)
args(186-186)args(201-201)args(222-222)new(96-99)new(305-341)new(396-400)wasm/lib/src/vm_class.rs (1)
stored_vm_from_wasm(99-105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (10)
vm/src/readline.rs (1)
98-105: LGTM on the let-chain conversion.Semantics are preserved; directory creation only happens when a parent exists, and history saving remains unchanged.
vm/src/windows.rs (1)
113-123: Let-chain refactor looks correct and preserves control flow.Binding
errnoand gating withmatches!is semantically equivalent to the prior nested form. No behavior change detected.vm/src/protocol/mapping.rs (1)
81-87: LGTM: let-chain simplifies control flow without changing semantics.Condition combines method lookup and capability check; returned borrow_static usage matches existing patterns.
vm/src/signal.rs (1)
38-43: LGTM: chained guards are correct and short-circuit as intended.Preserves prior behavior: index and invoke only when triggered, handler exists, and is callable.
stdlib/src/sqlite.rs (2)
574-578: LGTM: progress callback let-chain is fine.Shadowing
valis clear; fall-through to-1unchanged.
1721-1725: LGTM: close() release path is equivalent and safe.Resets active statement when present; ownership via
take()remains correct.stdlib/src/contextvars.rs (1)
381-385: Let-chain refactor LGTM.The conversion to an if-let chain reads correctly and preserves the early-return fast path semantics.
common/src/fileutils.rs (1)
84-91: Let-chain refactor looks good; adds .com to executable set (Windows-correct).Clear, preserves behavior, and marking
.comas executable is appropriate on Windows. No further action.wasm/lib/src/convert.rs (2)
83-89: Guarded let-chain is fine.Concise and equivalent to the prior nested checks.
136-140: Promise bridge let-chain LGTM.Matches the project style used elsewhere; behavior unchanged.
| if let Some(cached) = unsafe { &*cached_ptr } | ||
| && zelf.cached_id.load(Ordering::SeqCst) == ctx.get_id() | ||
| && cached.idx + 1 == ctxs.len() | ||
| { | ||
| return Some(cached.object.clone()); |
There was a problem hiding this comment.
Potential UB: dereferencing AtomicCell::as_ptr without synchronization.
Reading unsafe { &*cached_ptr } races with concurrent store/swap on cached, risking undefined behavior. This preexisted the refactor, but the let-chain keeps the hazard.
Consider replacing cached: AtomicCell<Option<ContextVarCache>> with a synchronization primitive suited for non-Copy data:
- RwLock<Option> (simple, safe), or
- arc-swap’s ArcSwapOption (lock-free reads with Arc).
If keeping AtomicCell, you need a design that avoids borrowing through as_ptr (e.g., store Arc<ContextVarCache> and atomically swap/load the Arc).
I can open an issue and sketch a safe refactor using ArcSwapOption if you’d like.
🤖 Prompt for AI Agents
In stdlib/src/contextvars.rs around lines 381-385 the code unsafely dereferences
cached_ptr from AtomicCell::as_ptr which races with concurrent store/swap and
can produce UB; fix by replacing the AtomicCell<Option<ContextVarCache>> with a
synchronization primitive that supports safe non-Copy data (either
RwLock<Option<ContextVarCache>> for a simple fix, or arc-swap’s
ArcSwapOption<ContextVarCache> for lock-free reads). Update all reads to use a
read lock or ArcSwapOption::load (and clone the Arc) instead of dereferencing
raw pointers; alternatively, if you must keep AtomicCell, change it to store
Arc<ContextVarCache> and atomically swap/load Arcs and never borrow through
as_ptr. Ensure all unsafe derefs are removed and replace with safe load/clone or
read-guard access.
1546204 to
5a1371d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
vm/src/builtins/str.rs (1)
327-333: Avoid shadowing the outervaluefor readability.The let-chain rebinding
valueshadows the earliervalue: &Wtf8. Renaming the slice binding reduces cognitive load.Apply:
- } else if let Some(value) = value.get(internal.1..) - && let Some(ch) = value.code_points().next() + } else if let Some(rest) = value.get(internal.1..) + && let Some(ch) = rest.code_points().next() { internal.0.position += 1; internal.1 += ch.len_wtf8(); return Ok(PyIterReturn::Return(ch.to_pyobject(vm))); }compiler/codegen/src/symboltable.rs (3)
243-258: DRY the two free-variable checks with matches! and|=.Avoids temporary bindings and duplication while preserving behavior.
- if let Some(class_symbol) = symbol_table.symbols.get("__class__") - && class_symbol.scope == SymbolScope::Free - { - symbol_table.needs_class_closure = true; - // Note: In CPython, the symbol is removed from the free set, - // but in RustPython we handle this differently during code generation - } + symbol_table.needs_class_closure |= matches!( + symbol_table.symbols.get("__class__"), + Some(sym) if sym.scope == SymbolScope::Free + ); - if let Some(classdict_symbol) = symbol_table.symbols.get("__classdict__") - && classdict_symbol.scope == SymbolScope::Free - { - symbol_table.needs_classdict = true; - // Note: In CPython, the symbol is removed from the free set, - // but in RustPython we handle this differently during code generation - } + symbol_table.needs_classdict |= matches!( + symbol_table.symbols.get("__classdict__"), + Some(sym) if sym.scope == SymbolScope::Free + ); + // Note: CPython removes the symbol from the free set; RustPython handles this during codegen.
736-744: Simplify future-import detection; useany()andas_str().More idiomatic and avoids iterating after the flag is set.
- if let Stmt::ImportFrom(StmtImportFrom { module, names, .. }) = &statement - && module.as_ref().map(|id| id.as_str()) == Some("__future__") - { - for feature in names { - if &feature.name == "annotations" { - self.future_annotations = true; - } - } - } + if let Stmt::ImportFrom(StmtImportFrom { module, names, .. }) = &statement + && module.as_ref().is_some_and(|id| id.as_str() == "__future__") + && names.iter().any(|a| a.name.as_str() == "annotations") + { + self.future_annotations = true; + }
1035-1052: Prefer “assignment expression” wording for consistency with other diagnostics.Elsewhere you use “assignment expression”; align messaging here.
- Expr::Named(_) => Some("named"), + Expr::Named(_) => Some("assignment"),
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (53)
Cargo.toml(1 hunks)common/src/fileutils.rs(1 hunks)compiler/codegen/src/compile.rs(4 hunks)compiler/codegen/src/symboltable.rs(3 hunks)derive-impl/src/compile_bytecode.rs(2 hunks)jit/src/instructions.rs(2 hunks)pylib/build.rs(1 hunks)src/shell.rs(1 hunks)stdlib/src/contextvars.rs(1 hunks)stdlib/src/math.rs(1 hunks)stdlib/src/scproxy.rs(1 hunks)stdlib/src/select.rs(1 hunks)stdlib/src/socket.rs(1 hunks)stdlib/src/sqlite.rs(2 hunks)stdlib/src/syslog.rs(1 hunks)stdlib/src/unicodedata.rs(2 hunks)stdlib/src/zlib.rs(2 hunks)vm/src/builtins/descriptor.rs(1 hunks)vm/src/builtins/enumerate.rs(1 hunks)vm/src/builtins/function.rs(1 hunks)vm/src/builtins/genericalias.rs(4 hunks)vm/src/builtins/int.rs(1 hunks)vm/src/builtins/iter.rs(1 hunks)vm/src/builtins/mappingproxy.rs(1 hunks)vm/src/builtins/memory.rs(3 hunks)vm/src/builtins/object.rs(2 hunks)vm/src/builtins/property.rs(2 hunks)vm/src/builtins/str.rs(1 hunks)vm/src/builtins/super.rs(1 hunks)vm/src/builtins/type.rs(5 hunks)vm/src/byte.rs(1 hunks)vm/src/cformat.rs(1 hunks)vm/src/exceptions.rs(2 hunks)vm/src/object/core.rs(1 hunks)vm/src/protocol/mapping.rs(1 hunks)vm/src/protocol/object.rs(2 hunks)vm/src/readline.rs(1 hunks)vm/src/signal.rs(1 hunks)vm/src/stdlib/ast/python.rs(1 hunks)vm/src/stdlib/builtins.rs(2 hunks)vm/src/stdlib/io.rs(1 hunks)vm/src/stdlib/itertools.rs(1 hunks)vm/src/stdlib/nt.rs(1 hunks)vm/src/stdlib/sys.rs(3 hunks)vm/src/stdlib/time.rs(1 hunks)vm/src/stdlib/warnings.rs(1 hunks)vm/src/vm/context.rs(1 hunks)vm/src/vm/method.rs(2 hunks)vm/src/vm/mod.rs(3 hunks)vm/src/vm/vm_new.rs(1 hunks)vm/src/vm/vm_ops.rs(3 hunks)vm/src/windows.rs(1 hunks)wasm/lib/src/convert.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (47)
- stdlib/src/syslog.rs
- stdlib/src/contextvars.rs
- vm/src/builtins/enumerate.rs
- vm/src/builtins/iter.rs
- derive-impl/src/compile_bytecode.rs
- vm/src/protocol/mapping.rs
- vm/src/stdlib/builtins.rs
- stdlib/src/math.rs
- vm/src/builtins/function.rs
- vm/src/object/core.rs
- stdlib/src/unicodedata.rs
- vm/src/vm/vm_new.rs
- Cargo.toml
- vm/src/byte.rs
- vm/src/builtins/super.rs
- vm/src/exceptions.rs
- vm/src/stdlib/ast/python.rs
- vm/src/vm/vm_ops.rs
- vm/src/stdlib/time.rs
- vm/src/builtins/genericalias.rs
- pylib/build.rs
- stdlib/src/socket.rs
- vm/src/stdlib/itertools.rs
- vm/src/builtins/mappingproxy.rs
- stdlib/src/zlib.rs
- vm/src/builtins/descriptor.rs
- vm/src/signal.rs
- vm/src/builtins/memory.rs
- src/shell.rs
- vm/src/windows.rs
- stdlib/src/select.rs
- stdlib/src/sqlite.rs
- vm/src/readline.rs
- vm/src/builtins/object.rs
- common/src/fileutils.rs
- vm/src/builtins/int.rs
- vm/src/stdlib/sys.rs
- compiler/codegen/src/compile.rs
- vm/src/protocol/object.rs
- vm/src/stdlib/nt.rs
- vm/src/stdlib/io.rs
- vm/src/stdlib/warnings.rs
- vm/src/builtins/type.rs
- vm/src/vm/method.rs
- wasm/lib/src/convert.rs
- jit/src/instructions.rs
- vm/src/builtins/property.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
compiler/codegen/src/symboltable.rsstdlib/src/scproxy.rsvm/src/builtins/str.rsvm/src/cformat.rsvm/src/vm/context.rsvm/src/vm/mod.rs
🧬 Code graph analysis (2)
vm/src/cformat.rs (2)
vm/src/protocol/object.rs (1)
s(371-371)vm/src/frame.rs (1)
spec(2236-2236)
vm/src/vm/mod.rs (1)
vm/src/import.rs (1)
init_importlib_package(32-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). (10)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (8)
stdlib/src/scproxy.rs (2)
89-93: Good let-chain conversion and short-circuit orderingChecking
enabledfirst before attempting the host lookup is the right evaluation order. Readable and efficient.
100-102: Confirm proxy scheme for non-HTTP entriesThe value uses
http://for all protocols, including"https","ftp", and"gopher". If that mirrors CPython/_scproxy behavior, fine; otherwise consider protocol-appropriate schemes.vm/src/cformat.rs (1)
198-201: Let-chain refactor for %c (str case) looks correct.Semantics match prior behavior: only a single code point passes; others fall through to the existing TypeError.
vm/src/vm/mod.rs (3)
366-374: Concise error-guarded init via let-chain; no behavior change.Reads well and keeps the side effect gated behind settings and feature flag.
718-722: Capacity guard flattened; logic preserved.Early return on pathological capacity remains intact and clearer now.
833-845: Exception context linking refactor is sound.Cycle-avoidance loop and final
set___context__are unchanged semantically; the let-chain reduces nesting.vm/src/vm/context.rs (2)
390-395: Let-chain refactor preserves behavior; cache hit path stays correct.Short-circuit evaluation matches the previous nested
if letsemantics, and the computed index is provably non-negative within the guarded range.
401-406: Remove optional refactor suggestion
Noimpl From<&BigInt> for PyIntexists in the codebase, so the current clone-based fallback is necessary.
| let h = std::borrow::Cow::<str>::from(&host); | ||
| let v = if let Some(port) = proxy_dict | ||
| .find(port_key) | ||
| .and_then(|v| v.downcast::<CFNumber>()) | ||
| .and_then(|v| v.to_i32()) | ||
| { | ||
| let h = std::borrow::Cow::<str>::from(&host); | ||
| let v = if let Some(port) = proxy_dict | ||
| .find(port_key) | ||
| .and_then(|v| v.downcast::<CFNumber>()) | ||
| .and_then(|v| v.to_i32()) | ||
| { | ||
| format!("http://{h}:{port}") | ||
| } else { | ||
| format!("http://{h}") | ||
| }; | ||
| result.set_item(proto, vm.new_pyobj(v), vm)?; | ||
| } | ||
| format!("http://{h}:{port}") | ||
| } else { | ||
| format!("http://{h}") | ||
| }; | ||
| result.set_item(proto, vm.new_pyobj(v), vm)?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cow from CFString won’t compile; build the String explicitly
CFString doesn’t coerce to &str, so Cow::<str>::from(&host) is invalid on macOS builds. Convert to String (or format host directly) and avoid the extra Cow.
Apply:
- let h = std::borrow::Cow::<str>::from(&host);
- let v = if let Some(port) = proxy_dict
+ let host_s = host.to_string();
+ let proxy_url = if let Some(port) = proxy_dict
.find(port_key)
.and_then(|v| v.downcast::<CFNumber>())
.and_then(|v| v.to_i32())
{
- format!("http://{h}:{port}")
+ format!("http://{host_s}:{port}")
} else {
- format!("http://{h}")
+ format!("http://{host_s}")
};
- result.set_item(proto, vm.new_pyobj(v), vm)?;
+ result.set_item(proto, vm.new_pyobj(proxy_url), vm)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let h = std::borrow::Cow::<str>::from(&host); | |
| let v = if let Some(port) = proxy_dict | |
| .find(port_key) | |
| .and_then(|v| v.downcast::<CFNumber>()) | |
| .and_then(|v| v.to_i32()) | |
| { | |
| let h = std::borrow::Cow::<str>::from(&host); | |
| let v = if let Some(port) = proxy_dict | |
| .find(port_key) | |
| .and_then(|v| v.downcast::<CFNumber>()) | |
| .and_then(|v| v.to_i32()) | |
| { | |
| format!("http://{h}:{port}") | |
| } else { | |
| format!("http://{h}") | |
| }; | |
| result.set_item(proto, vm.new_pyobj(v), vm)?; | |
| } | |
| format!("http://{h}:{port}") | |
| } else { | |
| format!("http://{h}") | |
| }; | |
| result.set_item(proto, vm.new_pyobj(v), vm)?; | |
| let host_s = host.to_string(); | |
| let proxy_url = if let Some(port) = proxy_dict | |
| .find(port_key) | |
| .and_then(|v| v.downcast::<CFNumber>()) | |
| .and_then(|v| v.to_i32()) | |
| { | |
| format!("http://{host_s}:{port}") | |
| } else { | |
| format!("http://{host_s}") | |
| }; | |
| result.set_item(proto, vm.new_pyobj(proxy_url), vm)?; |
🤖 Prompt for AI Agents
In stdlib/src/scproxy.rs around lines 94 to 104, the code uses
Cow::<str>::from(&host) where host is a CFString-derived value; CFString doesn't
coerce to &str so that conversion fails to compile. Replace the Cow usage by
constructing an owned String from host (e.g. host.to_string() or format!("{}",
host)) and use that String when formatting the URL (or interpolate host directly
into format! calls) so the code produces an owned Rust String before calling
vm.new_pyobj(v).
Raising the
rust-versionto 1.89 prompts clippy to start recommending replacing nestedif lets with let chains. This is the result of doing exactly that and runningcargo clippy --fix --workspace. It's a big diff but I think the prevented rightward creep really does aid in clarity. Reviewed well with whitespace-insensitivity turned on.Summary by CodeRabbit
Refactor
Chores
Notes