Skip to content

Apply clippy suggestions to switch to let chains #6126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 4, 2025
+644 −673
Merged

Apply clippy suggestions to switch to let chains #6126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ members = [
version = "0.4.0"
authors = ["RustPython Team"]
edition = "2024"
rust-version = "1.87.0"
rust-version = "1.89.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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
fi

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

repository = "https://github.com/RustPython/RustPython"
license = "MIT"

Expand Down
11 changes: 5 additions & 6 deletions common/src/fileutils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,13 @@ pub mod windows {
.next_back()
.and_then(|s| String::from_utf16(s).ok());

if let Some(file_extension) = file_extension {
if file_extension.eq_ignore_ascii_case("exe")
if let Some(file_extension) = file_extension
&& (file_extension.eq_ignore_ascii_case("exe")
|| file_extension.eq_ignore_ascii_case("bat")
|| file_extension.eq_ignore_ascii_case("cmd")
|| file_extension.eq_ignore_ascii_case("com")
{
self.st_mode |= 0o111;
}
|| file_extension.eq_ignore_ascii_case("com"))
{
self.st_mode |= 0o111;
}
}
}
Expand Down
37 changes: 18 additions & 19 deletions compiler/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,12 +942,11 @@ impl Compiler {
if stack_size > self.symbol_table_stack.len() {
// We might be in a situation where symbol table isn't pushed yet
// In this case, check the parent symbol table
if let Some(parent_table) = self.symbol_table_stack.last() {
if let Some(symbol) = parent_table.lookup(&current_obj_name) {
if symbol.scope == SymbolScope::GlobalExplicit {
force_global = true;
}
}
if let Some(parent_table) = self.symbol_table_stack.last()
&& let Some(symbol) = parent_table.lookup(&current_obj_name)
&& symbol.scope == SymbolScope::GlobalExplicit
{
force_global = true;
}
} else if let Some(_current_table) = self.symbol_table_stack.last() {
// Mangle the name if necessary (for private names in classes)
Expand All @@ -956,10 +955,10 @@ impl Compiler {
// Look up in parent symbol table to check scope
if self.symbol_table_stack.len() >= 2 {
let parent_table = &self.symbol_table_stack[self.symbol_table_stack.len() - 2];
if let Some(symbol) = parent_table.lookup(&mangled_name) {
if symbol.scope == SymbolScope::GlobalExplicit {
force_global = true;
}
if let Some(symbol) = parent_table.lookup(&mangled_name)
&& symbol.scope == SymbolScope::GlobalExplicit
{
force_global = true;
}
}
}
Expand Down Expand Up @@ -3528,10 +3527,10 @@ impl Compiler {
}

// Validate rest pattern: '_' cannot be used as a rest target
if let Some(rest) = star_target {
if rest.as_str() == "_" {
return Err(self.error(CodegenErrorType::SyntaxError("invalid syntax".to_string())));
}
if let Some(rest) = star_target
&& rest.as_str() == "_"
{
return Err(self.error(CodegenErrorType::SyntaxError("invalid syntax".to_string())));
}

// Step 1: Check if subject is a mapping
Expand Down Expand Up @@ -5377,11 +5376,11 @@ impl Compiler {
}

fn emit_return_value(&mut self) {
if let Some(inst) = self.current_block().instructions.last_mut() {
if let Instruction::LoadConst { idx } = inst.instr {
inst.instr = Instruction::ReturnConst { idx };
return;
}
if let Some(inst) = self.current_block().instructions.last_mut()
&& let Instruction::LoadConst { idx } = inst.instr
{
inst.instr = Instruction::ReturnConst { idx };
return;
}
emit!(self, Instruction::ReturnValue)
}
Expand Down
71 changes: 34 additions & 37 deletions compiler/codegen/src/symboltable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,21 +240,21 @@ fn analyze_symbol_table(symbol_table: &mut SymbolTable) -> SymbolTableResult {
*/
fn drop_class_free(symbol_table: &mut SymbolTable) {
// Check if __class__ is used as a free variable
if let Some(class_symbol) = symbol_table.symbols.get("__class__") {
if 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
}
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
}

// Check if __classdict__ is used as a free variable
if let Some(classdict_symbol) = symbol_table.symbols.get("__classdict__") {
if 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
}
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
}
}

Expand Down Expand Up @@ -733,12 +733,12 @@ impl SymbolTableBuilder {

fn scan_statement(&mut self, statement: &Stmt) -> SymbolTableResult {
use ruff_python_ast::*;
if let Stmt::ImportFrom(StmtImportFrom { module, names, .. }) = &statement {
if 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().map(|id| id.as_str()) == Some("__future__")
{
for feature in names {
if &feature.name == "annotations" {
self.future_annotations = true;
}
}
}
Expand Down Expand Up @@ -1032,26 +1032,23 @@ impl SymbolTableBuilder {
use ruff_python_ast::*;

// Check for expressions not allowed in type parameters scope
if let Some(table) = self.tables.last() {
if table.typ == CompilerScope::TypeParams {
if let Some(keyword) = match expression {
Expr::Yield(_) | Expr::YieldFrom(_) => Some("yield"),
Expr::Await(_) => Some("await"),
Expr::Named(_) => Some("named"),
_ => None,
} {
return Err(SymbolTableError {
error: format!(
"{keyword} expression cannot be used within a type parameter"
),
location: Some(
self.source_file
.to_source_code()
.source_location(expression.range().start()),
),
});
}
if let Some(table) = self.tables.last()
&& table.typ == CompilerScope::TypeParams
&& let Some(keyword) = match expression {
Expr::Yield(_) | Expr::YieldFrom(_) => Some("yield"),
Expr::Await(_) => Some("await"),
Expr::Named(_) => Some("named"),
_ => None,
}
{
return Err(SymbolTableError {
error: format!("{keyword} expression cannot be used within a type parameter"),
location: Some(
self.source_file
.to_source_code()
.source_location(expression.range().start()),
),
});
}

match expression {
Expand Down
24 changes: 12 additions & 12 deletions derive-impl/src/compile_bytecode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ impl CompilationSource {
let mut code_map = HashMap::new();
let paths = fs::read_dir(path)
.or_else(|e| {
if cfg!(windows) {
if let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap()) {
return fs::read_dir(real_path.trim());
}
if cfg!(windows)
&& let Ok(real_path) = fs::read_to_string(path.canonicalize().unwrap())
{
return fs::read_dir(real_path.trim());
}
Comment on lines +143 to 147
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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.

Err(e)
})
Expand Down Expand Up @@ -195,14 +195,14 @@ impl CompilationSource {
})
};
let code = compile_path(&path).or_else(|e| {
if cfg!(windows) {
if 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);
}
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)
Expand Down
17 changes: 9 additions & 8 deletions jit/src/instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,10 +175,11 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
let target_block = self.get_or_create_block(label);

// If the current block isn't terminated, jump:
if let Some(cur) = self.builder.current_block() {
if cur != target_block && self.builder.func.layout.last_inst(cur).is_none() {
self.builder.ins().jump(target_block, &[]);
}
if let Some(cur) = self.builder.current_block()
&& cur != target_block
&& self.builder.func.layout.last_inst(cur).is_none()
{
self.builder.ins().jump(target_block, &[]);
}
// Switch to the target block
if self.builder.current_block() != Some(target_block) {
Expand Down Expand Up @@ -207,10 +208,10 @@ impl<'a, 'b> FunctionCompiler<'a, 'b> {
}

// After processing, if the current block is unterminated, insert a trap or fallthrough
if let Some(cur) = self.builder.current_block() {
if self.builder.func.layout.last_inst(cur).is_none() {
self.builder.ins().trap(TrapCode::user(0).unwrap());
}
if let Some(cur) = self.builder.current_block()
&& self.builder.func.layout.last_inst(cur).is_none()
{
self.builder.ins().trap(TrapCode::user(0).unwrap());
}
Ok(())
}
Expand Down
18 changes: 9 additions & 9 deletions pylib/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,15 @@ fn main() {
process_python_libs("./Lib/**/*");
}

if cfg!(windows) {
if let Ok(real_path) = std::fs::read_to_string("Lib") {
let canonicalized_path = std::fs::canonicalize(real_path)
.expect("failed to resolve RUSTPYTHONPATH during build time");
println!(
"cargo:rustc-env=win_lib_path={}",
canonicalized_path.to_str().unwrap()
);
}
if cfg!(windows)
&& let Ok(real_path) = std::fs::read_to_string("Lib")
{
let canonicalized_path = std::fs::canonicalize(real_path)
.expect("failed to resolve RUSTPYTHONPATH during build time");
println!(
"cargo:rustc-env=win_lib_path={}",
canonicalized_path.to_str().unwrap()
);
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ fn shell_exec(
{
let loc = raw_location.start().to_usize();
let mut iter = source.chars();
if let Some(quote) = iter.nth(loc) {
if iter.next() == Some(quote) && iter.next() == Some(quote) {
return ShellExecResult::ContinueLine;
}
if let Some(quote) = iter.nth(loc)
&& iter.next() == Some(quote)
&& iter.next() == Some(quote)
{
return ShellExecResult::ContinueLine;
}
};

Expand Down
11 changes: 5 additions & 6 deletions stdlib/src/contextvars.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,11 @@ mod _contextvars {
let ctx = ctxs.last()?;
let cached_ptr = zelf.cached.as_ptr();
debug_assert!(!cached_ptr.is_null());
if let Some(cached) = unsafe { &*cached_ptr } {
if zelf.cached_id.load(Ordering::SeqCst) == ctx.get_id()
&& cached.idx + 1 == ctxs.len()
{
return Some(cached.object.clone());
}
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());
Comment on lines +381 to +385
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

}
let vars = ctx.borrow_vars();
let obj = vars.get(zelf)?;
Expand Down
20 changes: 10 additions & 10 deletions stdlib/src/math.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,23 +518,23 @@ mod math {
#[pyfunction]
fn ceil(x: PyObjectRef, vm: &VirtualMachine) -> PyResult {
let result_or_err = try_magic_method(identifier!(vm, __ceil__), vm, &x);
if result_or_err.is_err() {
if let Some(v) = x.try_float_opt(vm) {
let v = try_f64_to_bigint(v?.to_f64().ceil(), vm)?;
return Ok(vm.ctx.new_int(v).into());
}
if result_or_err.is_err()
&& let Some(v) = x.try_float_opt(vm)
{
let v = try_f64_to_bigint(v?.to_f64().ceil(), vm)?;
return Ok(vm.ctx.new_int(v).into());
}
result_or_err
}

#[pyfunction]
fn floor(x: PyObjectRef, vm: &VirtualMachine) -> PyResult {
let result_or_err = try_magic_method(identifier!(vm, __floor__), vm, &x);
if result_or_err.is_err() {
if let Some(v) = x.try_float_opt(vm) {
let v = try_f64_to_bigint(v?.to_f64().floor(), vm)?;
return Ok(vm.ctx.new_int(v).into());
}
if result_or_err.is_err()
&& let Some(v) = x.try_float_opt(vm)
{
let v = try_f64_to_bigint(v?.to_f64().floor(), vm)?;
return Ok(vm.ctx.new_int(v).into());
}
result_or_err
}
Expand Down
27 changes: 13 additions & 14 deletions stdlib/src/scproxy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,22 @@ mod _scproxy {
.and_then(|v| v.downcast::<CFNumber>())
.and_then(|v| v.to_i32())
.unwrap_or(0);
if enabled {
if let Some(host) = proxy_dict
if enabled
&& let Some(host) = proxy_dict
.find(host_key)
.and_then(|v| v.downcast::<CFString>())
{
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)?;
Comment on lines +94 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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).

}
Ok(())
};
Expand Down
Loading
Loading