Idea: Early returns and irrefutability

drXor · 2019-04-01T17:19:55.957Z

Ah, excellent. I was somewhat surprised that this hadn’t otherwise been thought of.

Yato · 2019-04-01T17:49:40.939Z

Also, you can do this right now

let x = match x {
    Some(inside) => inside,
    None => return
}

H2CO3 · 2019-04-01T20:57:18.043Z

Because match is an expression (see @Yato 's answer), and Rust’s control flow analysis is sound and strongly connected with the type system, so using a common, simple, and teachable pattern, this problem is already almost trivially solvable.

drXor · 2019-04-02T14:11:08.494Z

Hmm, fair enough. I feel like rust has a serious “rightward drift” problem, though, which I care a lot about solving. I think this requires more thought than “dragging C++ patterns over”, though.

rkruppe · 2019-04-02T14:39:19.001Z

One doesn’t need to introduce flow typing or other static analyses at all to get rid of the rightward drift. As previously proposed one could just add new surface syntax that has a different nesting structure. The linked RFC was postponed in 2016, but I feel it’s been long enough to revisit it, and any case I doubt whether a far more invasive language change such as flow typing would fare any better.

CAD97 · 2019-04-02T15:06:34.826Z

I’ve written a decent deal on this topic before, both in a blog post pretending to be a rust2018 post and here on irlo.

I do like refinement typing / flow typing / smart casts in Kotlin, but in Kotlin it only happens for a) non-nullable from nullable and b) subtype from supertype.

But I honestly think that “guard let”/“refutable let” is the way to go for Rust, making a semantic version of let pat = match expr { pat => pat, _ => ! }.

The problem is that it requires introducing a new syntax to do something that is already very possible (using the “match” desugar). It’s a question of how much gain we get for the cost.

And I don’t know whether it’s really worth it. The match version is strictly more powerful, and the _ arm is already required to be appropriately typed, which includes diverging/breaking.

Where it starts becoming a clear win is when you are binding more than one thing in the refutable let. But how much does that actually happen, where you actually have a deep pattern and extract multiple values or bail?

I think it’s worth it to “canonize” the “guard” pattern with some dedicated syntax to encourage people to use that syntax instead of introducing rightward drift. But proving that to people and the several valid arguments against is no simple task.

Not to mention the bikeshed potential which tends to drown out meaningful talk a lot of the time

djc · 2019-04-03T08:19:23.494Z

This is definitely a pain point for me. Here’s some examples from Quinn:

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L441


        Ok(false) => {}
        Ok(true) => {
            self.congestion_event(now, largest_sent_time);
        }
    }
}


// Not timing-aware, so it's safe to call this for inferred acks, such as arise from
// high-latency handshakes
fn on_packet_acked(&mut self, space: SpaceId, packet: u64) {
    let info = if let Some(x) = self.space_mut(space).sent_packets.remove(&packet) {
        x
    } else {
        return;
    };
    self.in_flight.remove(&info);
    if info.ack_eliciting {
        // Congestion control
        // Do not increase congestion window in recovery period.
        if !self.in_recovery(info.time_sent) {
            if self.congestion_window < self.ssthresh {

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L475


        self.streams.get_send_mut(id).unwrap().state
    {
        self.streams.get_send_mut(id).unwrap().state =
            stream::SendState::ResetRecvd { stop_reason };
        if stop_reason.is_none() {
            self.streams.maybe_cleanup(id);
        }
    }
}
for frame in info.retransmits.stream {
    let ss = if let Some(x) = self.streams.get_send_mut(frame.id) {
        x
    } else {
        continue;
    };
    ss.bytes_in_flight -= frame.data.len() as u64;
    self.unacked_data -= frame.data.len() as u64;
    if ss.state == stream::SendState::DataSent && ss.bytes_in_flight == 0 {
        ss.state = stream::SendState::DataRecvd;
        self.streams.maybe_cleanup(frame.id);
        self.events

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L736


) {
    self.remote_validated |= self.state.is_handshake() && space_id == SpaceId::Handshake;
    self.reset_keep_alive(now);
    self.reset_idle_timeout(now);
    self.permit_idle_reset = true;
    self.receiving_ecn |= ecn.is_some();
    if let Some(x) = ecn {
        self.ecn_counters += x;
    }


    let packet = if let Some(x) = packet {
        x
    } else {
        return;
    };
    trace!(
        self.log,
        "{space:?} packet {packet} authenticated",
        space = space_id,
        packet = packet
    );

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L819


///
/// # Panics
/// - when applied to a receive stream or an unopened send stream
pub fn reset(&mut self, stream_id: StreamId, error_code: u16) {
    assert!(
        stream_id.directionality() == Directionality::Bi || stream_id.initiator() == self.side,
        "only streams supporting outgoing data may be reset"
    );


    // reset is a noop on a closed stream
    let stream = if let Some(x) = self.streams.get_send_mut(stream_id) {
        x
    } else {
        return;
    };
    match stream.state {
        stream::SendState::DataRecvd
        | stream::SendState::ResetSent { .. }
        | stream::SendState::ResetRecvd { .. } => {
            return;
        } // Nothing to do

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L1949


            "PATH_RESPONSE {token:08x}",
            token = response.token
        );
        buf.write(frame::Type::PATH_RESPONSE);
        buf.write(response.token);
    }
}


// CRYPTO
while buf.len() + frame::Crypto::SIZE_BOUND < max_size && !is_0rtt {
    let mut frame = if let Some(x) = space.pending.crypto.pop_front() {
        x
    } else {
        break;
    };
    let len = cmp::min(
        frame.data.len(),
        max_size as usize - buf.len() - frame::Crypto::SIZE_BOUND,
    );
    let data = frame.data.split_to(len);
    let truncated = frame::Crypto {

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L1979


    truncated.encode(buf);
    sent.crypto.push_back(truncated);
    if !frame.data.is_empty() {
        frame.offset += len as u64;
        space.pending.crypto.push_front(frame);
    }
}


// RESET_STREAM
while buf.len() + frame::ResetStream::SIZE_BOUND < max_size && space_id == SpaceId::Data {
    let (id, error_code) = if let Some(x) = space.pending.rst_stream.pop() {
        x
    } else {
        break;
    };
    let stream = if let Some(x) = self.streams.get_send_mut(id) {
        x
    } else {
        continue;
    };
    trace!(self.log, "RESET_STREAM"; "stream" => id.0);

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L1984


    }
}


// RESET_STREAM
while buf.len() + frame::ResetStream::SIZE_BOUND < max_size && space_id == SpaceId::Data {
    let (id, error_code) = if let Some(x) = space.pending.rst_stream.pop() {
        x
    } else {
        break;
    };
    let stream = if let Some(x) = self.streams.get_send_mut(id) {
        x
    } else {
        continue;
    };
    trace!(self.log, "RESET_STREAM"; "stream" => id.0);
    sent.rst_stream.push((id, error_code));
    frame::ResetStream {
        id,
        error_code,
        final_offset: stream.offset,

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2001


    frame::ResetStream {
        id,
        error_code,
        final_offset: stream.offset,
    }
    .encode(buf);
}


// STOP_SENDING
while buf.len() + 11 < max_size && space_id == SpaceId::Data {
    let (id, error_code) = if let Some(x) = space.pending.stop_sending.pop() {
        x
    } else {
        break;
    };
    let stream = if let Some(x) = self.streams.get_recv_mut(id) {
        x
    } else {
        continue;
    };
    if stream.is_finished() {

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2006


    .encode(buf);
}


// STOP_SENDING
while buf.len() + 11 < max_size && space_id == SpaceId::Data {
    let (id, error_code) = if let Some(x) = space.pending.stop_sending.pop() {
        x
    } else {
        break;
    };
    let stream = if let Some(x) = self.streams.get_recv_mut(id) {
        x
    } else {
        continue;
    };
    if stream.is_finished() {
        continue;
    }
    trace!(self.log, "STOP_SENDING"; "stream" => id.0);
    sent.stop_sending.push((id, error_code));
    buf.write(frame::Type::STOP_SENDING);

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2038


}


// MAX_STREAM_DATA
while buf.len() + 17 < max_size {
    let id = if let Some(x) = space.pending.max_stream_data.iter().next() {
        *x
    } else {
        break;
    };
    space.pending.max_stream_data.remove(&id);
    let rs = if let Some(x) = self.streams.get_recv_mut(id) {
        x
    } else {
        continue;
    };
    if rs.is_finished() {
        continue;
    }
    sent.max_stream_data.insert(id);
    let max = rs.bytes_read + self.config.stream_receive_window;
    trace!(

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2079


if space.pending.max_bi_stream_id && buf.len() + 9 < max_size {
    space.pending.max_bi_stream_id = false;
    sent.max_bi_stream_id = true;
    trace!(self.log, "MAX_STREAMS (bidirectional)"; "value" => self.streams.max_remote_bi - 1);
    buf.write(frame::Type::MAX_STREAMS_BIDI);
    buf.write_var(self.streams.max_remote_bi);
}


// NEW_CONNECTION_ID
while buf.len() + 44 < max_size {
    let frame = if let Some(x) = space.pending.new_cids.pop() {
        x
    } else {
        break;
    };
    trace!(
        self.log,
        "NEW_CONNECTION_ID {sequence} = {id}",
        sequence = frame.sequence,
        id = frame.id,
    );

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2096


        "NEW_CONNECTION_ID {sequence} = {id}",
        sequence = frame.sequence,
        id = frame.id,
    );
    frame.encode(buf);
    sent.new_cids.push(frame);
}


// RETIRE_CONNECTION_ID
while buf.len() + frame::RETIRE_CONNECTION_ID_SIZE_BOUND < max_size {
    let seq = if let Some(x) = space.pending.retire_cids.pop() {
        x
    } else {
        break;
    };
    trace!(self.log, "RETIRE_CONNECTION_ID {sequence}", sequence = seq);
    buf.write(frame::Type::RETIRE_CONNECTION_ID);
    buf.write_var(seq);
    sent.retire_cids.push(seq);
}

github.com

djc/quinn/blob/master/quinn-proto/src/connection.rs#L2109


        break;
    };
    trace!(self.log, "RETIRE_CONNECTION_ID {sequence}", sequence = seq);
    buf.write(frame::Type::RETIRE_CONNECTION_ID);
    buf.write_var(seq);
    sent.retire_cids.push(seq);
}


// STREAM
while buf.len() + frame::Stream::SIZE_BOUND < max_size {
    let mut stream = if let Some(x) = space.pending.stream.pop_front() {
        x
    } else {
        break;
    };
    if self
        .streams
        .get_send_mut(stream.id)
        .map_or(true, |s| s.state.was_reset())
    {
        self.unacked_data -= stream.data.len() as u64;

That’s just one file and only those instances where we used x as the variable name, because they were easy to find (which to me also means that one of the pains associated with this problem is having to introduce an intermediate variable name).

scottmcm · 2019-04-03T09:50:59.043Z

djc:

    let stream = if let Some(x) = self.streams.get_send_mut(id) {
        x
    } else {
        continue;
    };

While I do like something more pattern-general, I think these cases could all be done with something like let stream = self.streams.get_send_mut(id) || continue; using

Something for coalescing; aka generalized/improved `or_else`

It was noted in the await syntax thread that Option::or_else mixes poorly with await (and ?): https://github.com/rust-lang/rust/issues/57640#issuecomment-457518407 Since one can think of ? as and_then, would it make sense for there to be some kind of standard macro or operator for or_else? Like how C# has the null coalescing operator ?? (which spawned the or_else conversation) and SQL has the COALESCE function. As I’ve been spending a bunch of time with ops::Try lately, I realized we could do…

dodomorandi · 2019-04-03T10:23:02.302Z

What do you think about relying on the unstable try_trait and a macro? Something like:

macro_rules! unwrap_or_else {
    ($e:expr, $op:tt) => {
        if let Ok(x) = $e.into_result() {
            x
        } else {
            $op
        }
    };    
}

This can be improved a lot, but I just wanted to show a basic idea. Obviously, we need to stabilize the Try trait, at that point we could experiment with different solutions that do not require any changes in the language.

djc · 2019-04-03T10:56:51.376Z

scottmcm:

While I do like something more pattern-general, I think these cases could all be done with something like let stream = self.streams.get_send_mut(id) || continue; using

I would like a single-line solution for this pattern, but this usage of continue seems to deviate somewhat from what I’ve seen so far (IIRC it’s mostly used as a statement rather than an expression?).

CAD97 · 2019-04-03T12:47:34.016Z

In your case it’d be break since that’s what the code uses currently.

But yes, all of return _, break, break _, and continue are diverging expressions. Effectively they evaluate to ! and can coerce to any type, so the usage of “.unwrap_or!” here does make sense.

zackw · 2019-04-03T15:13:22.041Z

djc:

 let ss = if let Some(x) = self.streams.get_send_mut(frame.id) {
     x
 } else {
     continue;
 };

@Yato’s match form makes this a little more readable IMO:

let ss = match self.streams.get_send_mut(frame.id) {
    None => continue,
    Some(x) => x
}

But a generalization of ? does seem like it would be even better…

let ss = self.streams.get_send_mut(frame_id).unwrap_or! {
    continue
};

(with a block argument because it’s going to do a control transfer)

scottmcm · 2019-04-04T00:38:32.057Z

dodomorandi:

on the unstable try_trait and a macro?

That’s exactly the macro I linked in the previous post

djc:

but this usage of continue seems to deviate somewhat from what I’ve seen so far (IIRC it’s mostly used as a statement rather than an expression?).

continue and break and return are absolutely expressions – you can already do things like a == b || continue; today, though I agree that conventions tend to prefer ifs for that.

drXor · 2019-04-04T01:47:20.750Z

That is… a scary, valid use of short-circuiting I had not thought of. It is Javascripty in the worstest way, though. ._.

scottmcm · 2019-04-04T02:35:14.522Z

drXor:

It is Javascripty in the worstest way, though. ._.

It’s Perl5-y, actually – or die is classic, as seen in https://perldoc.perl.org/functions/open.html

C# seems to have picked it up too. In constructors it’s common to do this.whatever = whatever ?? throw new ArgumentNullException(nameof(whatever)); – basically it’s the same as an unwrap or precondition-assert!.

drXor · 2019-04-04T02:56:37.186Z

scottmcm:

It’s Perl5-y, actually – or die is classic

You say that like it’s supposed to make me feel better… =P

I’m mostly unhappy about mixing things that are clearly not bools in with short-circuiting (I write a lot of C++ at work, and operator bool is not banned by our style guide…). At least, I don’t think of short-circuit operators as control flow, and it’s really wierd to someone who doesn’t know that break (and keywords that desugar to it) type at !. Something something there should be a clippy lint for using break/continue/return (and maybe in general expressions that type at !) with short-circuit operators.

dodomorandi · 2019-04-04T06:22:21.630Z

Ups, I did not check . However, the version you linked is more interesting than my macro (and safe against other “into_result” methods). Maybe the only problem is that the coalesce crate is already taken .

Centril · 2019-04-04T07:29:15.633Z

drXor:

It is Javascripty in the worstest way, though. ._.

drXor:

At least, I don’t think of short-circuit operators as control flow, and it’s really wierd to someone who doesn’t know that break (and keywords that desugar to it) type at ! .

I’m not sure what you mean by “Javascripty”, but this is not one of those “wat” moments that arises due to ad-hoc design. foo || continue falls out of continue : ! and ! coercing to any other type which is consistent with the language overall.

Also, short-circuit operators really should be thought of as control flow and they may desugar into matches in the HIR soon.

repax · 2019-04-04T08:34:28.995Z

scottmcm:

let stream = self.streams.get_send_mut(id) || continue;

I do like this idea, but I’m a little bit worried. Doesn’t the type of stream change somewhat surprisingly if you exclude the || continue part? It becomes an Option instead:

let stream: Option<&mut Send> = self.streams.get_send_mut(id);

Would the following syntax be clearer?

let Some(stream) = self.streams.get_send_mut(id) || continue;

scottmcm · 2019-04-04T08:46:19.171Z

repax:

Doesn’t the type of stream change somewhat surprisingly if you exclude the || continue part?

No more surprisingly than any other operator. C# has demonstrated that x ?? 0 (where x: int?) or x ?? throw or x ?? continue or whatever is completely understandable in practice.