-
Notifications
You must be signed in to change notification settings - Fork 31
chore(lint): add clippy rule to prevent unwrap usage #229
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Add `-D clippy::unwrap_used` flag to cargo clippy linting command - Enforce stricter code quality standards by disallowing unwrap calls - Improve error handling practices across the codebase
WalkthroughThe Makefile's lint target is enhanced to include an additional Clippy lint flag ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build and test (ubuntu-latest)
- GitHub Check: cargo fmt (windows-latest)
- GitHub Check: cargo clippy (windows-latest)
- GitHub Check: cargo clippy (macos-latest)
- GitHub Check: build and test (macos-latest)
- GitHub Check: cargo clippy (ubuntu-latest)
- GitHub Check: build and test (windows-latest)
- GitHub Check: test
- GitHub Check: test-windows
- GitHub Check: lint
- GitHub Check: test-macos
| lint: | ||
| @echo "Linting code..." | ||
| @cargo clippy --manifest-path ./Cargo.toml --all-features --workspace -- -D warnings | ||
| @cargo clippy --manifest-path ./Cargo.toml --all-features --workspace -- -D warnings -D clippy::unwrap_used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for .unwrap() calls in Rust source files
echo "=== Searching for .unwrap() usage ==="
find . -name "*.rs" -type f | head -20
echo ""
echo "=== Checking for unwrap() in Rust files ==="
rg '\.unwrap\(\)' --type rust --max-count=10 || echo "No unwrap() found or rg not available"
echo ""
echo "=== Checking for expect() in Rust files ==="
rg '\.expect\(' --type rust --max-count=10 || echo "No expect() found or rg not available"
echo ""
echo "=== Checking Makefile content around line 56 ==="
cat -n Makefile | sed -n '50,65p'
echo ""
echo "=== Looking for clippy configuration ==="
if [ -f "clippy.toml" ]; then
echo "Found clippy.toml:"
cat clippy.toml
else
echo "No clippy.toml found"
fi
if [ -f "Cargo.toml" ]; then
echo ""
echo "Checking Cargo.toml for lints configuration:"
grep -A 10 "\[lints" Cargo.toml || echo "No [lints] section found"
fiRepository: arana-db/kiwi
Length of output: 50369
🏁 Script executed:
#!/bin/bash
# Check for clippy configuration
echo "=== Checking for clippy.toml ==="
if [ -f "clippy.toml" ]; then
cat clippy.toml
else
echo "No clippy.toml found"
fi
echo ""
echo "=== Checking Cargo.toml for lints configuration ==="
if [ -f "Cargo.toml" ]; then
grep -A 20 "\[lints" Cargo.toml | head -30
else
echo "No Cargo.toml found"
fi
echo ""
echo "=== Checking for allow-unwrap-in-tests in Cargo.toml ==="
if [ -f "Cargo.toml" ]; then
grep "allow-unwrap-in-tests" Cargo.toml || echo "Not found"
else
echo "No Cargo.toml found"
fi
echo ""
echo "=== Count unwrap() in production src code (excluding tests) ==="
rg '\.unwrap\(\)' --type rust src/ --glob '!**/tests/**' --glob '!**/benches/**' --glob '!**/test.rs' --count-all 2>/dev/null | tail -5Repository: arana-db/kiwi
Length of output: 1475
🏁 Script executed:
#!/bin/bash
echo "=== Full Makefile around line 56 ==="
cat -n Makefile | sed -n '45,70p'
echo ""
echo "=== Check if there's a recent git commit for unwrap fixes ==="
git log --oneline -20 2>/dev/null | head -10 || echo "Git not available"
echo ""
echo "=== Verify count of production unwraps more carefully ==="
rg '\.unwrap\(\)' --type rust src/ -c 2>/dev/null | grep -v ":0$" | wc -lRepository: arana-db/kiwi
Length of output: 1159
This lint change requires either removing all 95 instances of .unwrap() from production code first, or adding allow-unwrap-in-tests configuration to Clippy.
The codebase currently contains .unwrap() calls across 95 files in production code. Adding -D clippy::unwrap_used will immediately break the build unless all .unwrap() calls are removed or properly configured to allow only test usage.
Two approaches:
- Refactor production code first: Replace all
.unwrap()calls with proper error handling before merging this lint change. - Configure for tests only: Add
allow-unwrap-in-tests = truetoclippy.toml, then refactor production code separately. This allows.unwrap()in tests while denying it in library/binary code.
Also consider adding -D clippy::expect_used for consistency, since both .unwrap() and .expect() result in panics.
🤖 Prompt for AI Agents
In Makefile at line 56, the CI lint invocation disables builds because -D
clippy::unwrap_used will fail due to 95 .unwrap() occurrences; either
remove/refactor all production .unwrap()/.expect() usages before enabling this
lint, or add a clippy.toml with allow-unwrap-in-tests = true (and consider
adding deny for expect_used as well) so tests can still use unwrap while
production code is enforced; update the Makefile/lint config accordingly to
avoid breaking the build until production unwraps are fixed.
#218
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.