Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

let_chains desugaring is wrong #100513

Closed
Noratrieb opened this issue Aug 13, 2022 · 21 comments · Fixed by #102998
Closed

let_chains desugaring is wrong #100513

Noratrieb opened this issue Aug 13, 2022 · 21 comments · Fixed by #102998
Assignees
Labels
C-bug Category: This is a bug.

Comments

@Noratrieb
Copy link
Member

Noratrieb commented Aug 13, 2022

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 Noratrieb added the C-bug Category: This is a bug. label Aug 13, 2022
@Mark-Simulacrum Mark-Simulacrum added the I-lang-nominated Nominated for discussion during a lang team meeting. label Aug 13, 2022
@Mark-Simulacrum Mark-Simulacrum added this to the 1.64.0 milestone Aug 13, 2022
@Noratrieb
Copy link
Member Author

This also causes issues like #99852 and #100276

@joshtriplett
Copy link
Member

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

Noratrieb added a commit to Noratrieb/rust that referenced this issue Aug 14, 2022
In rust-lang#100513 it was shown that the drop behavior of let_chains is not correct
currently. Since drop behavior is something pretty subtle, this adds
explicit tests for the drop behavior of `if`, `if let` and `match` to
make sure that it does not regress in the future.

The `println!`s were left in to make debugging easier in case something
goes wrong, but they are not required for the test.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 14, 2022
Add tests for the drop behavior of some control flow constructs

In rust-lang#100513 it was shown that the drop behaviour of let_chains is not correct currently. Since drop behaviour is something pretty subtle, this adds explicit tests for the drop behavior of `if`, `if let` and `match` to make sure that it does not regress in the future.

The `println!`s were left in to make debugging easier in case something goes wrong, but they are not required for the test.
@Noratrieb
Copy link
Member Author

Noratrieb commented Aug 14, 2022

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 Noratrieb reopened this Aug 14, 2022
@Noratrieb
Copy link
Member Author

Noratrieb commented Aug 14, 2022

That was a misclick 😆

@Manishearth
Copy link
Member

@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?

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 18, 2022
…compiler-errors

Revert let_chains stabilization

This reverts commit 3266460.

It was discovered in rust-lang#100513 that they are not implemented correctly, which does not make them ready for stabilization.

The merge in the let parsing had a few conflicts, cc `@compiler-errors` and `@c410-f3r` to make sure I did it correctly (alternatively I could also revert `@compiler-errors'` let diagnostic improvement PR as well if a simpler revert is desired).

r? `@Mark-Simulacrum`
@CrimsonVoid
Copy link

CrimsonVoid commented Aug 18, 2022

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
Copy link

T-Dark0 commented Aug 18, 2022

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
Copy link
Member

est31 commented Aug 22, 2022

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
Copy link
Member

est31 commented Aug 22, 2022

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
Copy link

CrimsonVoid commented Aug 22, 2022

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
Copy link
Member

est31 commented Aug 22, 2022

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
Copy link
Contributor

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

@nikomatsakis nikomatsakis removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 13, 2022
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.64.0 milestone Sep 16, 2022
@nathanwhit
Copy link
Member

@rustbot claim

@est31
Copy link
Member

est31 commented Oct 16, 2022

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

est31 commented Oct 16, 2022

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):

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
Copy link
Member

nathanwhit commented Oct 16, 2022

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
Copy link
Member

est31 commented Oct 16, 2022

@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
Copy link
Member

est31 commented Oct 19, 2022

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

@est31
Copy link
Member

est31 commented Oct 20, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants