Unchecked code fixes#7786
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR consolidates SSL/TLS feature configuration by gating the ChangesSSL/TLS Feature Gating & Module Configuration
OpenSSL FFI Portability to
SSL Support Code Cleanup & Warnings
VM Internal Visibility & Instrumentation Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
| ) | ||
| } | ||
|
|
||
| #[allow(dead_code, reason = "This seems like a false positive")] |
There was a problem hiding this comment.
Perhaps more reasonable here (and elsewhere in this file) would be to only let this be compiled if it is used:
#[cfg(...)]There was a problem hiding this comment.
I'm passing --all-targets --all-features, I haven't looked to much into it 🤷
Co-authored-by: fanninpm <27117322+fanninpm@users.noreply.github.com>
Co-authored-by: fanninpm <27117322+fanninpm@users.noreply.github.com>
Co-authored-by: fanninpm <27117322+fanninpm@users.noreply.github.com>
Co-authored-by: fanninpm <27117322+fanninpm@users.noreply.github.com>
|
The reason why I ask "Does this work?" is that |
I'm aware, let's see if that works:) |
It doesn't work. |
Right, that's because it isn't reached without passing |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/stdlib/src/tkinter.rs (1)
83-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
#[expect(dead_code)]here is brittle and can fail CI when unfulfilledGiven the prior unfulfilled-expectation failure on this PR, annotating the whole
TkAppitem with#[expect(dead_code)]is risky across feature/cfg combinations. Prefer#[allow(dead_code)]for this placeholder phase, or move lint handling to the specific unused fields/methods once identified.In Rust, when does `#[expect(dead_code)]` produce an "unfulfilled lint expectation" warning/error, especially under cfg/feature-gated code? Is `#[allow(dead_code)]` recommended for temporarily-unused structs during staged implementation?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/tkinter.rs` at line 83, The #[expect(dead_code, reason = "TODO: Impl more methods")] on the TkApp item is brittle and can cause "unfulfilled lint expectation" failures across cfg/feature permutations; replace it with #[allow(dead_code)] on the TkApp declaration (or, even better, move #[allow(dead_code)] to the specific unused fields/methods inside the TkApp impl/struct) so the placeholder doesn't trigger CI failures—look for the TkApp struct/item and change the attribute accordingly.
🧹 Nitpick comments (1)
crates/stdlib/src/ssl/error.rs (1)
128-138: ⚡ Quick winScope dead-code suppression to clippy-only builds.
These unconditional
allow(dead_code)attributes can hide real dead-code drift outside clippy runs. Since this PR is clippy-focused, prefercfg_attr(clippy, ...)here.♻️ Suggested change
- #[allow(dead_code, reason = "This seems like a false positive")] + #[cfg_attr(clippy, allow(dead_code, reason = "False positive under clippy feature matrix"))] pub(crate) fn create_ssl_zero_return_error(vm: &VirtualMachine) -> PyRef<PyOSError> { @@ - #[allow(dead_code, reason = "This seems like a false positive")] + #[cfg_attr(clippy, allow(dead_code, reason = "False positive under clippy feature matrix"))] pub(crate) fn create_ssl_syscall_error(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/stdlib/src/ssl/error.rs` around lines 128 - 138, The unconditional #[allow(dead_code, reason = "...")] on functions like create_ssl_zero_return_error and create_ssl_syscall_error should be scoped to clippy runs; replace those attributes with cfg_attr(clippy, allow(dead_code, reason = "...")) so the dead-code allowance only applies when clippy is enabled, keeping normal builds able to surface true dead code. Ensure you update both function attributes to use cfg_attr(clippy, ...) and preserve the existing reason text.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/builtins/function/jit.rs`:
- Line 14: The visibility mismatch is that get_jit_args is pub(crate) but
ArgsError is declared pub(super); fix by promoting ArgsError to pub(crate) so
its visibility matches all public signatures that return it (update the enum
declaration for ArgsError to pub(crate) and ensure related constructors like
new_jit_error and callers get_jit_arg_types and jit_ret_type continue to
compile); alternatively, if you prefer to restrict the API, change get_jit_args
(and any other pub(crate) functions returning ArgsError) to pub(super) so
signatures and ArgsError visibility align—choose one consistent visibility and
apply it to ArgsError and the affected functions.
---
Duplicate comments:
In `@crates/stdlib/src/tkinter.rs`:
- Line 83: The #[expect(dead_code, reason = "TODO: Impl more methods")] on the
TkApp item is brittle and can cause "unfulfilled lint expectation" failures
across cfg/feature permutations; replace it with #[allow(dead_code)] on the
TkApp declaration (or, even better, move #[allow(dead_code)] to the specific
unused fields/methods inside the TkApp impl/struct) so the placeholder doesn't
trigger CI failures—look for the TkApp struct/item and change the attribute
accordingly.
---
Nitpick comments:
In `@crates/stdlib/src/ssl/error.rs`:
- Around line 128-138: The unconditional #[allow(dead_code, reason = "...")] on
functions like create_ssl_zero_return_error and create_ssl_syscall_error should
be scoped to clippy runs; replace those attributes with cfg_attr(clippy,
allow(dead_code, reason = "...")) so the dead-code allowance only applies when
clippy is enabled, keeping normal builds able to surface true dead code. Ensure
you update both function attributes to use cfg_attr(clippy, ...) and preserve
the existing reason text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f624171e-21d5-4b0f-8ab0-da1a7efb247e
📒 Files selected for processing (10)
crates/stdlib/src/lib.rscrates/stdlib/src/openssl.rscrates/stdlib/src/openssl/cert.rscrates/stdlib/src/ssl/compat.rscrates/stdlib/src/ssl/error.rscrates/stdlib/src/tkinter.rscrates/vm/src/builtins/function/jit.rscrates/vm/src/frame.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/_thread.rs
|
|
||
| #[derive(Debug, thiserror::Error)] | ||
| pub enum ArgsError { | ||
| pub(super) enum ArgsError { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usages of the narrowed-visibility symbols outside their defining module.
echo "=== get_jit_args callers ==="
rg -n --type=rust 'get_jit_args' -g '!crates/vm/src/builtins/function/**'
echo "=== new_jit_error callers ==="
rg -n --type=rust 'new_jit_error' -g '!crates/vm/src/builtins/function/**'
echo "=== get_jit_arg_types callers ==="
rg -n --type=rust 'get_jit_arg_types' -g '!crates/vm/src/builtins/function/**'
echo "=== jit_ret_type callers ==="
rg -n --type=rust 'jit_ret_type' -g '!crates/vm/src/builtins/function/**'
echo "=== ArgsError usages outside module ==="
rg -n --type=rust 'ArgsError' -g '!crates/vm/src/builtins/function/**'Repository: RustPython/RustPython
Length of output: 643
🏁 Script executed:
# Check the actual visibility declarations in jit.rs
head -20 crates/vm/src/builtins/function/jit.rs | cat -nRepository: RustPython/RustPython
Length of output: 879
🏁 Script executed:
# Confirm visibility of get_jit_args and ArgsError around the relevant lines
sed -n '10,20p' crates/vm/src/builtins/function/jit.rs | cat -n
sed -n '155,170p' crates/vm/src/builtins/function/jit.rs | cat -nRepository: RustPython/RustPython
Length of output: 1340
🏁 Script executed:
# Check if there's a #[cfg(feature = "jit")] gate on these symbols
rg -B2 'pub\(super\) enum ArgsError|pub\(crate\) fn get_jit_args' crates/vm/src/builtins/function/jit.rs -A1Repository: RustPython/RustPython
Length of output: 337
🏁 Script executed:
# Check context around the callers to understand scope
sed -n '535,550p' crates/vm/src/builtins/function.rs | cat -n
sed -n '1060,1075p' crates/vm/src/builtins/function.rs | cat -nRepository: RustPython/RustPython
Length of output: 1543
🏁 Script executed:
# Verify module structure - is function.rs the parent of jit.rs?
head -50 crates/vm/src/builtins/function.rs | grep -E "mod jit|pub mod jit" | cat -nRepository: RustPython/RustPython
Length of output: 81
🏁 Script executed:
# Check if this code compiles - look for any existing Rust compiler lint/error reports
rg -l "private_in_public|ArgsError" crates/vm/ --type=rust | head -5Repository: RustPython/RustPython
Length of output: 104
pub(super) ArgsError is more restricted than pub(crate) get_jit_args — violates type visibility rules
get_jit_args (line 160) is pub(crate) but returns Result<Args<'a>, ArgsError> where ArgsError is pub(super) — a return type that is less visible than the function itself. This violates Rust's type visibility invariant: all types in a public function's signature must be at least as visible as the function.
All callers of get_jit_args, new_jit_error, get_jit_arg_types, and jit_ret_type are within the parent function module (lines 541, 1065, 1066, 1069 in function.rs), so pub(super) visibility is sufficient for actual usage. Either promote ArgsError to pub(crate) to match the function's visibility, or demote get_jit_args to pub(super) since it is only called from the parent module.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/builtins/function/jit.rs` at line 14, The visibility mismatch
is that get_jit_args is pub(crate) but ArgsError is declared pub(super); fix by
promoting ArgsError to pub(crate) so its visibility matches all public
signatures that return it (update the enum declaration for ArgsError to
pub(crate) and ensure related constructors like new_jit_error and callers
get_jit_arg_types and jit_ret_type continue to compile); alternatively, if you
prefer to restrict the API, change get_jit_args (and any other pub(crate)
functions returning ArgsError) to pub(super) so signatures and ArgsError
visibility align—choose one consistent visibility and apply it to ArgsError and
the affected functions.
While trying to run:
clippy --all-features --workspaceI've seen lots of errors. This PR goal is to break the fixes needed for it to run on the CI, into smaller chunksSummary by CodeRabbit