`TrySendError::is_closed()` and `SendTimeoutError::is_closed()` test the wrong variant

[PeterKnego]

Both TrySendError::is_closed() and SendTimeoutError::is_closed() match on the other variant of their enum, so each is an exact duplicate of its sibling predicate and returns the opposite of what its name and docs promise.

src/mpsc/errors.rs (current main)

impl<T> TrySendError<T> {
    pub fn is_full(&self) -> bool {
        matches!(self, Self::Full(_))
    }

    /// Returns `true` if this error was returned because the channel has closed
    pub fn is_closed(&self) -> bool {
        matches!(self, Self::Full(_))   // <-- should be Self::Closed(_)
    }
}

#[cfg(feature = "std")]
impl<T> SendTimeoutError<T> {
    pub fn is_timeout(&self) -> bool {
        matches!(self, Self::Timeout(_))
    }

    /// Returns `true` if this error was returned because the channel has closed
    pub fn is_closed(&self) -> bool {
        matches!(self, Self::Timeout(_))   // <-- should be Self::Closed(_)
    }
}

Reproduction

use std::time::Duration;
use thingbuf::mpsc::blocking;
use thingbuf::mpsc::errors::{SendTimeoutError, TrySendError};

fn main() {
    let (tx, _rx) = blocking::channel::<u64>(1);
    tx.try_send(1).unwrap();
    let e = tx.try_send(2).unwrap_err();
    assert!(matches!(e, TrySendError::Full(_)));
    println!("Full    is_full()={:<5} is_closed()={}", e.is_full(), e.is_closed());

    let (tx, rx) = blocking::channel::<u64>(4);
    drop(rx);
    let e = tx.try_send(1).unwrap_err();
    assert!(matches!(e, TrySendError::Closed(_)));
    println!("Closed  is_full()={:<5} is_closed()={}", e.is_full(), e.is_closed());

    let (tx, _rx) = blocking::channel::<u64>(1);
    tx.try_send(1).unwrap();
    let e = tx.send_timeout(2, Duration::from_millis(20)).unwrap_err();
    assert!(matches!(e, SendTimeoutError::Timeout(_)));
    println!("Timeout is_timeout()={:<5} is_closed()={}", e.is_timeout(), e.is_closed());

    let (tx, rx) = blocking::channel::<u64>(4);
    drop(rx);
    let e = tx.send_timeout(1, Duration::from_millis(20)).unwrap_err();
    assert!(matches!(e, SendTimeoutError::Closed(_)));
    println!("Closed  is_timeout()={:<5} is_closed()={}", e.is_timeout(), e.is_closed());
}

Output on thingbuf 0.1.6:

Full    is_full()=true  is_closed()=true      <- want false
Closed  is_full()=false is_closed()=false     <- want true
Timeout is_timeout()=true  is_closed()=true   <- want false
Closed  is_timeout()=false is_closed()=false  <- want true

So is_closed() is true exactly when the channel is not closed, and false when it is.

Why it matters

The natural retry loop silently becomes an infinite loop, because the condition that is supposed to break it never becomes true:

loop {
    match tx.try_send(v) {
        Ok(()) => break,
        Err(e) if e.is_closed() => return,  // never taken on a closed channel
        Err(e) => { v = e.into_inner(); std::hint::spin_loop(); }
    }
}

Matching on the variants directly is unaffected, which is probably why this has gone unnoticed.

Fix

pub fn is_closed(&self) -> bool {
    matches!(self, Self::Closed(_))
}

in both impls. Happy to open a PR if that is useful.

Found while benchmarking thingbuf as a comparison point for a bounded-ring crate of my own; version tested is 0.1.6, and the code is identical on main as of this writing.