Linting: Disallow unwraps, replace with expect or error propagation

[tnull]

For the longest time we wanted to enforce clear rules around unwrap use. Here we finally do that:

1. We switch all Mutex/RwLock locking to Ext helpers that avoid the unwrap and are shorter, reducing clutter in code
2. We switch over any other unwraps to use expects, providing clear rationale why they are deemed unreachable
3. For some cases we switch to error propagation. While they are not reachable, it seems simpler/cleaner that way.
4. We ban the introduction of new unwraps through clippy in CI (in non-test/documentation code, that is)

[ldk-reviews-bot]

👋 Thanks for assigning @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.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! 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.

[ldk-reviews-bot]

🔔 2nd Reminder

Hey @joostjager! 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.

[tnull]

Rebased to address minor conflict.

[joostjager]

Small comments only.

[joostjager]

I don't like this much, renaming methods on data types that are very famliar.

[tnull]

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.

[joostjager]

Would it be possible to do add-on a lock-specific expect method, as a middle road?

Otherwise it might be good to ask a second reviewer for their opinion. This is quite a visible thing.

[tnull]

Would it be possible to do add-on a lock-specific expect method, as a middle road?

I'm not quite sure I understand what you mean by that? Anything different than expect("lock") I mentioned above?

[joostjager]

I was thinking something like

 trait LockResultExt<T> {
      fn expect_not_poisoned(self) -> T;
  }

  impl<T> LockResultExt<MutexGuard<'_, T>> for LockResult<MutexGuard<'_, T>> {
      fn expect_not_poisoned(self) -> MutexGuard<'_, T> {
          self.expect("mutex poisoned")
      }
  }

  impl<T> LockResultExt<RwLockReadGuard<'_, T>> for LockResult<RwLockReadGuard<'_, T>> {
      fn expect_not_poisoned(self) -> RwLockReadGuard<'_, T> {
          self.expect("rwlock poisoned")
      }
  }

  impl<T> LockResultExt<RwLockWriteGuard<'_, T>> for LockResult<RwLockWriteGuard<'_, T>> {
      fn expect_not_poisoned(self) -> RwLockWriteGuard<'_, T> {
          self.expect("rwlock poisoned")
      }
  }

Usage: self.wallet_polling_status.lock().expect_not_poisoned()

[tnull]

Decidedly didn't go this way to keep it as brief as possible. Anyways, now switched to use expect for locks also.

[joostjager]

Don't mind the duplication of the panic string really, only wanted to suggest something more in the direction of what you had initially.

[tnull]

Addressed feedback so far.

[tnull]

Now switched away from the custom lock extensions and to just using expect("lock") for locks. Also rebased on main to incorporate latest changes.

[joostjager]

I don't think the test code is passed through the linter. It doesn't hurt, but also doesn't add much here.