Linting: Disallow unwraps, replace with expect or error propagation#868
Linting: Disallow unwraps, replace with expect or error propagation#868tnull wants to merge 11 commits intolightningdevkit:mainfrom
unwraps, replace with expect or error propagation#868Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
tnull
force-pushed
the
2026-04-disallow-unwrap
branch
from
a05b2b6 to
2eb9f09
Compare
|
🔔 1st Reminder Hey @joostjager! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @joostjager! This PR has been waiting for your review. |
Introduce a small lock helper module so mutex and rwlock access can use a single naming convention instead of repeating direct locking at every call site. Co-Authored-By: HAL 9000
Switch the codebase over to the new lock helper names so lock poisoning behavior is centralized and the call sites stay shorter and more consistent. Co-Authored-By: HAL 9000
Document the internal invariants behind the remaining non-lock unwrap sites so panic paths explain why they should be unreachable, while keeping the known reachable cases explicit for later handling. Co-Authored-By: HAL 9000
Log and skip runtime listener failures instead of panicking when accepting inbound connections or converting accepted sockets. These errors can happen in normal operation, so keeping the node running is safer than treating them as unreachable. Co-Authored-By: HAL 9000
Replace the success-path unwrap on payment amounts with an expect that explains why outbound payments must already have a recorded amount by the time LDK reports them as sent. Co-Authored-By: HAL 9000
Replace the pending-channel unwrap with an expect that records why supported LDK Node state should always include the former temporary channel id. Older rust-lightning state could omit it, but LDK Node never shipped before that field existed. Co-Authored-By: HAL 9000 Signed-off-by: Elias Rohrer <dev@tnull.de>
Replace the user_version query unwrap with normal io::Error propagation so database initialization failures are reported cleanly instead of panicking. Co-Authored-By: HAL 9000
Replace the Tokio runtime builder unwrap with io::Error propagation so VSS startup failures surface through the constructor instead of panicking. Co-Authored-By: HAL 9000
Use a zero-millisecond fallback for elapsed-time logging so clock adjustments do not panic the chain polling loop. Co-Authored-By: HAL 9000
Return Esplora client construction failures through build-time error handling instead of panicking so invalid headers or reqwest setup errors fail node construction cleanly. Co-Authored-By: HAL 9000
Fail library clippy runs when new unwrap calls are introduced so the unwrap policy stays enforced without pulling tests, benches, or docs into the restriction. Co-Authored-By: HAL 9000
tnull
force-pushed
the
2026-04-disallow-unwrap
branch
from
2eb9f09 to
374b732
Compare
|
Rebased to address minor conflict. |
| // wait on the result before proceeding. | ||
| { | ||
| let mut status_lock = self.wallet_polling_status.lock().unwrap(); | ||
| let mut status_lock = self.wallet_polling_status.lck(); |
There was a problem hiding this comment.
I don't like this much, renaming methods on data types that are very famliar.
There was a problem hiding this comment.
Yeah, I usually follow the same train of thought, however, in this case it also leads us to the anti-pattern of sprinkling unwrap everywhere. We could replace them with except("lock") or something like this, but this would make it even more verbose everywhere. Hence why I opted to go this way, which even reduces clutter everywhere.
Let me know if you deem this a blocker though, happy to revert that commit and go the cluttered route if you prefer.
| self.keys_manager.get_secure_random_bytes()[..16].try_into().unwrap(), | ||
| self.keys_manager.get_secure_random_bytes()[..16] | ||
| .try_into() | ||
| .expect("a 16-byte slice should convert into a [u8; 16]"), |
There was a problem hiding this comment.
I think this belongs in a later commit?
| feerate_sat_per_1000_weight: value.feerate_sat_per_1000_weight.unwrap(), | ||
| feerate_sat_per_1000_weight: value | ||
| .feerate_sat_per_1000_weight | ||
| .expect("LDK objects that are serialized on v0.0.115+ (as ldk-node v0.1 already depended on v0.0.115)"), |
There was a problem hiding this comment.
phrasing similar to the below might be clearer
"should always be present ... etc"
There was a problem hiding this comment.
Hmm, not sure, for except it's usually preferable to keep it very brief and only state the expectation.
| - name: Ban unwrap in library code on Rust ${{ matrix.toolchain }} | ||
| if: matrix.build-uniffi | ||
| env: | ||
| RUSTFLAGS: "" |
There was a problem hiding this comment.
Comment needed why this is cleared?
| RUSTFLAGS: "" | ||
| run: | | ||
| rustup component add clippy | ||
| cargo clippy --lib --verbose --color always -- -A warnings -D clippy::unwrap_used -A clippy::tabs_in_doc_comments |
There was a problem hiding this comment.
It was unclear to me why this runs under if: matrix.build-uniffi? Perhaps it can be a separate step
3 participants
For the longest time we wanted to enforce clear rules around
unwrapuse. Here we finally do that:Mutex/RwLocklocking toExthelpers that avoid theunwrapand are shorter, reducing clutter in codeunwraps to useexpects, providing clear rationale why they are deemed unreachableunwraps throughclippyin CI (in non-test/documentation code, that is)