let_chains desugaring is wrong

[Noratrieb]

I tried this code: playground

struct LoudDrop(&'static str);

impl Drop for LoudDrop {
    fn drop(&mut self) {
        println!("Dropped! {}", self.0);
    }
}

fn get_drop(str: &'static str) -> Option<LoudDrop> {
    Some(LoudDrop(str))
}

fn main() {
    println!("1");
    if get_drop("first").is_some() && get_drop("1.5").is_some() && let None = get_drop("last") {
        println!("2");
    } else {
        println!("2");
    }
    println!("3");
}

This prints

1
Dropped! 1.5
Dropped! last
2
Dropped! first
3

According to RFC 2497, it should desugar to this: playground

fn main() {
    println!("1");
    'FRESH_LABEL: {
        if get_drop("first").is_some() {
            if get_drop("1.5").is_some() {
                if let None = get_drop("last") {
                    break 'FRESH_LABEL { println!("2") }
                }
            }
        }
        { println!("2") }
    }
    println!("3");
}

But this prints

1
Dropped! first
Dropped! 1.5
Dropped! last
2
3

Meta

Wrong on the current beta

1.64.0-beta.1

(2022-08-09 56714e5332f5f21c39d7)

[Noratrieb]

This also causes issues like #99852 and #100276

[joshtriplett]

Yeah, that definitely looks wrong; we will have to roll this back and try again for 1.65.

[Noratrieb]

I've opened a zulip thread about fixing them. cc @c410-f3r, I wasn't able to find you on zulip to ping you there

[Noratrieb]

That was a misclick 😆

[Manishearth]

@joshtriplett given rust-lang/rustfmt#5203 (comment) , can we use this opportunity to also ensure that there is a style decision put forth by the lang team in place before restabilizing so that rustfmt is able to merge formatting for let_chains?

[CrimsonVoid]

I'm not sure if this is the right place to provide feedback, but has there been any consideration on disabling 'irrefutable pattern' warnings for cases like if let _ = rx.read_exact(&mut buf).await? && buf != info_hash. I find myself repeating this pattern often enough that the compiler warnings are starting to get be kind of much.

[T-Dark0]

This isn't the proper place to discuss the feature, to my knowledge. That would probably be an IRLO thread (internals.rust-lang.org), or a Zulip one

[est31]

Note that one doesn't have to follow the RFC, what gets stabilized in the end can sometimes deviate from the things that an RFC decides upon. I'm not saying that the current drop order is good for stabilization though, it isn't. One could e.g. drop temporaries more early on, as in, before the else block runs. Then stuff accessed in the if expression can safely be re-used from the else block.

[est31]

I'm not sure if this is the right place to provide feedback, but has there been any consideration on disabling 'irrefutable pattern' warnings for cases like if let _ = rx.read_exact(&mut buf).await? && buf != info_hash.

The irrefutable pattern lint was specifically extended to let chains in #94951 , after my comment on the tracking issue received only positive responses (see the thumbs up reactions). As for the right place to provide feedback, it is let chains related I would say, but despite that, this specific thread is the wrong place to discuss it, as it is about the desugaring and not about let chains in general.

Personally I don't think it should be changed because the lint was made in a way that if it fires, there exists a way to move the irrefutable pattern out of the let chain. Your particular code would work with read_exact(&mut buf).await?; if buf != info_hash for example. This is similar to how if let _ = foo() { bar(); } lints on current master today because you can write that as let _ = foo(); bar(); (and remove the _ if the function is not #[must_use]).

Again, so far this lint has only received positive feedback.

[CrimsonVoid]

Again, so far this lint has only received positive feedback.

Ah yes, the black swan theory of RFCs. Well, consider this to be your first black swan.

[est31]

either I reply to your comment here and continue to derail the thread or let you have the last word.

The third option is do what was suggested: open a zulip thread, github issue, or internals thread, or such where you can make your case. You could have made that instead of making that comment to complain on how I have the last word. I didn't open a thread because that's your responsibility: you are the person who has the complaint, not me, and so far you haven't delivered convincing points why a change would make sense. I replied to you because that did feel like my responsibility, as I was the person who implemented the lint.

[nikomatsakis]

Discussed in lang-team meeting. We are removing the nomination. The current behavior seems clearly surprising and also not in accordance with the RFC.

[nathanwhit]

@rustbot claim

[est31]

Now that the first nightly with #102998 is out, I've tried running again the snippet from above. The snippet now prints:

1
Dropped! 1.5
Dropped! first
Dropped! last
2
3

This is not 100% what the desugaring from the RFC prints (should be "first", "1.5", "last"). It's closer than what is seen at the top of the issue's thread, but not quite it. Over at #102998 , @cjgillot has brought up similar concerns.

So maybe this issue should be reopened?

[Noratrieb]

I agree, I think this issue should only be closed once T-lang decided on a desired drop order and the compiler implements it.

[Noratrieb]

Although actually I have to say that my desugaring might be be fully correct. I split up a normal && into two nested ifs which is probably wrong. If it is let together, we should the current output.

[est31]

Yeah it doesn't have to match it 100%, one always deviate in the end from the RFC. What is important is that the details are well understood and approved. #102998 has greatly improved the state but we aren't there yet I think.

What is currently implemented

So in order to figure out what the current desugaring is doing, I've extended the drop_order.rs test that you added:

        if self.option_loud_drop(4).is_some()
            && self.option_loud_drop(1).is_some()
            && self.option_loud_drop(2).is_some()
            && self.option_loud_drop(3).is_some()
            && let Some(_d) = self.option_loud_drop(19)
            && let Some(_e) = self.option_loud_drop(18)
            && let Some(_e) = self.option_loud_drop(17)
            && self.option_loud_drop(5).is_some()
            && self.option_loud_drop(6).is_some()
            && self.option_loud_drop(7).is_some()
            && self.option_loud_drop(8).is_some()
            && let Some(_e) = self.option_loud_drop(16)
            && let Some(_e) = self.option_loud_drop(15)
            && let Some(_e) = self.option_loud_drop(14)
            && self.option_loud_drop(9).is_some()
            && self.option_loud_drop(10).is_some()
            && self.option_loud_drop(11).is_some()
            && self.option_loud_drop(12).is_some() {
                self.print(13);
        }

And:

        if let Some(_d) = self.option_loud_drop(8)
            && let Some(_e) = self.option_loud_drop(7)
            && let Some(_e) = self.option_loud_drop(6)
            && self.option_loud_drop(1).is_some()
            && self.option_loud_drop(2).is_some()
            && self.option_loud_drop(3).is_some()
            && self.option_loud_drop(4).is_some() {
                self.print(5);
        }

Compare this with normal if chains:

        if self.option_loud_drop(5).is_some()
            && self.option_loud_drop(1).is_some()
            && self.option_loud_drop(2).is_some()
            && self.option_loud_drop(3).is_some()
            && self.option_loud_drop(4).is_some() {
            self.print(6);
        }

And nested ifs:

        if let Some(_d) = self.option_loud_drop(4) {
            if let Some(_e) = self.option_loud_drop(3) {
                if let Some(_e) = self.option_loud_drop(2) {
                    self.print(1);
                }
            }
        }

So nested if let's drop in reverse definition order after their body. On the other hand, let-free ifs with && drop in reading order, except for the first condition, which gets droped last (and all conditions drop before their body). Let's call this the "twist".

For let chains, the current implementation seems to:

1. Drop all let conditions after the then body, but before the else body (edit: currently leading let conditions will always drop after the else as @nathanwhit points out below), and also after the else body (as of Let expressions on RHS shouldn't be terminating scopes #103034)
2. Drop all non-let conditions before the then/else body
3. Drop let conditions in reverse definition order
4. Drop non-let conditions mostly in definition order, but with a special version of the twist: if there is a block of non-let conditions right at the start of the if chain, then the first non-let condition is dropped after all other non-let conditions that are part of that leading block, but still before any possible non-let conditions that come after the first let condition.

What behaviour should look like

Personally, I agree a lot with the current implementation, except for that weird exception in rule number 4.

Rules 1 and 3 make sense because you need to keep the bindings around for the stuff that possibly uses them, which can be either the subsequent let conditions or the body. Ideally however we would deviate from if let in that we drop before the else, like if is doing it.

Rule 2 makes sense because ideally you drop stuff as quickly as possible. Same goes for rule 4, except the twist.

My proposal would be to:

1. Drop all let conditions after the then body, but before the else body
2. Drop all non-let conditions before the then/else body
3. Drop let conditions in reverse definition order
4. Drop non-let conditions in definition order

Changes required from the current implementation on master (as of 2022-10-20):

• dropping let temporaries before else body: Let chain temporaries should drop before else #103248
• removal of the && twist (in general or only for let chains): Remove drop order twist of && and || and make them associative #103293

What do you think?

An alternative proposal would drop everything in reverse order, after the body. This rule would be simpler, but you would keep around the temporaries for way too long.

---

(Side note: Ideally the twist would also be eliminated from normal if's but I'm not sure if we can do this due to backwards compatibly. This step would make bool-only && chains them associative wrt drop order though, and also if let chains associative, at least when we consider a hypothetical extension that allows ()s in let chains. I'm not sure if there are issues about this, and if not, maybe I should file one Edit: issue filed #103107)

[nathanwhit]

One note:

1. Drop all let conditions after the then body, but before the else body

This bit isn't quite accurate. In the current implementation it's inconsistent -- if the let is leading then it will drop after the else body, but if it is trailing it will drop before the else body. (example, which at time of writing prints 1,2, then 2,1,).

This is the result of the same bug that causes #100276. After #103034, let conditions will always drop after the else body, just like with normal if let (so the aforementioned example would print 1,2, then 1,2,).

[est31]

@nathanwhit thanks! I've edited my comment.

Regarding #103034, I wonder if it's possible to change it so that it always drops before else? Keeping around the references for the then block makes sense, but else doesn't need any of them, and it would be nice if you could re-access stuff in the else block that if let was accessing. The same holds for if let too, see #103108 which also contains an example of code that is currently not compiling due to the behaviour of dropping before the else drop order. It might be possible that if let can't be changed any more, but it would be great IMO if let chains would start with the better behaviour.

[est31]

I've filed an issue for let chains else dropping behaviour: #103248

[est31]

PR filed to get rid of the && drop order twist in all Rust: #103293