Skip to content

[RFC] clippy new warnings in rustc 1.86.0 (05f9846f8 2025-03-31) #3710

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

Merged
merged 3 commits into from
Apr 8, 2025

Conversation

vincenzopalazzo
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo commented Apr 5, 2025

I was using an older version of the compiler, and Clippy did not report any issues. However, after updating to Rust 1.86.0, I was able to reproduce the warnings that we are having inside the CI. This patch proposes an opinionated solution that aligns with Clippy’s suggestions, but I am open to disabling some of the warnings if needed.

The commits include specific changes to highlight the errors, making the review process easier.

Probably @TheBlueMatt will have some opinion on this!

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 5, 2025

👋 Thanks for assigning @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
```
error: manual implementation of `ok`
    --> lightning/src/ln/channel.rs:8551:3
     |
8551 | /         match self.sign_channel_announcement(node_signer, announcement) {
8552 | |             Ok(res) => Some(res),
8553 | |             Err(_) => None,
8554 | |         }
     | |_________^ help: replace with: `self.sign_channel_announcement(node_signer, announcement).ok()`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err
     = note: `-D clippy::manual-ok-err` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(clippy::manual_ok_err)]`

```

Signed-off-by: Vincenzo Palazzo <[email protected]>
Copy link

codecov bot commented Apr 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.04%. Comparing base (c4d23bc) to head (abadb79).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3710      +/-   ##
==========================================
- Coverage   89.04%   89.04%   -0.01%     
==========================================
  Files         155      155              
  Lines      122019   122017       -2     
  Branches   122019   122017       -2     
==========================================
- Hits       108652   108647       -5     
- Misses      10701    10710       +9     
+ Partials     2666     2660       -6     

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

@dunxen
Copy link
Contributor

dunxen commented Apr 6, 2025

The changes seem minimal and the formatting is being properly corrected, because it's aligning further lines with the leading backtick, unlike before where those instance were aligned with the first letter.

So this LGTM

Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Hmm, I'm a bit confused why we need to touch features.rs once more, wasn't this solved in #3704 ?

@dunxen
Copy link
Contributor

dunxen commented Apr 7, 2025

Hmm, I'm a bit confused why we need to touch features.rs once more, wasn't this solved in #3704 ?

Yeah, those changes are correct. I think this might have been applied to main (after 3704) here according to suggestions on a CI run (before 3704).

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @valentinewallace! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

LGTM after @dunxen's feedback

@vincenzopalazzo vincenzopalazzo force-pushed the macros/docs-fix branch 2 times, most recently from dcbd7f2 to 80a657e Compare April 7, 2025 20:23

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
with recent rustc version we can use `repeat_n` instead of `repeat().take()`

```
error: this `repeat().take()` can be written more concisely
   --> lightning-invoice/src/ser.rs:256:12
    |
256 |         Box::new(core::iter::repeat(Fe32::Q).take(to_pad).chain(fes))
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `repeat_n()` instead: `core::iter::repeat_n(Fe32::Q, to_pad)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_repeat_n
    = note: `-D clippy::manual-repeat-n` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::manual_repeat_n)]`
```

but to keep compatibility with older rustc versions we need to disable the warning
for the `repeat_n` lint.

Signed-off-by: Vincenzo Palazzo <[email protected]>

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
This is the most opinonated suggestion in this sequence of patches.
Probably IDK if would be an option disable this check, but for now
this commit propose a formatting fix to make clippy happy.

```
error: doc list item overindented
  --> lightning-types/src/features.rs:62:5
   |
62 | //!    onion.
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items
   = note: `-D clippy::doc-overindented-list-items` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::doc_overindented_list_items)]`

error: doc list item overindented
  --> lightning-types/src/features.rs:63:5
   |
63 | //!    (see [BOLT-11](https://github.com/lightning/bolts/blob/master/11-payment-encoding.md) for
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items

error: doc list item overindented
  --> lightning-types/src/features.rs:64:5
   |
64 | //!    more).
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items

error: doc list item overindented
  --> lightning-types/src/features.rs:66:5
   |
66 | //!    (see
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items

error: doc list item overindented
  --> lightning-types/src/features.rs:67:5
   |
67 | //!    [BOLT-2](https://github.com/lightning/bolts/blob/master/02-peer-protocol.md#the-channel_ready-message)
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items

error: doc list item overindented
  --> lightning-types/src/features.rs:68:5
   |
68 | //!    for more info).
   |     ^^^ help: try using `  ` (2 spaces)
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_overindented_list_items

error: could not compile `lightning-types` (lib) due to 6 previous errors
```

Suggested-by: @dunxen
Signed-off-by: Vincenzo Palazzo <[email protected]>
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, going ahead and landing this as generally trivial and the diff to previous ACKs is minimal.

@tnull tnull merged commit 06fb142 into lightningdevkit:main Apr 8, 2025
26 of 27 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/docs-fix branch April 8, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants