Skip to content

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

Merged
0ax1 merged 2 commits intodevelopfrom
ji/remove-clippy-allow
Nov 25, 2025
Merged

chore[*]: prefer #[expect(..)] over #[allow(..)]#5498
0ax1 merged 2 commits intodevelopfrom
ji/remove-clippy-allow

Conversation

@joseph-isaacs
Copy link
Contributor

@joseph-isaacs joseph-isaacs commented Nov 24, 2025

We cannot fully remove allow

@joseph-isaacs joseph-isaacs changed the title wip chore[*]: prefer #[expect(..)] over #[allow(..)] Nov 24, 2025
@joseph-isaacs joseph-isaacs added the changelog/chore A trivial change label Nov 24, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 24, 2025

CodSpeed Performance Report

Merging #5498 will not alter performance

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

Summary

✅ 1482 untouched
⏩ 189 skipped1

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
Copy link

codecov bot commented Nov 24, 2025

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.

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
@joseph-isaacs joseph-isaacs marked this pull request as ready for review November 25, 2025 15:13
@0ax1 0ax1 self-requested a review November 25, 2025 15:16
@0ax1
Copy link
Contributor

0ax1 commented Nov 25, 2025

@claude review

@claude
Copy link
Contributor

claude bot commented Nov 25, 2025

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

Copy link
Contributor

@0ax1 0ax1 left a comment

Choose a reason for hiding this comment

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

🙆

@0ax1 0ax1 merged commit e9427b9 into develop Nov 25, 2025
47 checks passed
@0ax1 0ax1 deleted the ji/remove-clippy-allow branch November 25, 2025 15:24
a10y pushed a commit that referenced this pull request Nov 25, 2025
We cannot fully remove allow

---------

Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/chore A trivial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants