chore[*]: prefer #[expect(..)] over #[allow(..)]

[joseph-isaacs]

We cannot fully remove allow

[codspeed-hq]

CodSpeed Performance Report

Merging #5498 will not alter performance

_{Comparing ji/remove-clippy-allow (0a320a2) with develop (8261ebf)}

Summary

✅ 1482 untouched
⏩ 189 skipped¹

Footnotes

1. 189 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.48%. Comparing base (c241f59) to head (0a320a2).
⚠️ Report is 5 commits behind head on develop.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[0ax1]

@claude review

[claude]

Claude finished @0ax1's task —— View job

---

Code Review: Prefer #[expect(..)] over #[allow(..)]

• Read repository guidelines (CLAUDE.md)
• Understand the #[allow] vs #[expect] distinction
• Review sample of changed files
• Analyze consistency and correctness
• Provide comprehensive review feedback

---

Summary

This PR migrates select #[allow(..)] attributes to #[expect(..)] with explanatory reason fields across 38 files. The migration is well-executed and follows Rust best practices. I recommend approval with minor suggestions below.

Background: #[allow] vs #[expect]

The #[expect] attribute (stabilized in Rust 1.81) improves upon #[allow] by:

1. Requiring documentation - The reason parameter forces developers to explain why the lint is suppressed
2. Detecting dead suppressions - If the lint is no longer triggered, #[expect] will emit a warning, helping prevent technical debt
3. Better maintainability - Future developers can understand the intent without archaeology

What This PR Does Well

✅ Selective migration: Only migrates suppressions where a clear reason can be articulated, leaving ~319 #[allow] attributes intact (mostly in tests, benches, and generated code)

✅ Clear reasoning: All migrated suppressions include meaningful reason fields:

• "complexity is from nested match_each_* macros" (compress.rs)
• "intentionally panic in debug mode when VORTEX_PANIC_ON_ERR is set" (error/lib.rs)
• "method names are self-documenting" (ptype.rs)
• "required by trait signature" (datafusion/format.rs)

✅ Appropriate scope: Maintains attribute placement at the correct level (function, impl block, etc.)

✅ Removal of obsolete suppressions: Some #[allow] attributes were simply removed (e.g., clippy::same_name_method, clippy::panic_in_result_fn) suggesting the underlying issues were resolved or no longer apply

Specific Changes Analysis

Type 1: Migration with Reasoning (Most Changes)

// Before
#[allow(clippy::cognitive_complexity)]

// After  
#[expect(
    clippy::cognitive_complexity,
    reason = "complexity is from nested match_each_* macros"
)]

Assessment: ✅ Excellent. Provides context for why complexity is acceptable.

Type 2: Removal of Obsolete Suppressions

// Before (vortex-dtype/src/arbitrary/mod.rs)
#[allow(
    clippy::unwrap_in_result,
    clippy::expect_used,
    clippy::cast_possible_truncation
)]
impl<'a> Arbitrary<'a> for DecimalDType { ... }

// After
impl<'a> Arbitrary<'a> for DecimalDType { ... }

Assessment: ✅ Good - these suppressions were removed entirely. However, I'd recommend verifying that clippy doesn't complain about this code without the suppressions.

Type 3: Updated Comments

// Before  
#[allow(missing_docs)] // Kind of obvious.

// After
#[expect(missing_docs, reason = "method names are self-documenting")]

Assessment: ✅ Better documentation intent.

Suggestions

1. Verify removed suppressions: Run cargo clippy --all-targets --all-features to ensure the removed #[allow] attributes in files like:

   • vortex-dtype/src/arbitrary/mod.rs (removed 3 suppressions)
   • vortex-array/src/arrays/primitive/array/conversion.rs (removed panic_in_result_fn)
   • vortex-array/src/arrays/struct_/array.rs (removed suppression)
   • vortex-dtype/src/arbitrary/decimal.rs (removed same_name_method)

   don't cause new clippy warnings.

2. PR description enhancement: The PR body says "We cannot fully remove allow" but doesn't explain why. Consider adding context:

   • Which patterns can't be migrated (e.g., inline test suppressions, generated code)
   • The criteria used to decide what to migrate
   • Why ~319 #[allow] remain

3. Formatting consistency: Most changes use 4-space indentation for multi-line attributes, which is consistent and readable.

Potential Issues

⚠️ Removed suppressions need verification: Several files had suppressions completely removed rather than converted. While this might be intentional cleanup, it needs validation:

• vortex-array/src/arrays/primitive/array/conversion.rs:110 - removed clippy::panic_in_result_fn but the function still calls vortex_panic!
• The arbitrary implementations that had suppressions removed

Testing

✅ CI shows:

• No performance regression
• Code coverage maintained at 85.48%
• All checks passing

Conclusion

This is a high-quality refactoring that improves code documentation and maintainability. The migration is thoughtful, selective, and well-reasoned.

Recommendation: ✅ Approve after verifying that the removed suppressions don't cause new clippy warnings.

Action Items for Author

• Run cargo clippy --all-targets --all-features and confirm no new warnings
• Consider enhancing PR description to explain migration strategy
• Verify that removed suppressions (not converted ones) were intentional

---

[0ax1]

🙆