Skip to content

Conversation

dunxen
Copy link
Contributor

@dunxen dunxen commented Aug 8, 2025

Hiding a lifetime that's elided elsewhere is confusing.

See: rust-lang/rust#138677

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Aug 8, 2025

I've assigned @joostjager 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.

Copy link

codecov bot commented Aug 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.92%. Comparing base (381416a) to head (b104f3a).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3997      +/-   ##
==========================================
- Coverage   88.94%   88.92%   -0.02%     
==========================================
  Files         174      174              
  Lines      124575   124578       +3     
  Branches   124575   124578       +3     
==========================================
- Hits       110797   110777      -20     
- Misses      11278    11308      +30     
+ Partials     2500     2493       -7     
Flag Coverage Δ
fuzzing 22.18% <15.00%> (ø)
tests 88.74% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@TheBlueMatt
Copy link
Collaborator

Clippy now says error: hiding a lifetime that's elided elsewhere is confusing in many places. Should we fix that?

@dunxen
Copy link
Contributor Author

dunxen commented Aug 8, 2025

Clippy now says error: hiding a lifetime that's elided elsewhere is confusing in many places. Should we fix that?

Ah I didn't fix all cases

@dunxen dunxen force-pushed the 2025-08-fixmismatched-lifetime-syntaxes branch 2 times, most recently from d476838 to d139d1a Compare August 8, 2025 14:36
Hiding a lifetime that's elided elsewhere is confusing.

See: rust-lang/rust#138677
@dunxen dunxen force-pushed the 2025-08-fixmismatched-lifetime-syntaxes branch 2 times, most recently from 5a3a3c9 to 646a80e Compare August 8, 2025 15:18
@dunxen dunxen force-pushed the 2025-08-fixmismatched-lifetime-syntaxes branch from 646a80e to b104f3a Compare August 8, 2025 15:20
@dunxen
Copy link
Contributor Author

dunxen commented Aug 8, 2025

Ugh, okay that should do it.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Gonna go ahead and land this but will remove the Keys struct in a followup.

@@ -49,7 +49,8 @@ use bitcoin::hash_types::BlockHash;
use bitcoin::pow::Work;

use lightning::chain;
use lightning::chain::Listen;
#[allow(unused_imports)] // This thinks trait imports are unused if they're use in macros :(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a bug in clippy? Did we report it? In any case it might make sense to move the imports into the macros themselves.

Copy link
Contributor Author

@dunxen dunxen Aug 8, 2025

Choose a reason for hiding this comment

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

I think it is a bug honestly. I could only find an issue about a false positive related to this rule for rust-analyzer but this definitely is an issue with the rustc lint rule.

I'll dig some more and file an issue if there isn't one.

Edit: I also noticed the clippy --fix missed a bunch of instances of some rules it can actually fix. I'm not sure if it was the options/flags I used not cooperating but I'll treat it with some suspicion and possibly also file an issue.

@@ -14192,6 +14192,7 @@ mod tests {
);
}

#[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this may actually be dead code, so we should probably remove it :)

@TheBlueMatt TheBlueMatt merged commit 91a0680 into lightningdevkit:main Aug 8, 2025
24 checks passed
@TheBlueMatt
Copy link
Collaborator

Also, in what world is the new code more readable? WTF rustc...

@dunxen
Copy link
Contributor Author

dunxen commented Aug 8, 2025

Also, in what world is the new code more readable? WTF rustc...

Honestly the "useless concat" rule messed up the pretty alignment we had :(

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.

3 participants