"Irrefutable Pattern" Warnings in Let Chains

I’m not sure if C++’s comma operator is what I am looking for; I'm not super interested in generalizing this syntax for expressions outside if blocks (but I could be convinced otherwise) or discarding the first/nth return value.

I find it strange that you say this is abusing a feature (which admittedly I have been known to do in the past so it’s not out of the realm of possibility) and at the same time consider if (setup; test) reasonable? In my mind this reads almost the same as if let _ = setup && test and is more flexible than languages that only allow one setup statement. If you didn’t use && you could drop the let _ altogether and write if setup, test and still declare bindings when you need to if let Ok(a) = setup, test || a.check()

Different question, if not sequencing, how do you view let chains and what problem are they trying to solve for you? Can “chains” exist without the “let’s” or is it “let-chains” for you? Why do bindings have to be refutable (i.e. if let hash from @Nemo157's comment seems reasonable to me, even if you were to say if let _ is in poor style)? (Sorry for the barrage of questions)

misc thoughts & opinions 🛸

At this point, I kinda want to ask if && is still a good choice for let chains? The more I think about it, the more I would rather get rid of && for let chains because, ya, the lint is helpful for other cases, just not universally in chains, and I can't think of a reason to prefer && over ,


if do rx.read_exact(); test {

I’m having a hard time trying to figure out why you need the do keyword here? It seems like it should be unambiguous without any additional syntax?

write some interesting code

that is some interesting code, i'll have to remember it for later haha

use () equality, and drop the let binding

I guess that's not the worst thing considering the alternative is universally allowing irrefutable_let_patterns, which I don't really want to do. I hate how it looks though lol. maybe i'll just macro it :pensive:

What do you think of this suggestion, the best one from CAD97's post?

Doesn't that do what you want without the unnecessary let, and without the && you dislike?

even if rustfmt formatted that correctly, it feels a little unsatisfactory considering let chains are so close to addressing this well. it's not that i dislike && (well kinda, it's complicated lol), it just happens to get in the way for this use case.

Prior to let chains rust allowed this code

if let Ok(a) = try_something() {
    ...
} else {
    return ErrSomethingFailed {};
}

but with some consistency this led to code like

if let Ok(a) = try_something() {
    if let Ok(b) = try_something_else() {
        if a.is_cool() && b.isnt_that_bad() {
            ...
        }
    } else {
        return ErrSomethingFailed {};
    }
} else {
    return ErrSomethingFailed {};
}

And okay you can use a match and you can do some other things. Between let-chains and let-else you get some significant ergonomics and the fact is that if you taught me all of the rest of rust and then told me that

if let Ok(a) = try_something() && let Ok(b) = try_something_else() { ... }

doesn't work I would be a little bit confused. Definitely the motivation sections of let-else and if-let-chains have some well thought out answers to some of your questions.

To put on my opinionated hat, if tests a condition, and runs code based on that condition. if communicates to the computer, and more importantly to readers of my code, the block that follows is conditional, it might run, it might not, based on the outcome of a test.

When I write if A && B I am communicating that A might be true, it might be false, B might be true, it might be false, and only if both are true will I run the code block. In your code sample, the code communicates a lie O_O It turns out that A is always true, and we really only want to test B, and in context conveniently that means && works the same as ;.

But rust actually already has ;, so just use that.

10 Likes

What you have is:

  1. an action with side effects (reading from a source, and filling a buffer)
  2. a condition that checks the outcome of that action
  3. some conditional code that depends on that condition

This seems perfectly expressed by the most straightforward code:

action();
if condition() {
    conditional_code();
}

You say that the condition is closely related to the preceding action -- but that is almost always true! That is already expressed by doing the action right before checking the condition -- consecutive statements in code are typically related.

You can cram the action() behind the if:

if {
    action();
    condition()
} {
    conditional_code();
}

but I fail to see any benefits of doing that.

You still have two expressions separated by ;. The differences are:

  • The if keyword has moved to a different location. The new location makes less sense because it's further away from the condition. It makes it harder to understand what exactly the actual condition being tested is.

  • action() is now indented. This makes it harder to understand that action() is done unconditionally and that it has side effects that persist after the whole if statement.

2 Likes

How about this:

rx.read_exact(&mut buf).await?; if buf != info_hash {
    code()
}

This doesn't require extra braces!

1 Like

What about a situation like:

if
    let Ok(a) = something() &&
    let b = some_function(a) &&
    some_condition(&b) &&
    some_other_condition(&b)
{
    /* ... */
} else {
    /* ... */
}

You can't inline the let b = some_function(a) because it's used in two conditions, and extracting it into an additional binding introduces a new nesting level which if-let-chains aimed to avoid.

1 Like

There is no warning for this case. It only triggers if the irrefutable pattern is the first thing or the last thing in the chain.

3 Likes

I'm glad to hear that detail got implemented; I remember we talked about it when considering let-chains, and several of us felt that being able to bind something in the middle of a let-chain was important, for this exact reason (you can't pull it before the conditional because another condition comes before it, and you should be able to name expressions for simplification or repeated use).

if { setup; test } and all small variations

i feel like your're all getting hung up on the fact that this specific example discards its return value and not what this feature actually is.

fn setup() -> &mut T; if let borrow = setup(); test

consider why you might want to drop mutable references as quickly as possible givens rusts sharing rules. what about Guards, Handles, or other scoped temporaries?

if let Ok(a) = try_something() && let Ok(b) = try_something_else()

you're using the feature as it is to defend itself. if the rfc proposed ; your example would be exactly the same except s/&&/;. i'm not sure why let-else is relevant here? can you elaborate?

Of course. This example conditionally runs a block of code when matches!(Ok(_), try_something()) && matches!(Ok(_), try_something_else()) and it won't try_something_else() if that first part of the test fails and it introduces bindings. So the semantics are quite aligned with && and quite different from ;.

That style is used a lot in rustc.

1 Like

i asked why let-else is relevant. you explained why s/&&/; isn't the same. fine w/e, use , and define the semantics you want; that's not the point.

saying semantics are aligned with && when && has literally only been used with boolean operands (not truthy exprs) is a little absurd

why are people so intent on not wanting to support if setup, test. is it really worth keeping some kind of cutesy symmetry with &&

Because if setup, test { is already supported in the form of if { setup; test } {.

1 Like

and again you've circled back to the specific example that discards its return value. can you please take a minute reading before replying. even if so, why do you want to restrict how you can use this?

Some of the design decisions behind if-let-chains and let-else are interrelated, this is discussed mostly in the let-else RFC. They both solve generally the same problem (ergonomics around conditional let).

There is some additional discussion about wanting to leave the door open for let X(_) = ... to be treated as an expression returning a boolean, or at least to mimic those semantics. The syntax was purposefully geared towards making this a natural analogy; in any case the motivation and justification and even some counterarguments are in the RFCs, as well there are some discussion threads for the pre-RFCs. Which is all a lot to read I know but it's also a lot to retread.

2 Likes

Then what do you want those semantics to mean? The comma operator in c/c++ does just discard the result of setup. If you want to use the result itself then you can just bind it in a block.

I guess I can sort of envision a case where you would want to bind a value to a name (unconditionally) and then continue the if chain also with patterns (of the form && let Some(inner) = get_maybe() .... If that is what you are talking about then sure this lint might be annoying. But it also isn't wrong and having it above the if in a block of its own isn't that bad either.

I think I've figured out the case which might actually be a false positive. (Saying "don't fixate on the previous example where the result is discarded and not providing an obvious new example is problematic because people will prefer discussing an example over a missable theoretical. Plus, asynchronous communication and all that means people aren't always replying just to the most recent thread status.)

Consider:

if let handle = mutex.lock().unwrap()
&& handle.test() {
    // do things with handle
}

// do things where the lock must be released

(Note that the unwrap here is desired, to propagate the panic that poisoned the mutex. Using if let Ok would be incorrect.)

This is potentially subtle because of drop order. The equivalent without the irrefutable if let would have to be

let handle = mutex.lock().unwrap();
if handle.test() {
    // do things with handle
}
drop(handle); // !important

// do things where the lock must be released

in order to drop the handle, or introduce an extra block, e.g.

{
    let handle = mutex.lock().unwrap();
    if handle.test() {
        // do things with handle
    }
}

// do things where the lock must be released

Note however that this isn't a problem if handle were some &mut (as in what might be your new example that you wanted people to consider? it is very unclear since the example was not spelled out well) as the lifetime &mut can be terminated early ("non lexical lifetimes"). The end-scope drop timing is only relevant for types which have drop glue (impl or transitively contain something which impl Drop).

So considering this example, I do think it might be reasonable to relax the irrefutable binding lint for the case where

  • the value is bound; and
  • the value's type has a semantic drop.

Thus

  • if let _ = setup() && test is linted against as an unbound irrefutable if let pattern, with a suggestion to move setup() before the if.
  • if let v = setup() && test where v is a reference is linted against as an irrefutable if let pattern, with a suggestion to move let v = setup(); before the if.
  • if let v = setup() && test where v has drop glue but is not used is linted against as an unused binding, with a suggestion to use or rename as _v.
  • if let v = setup() && test where v has drop glue and is used is not linted against.

This applies equally to any irrefutable pattern with a binding. Two additional clarifying notes:

  • If all patterns in an if let chain are irrefutable and no boolean tests exist, lint; it is better to just use a normal block and lets inside the block.
  • An unchained if let satisfies the "all" clause above, so this change only impacts unstable code.

I do think it's still potentially misleading in that the assignment cannot fail, but the drop order benefits are stronger when they are relevant IMHO.

5 Likes

I agree that the warning should be relaxed in some cases, especially since irrefutable patterns already are an accepted style in the middle of a let chain.

I think it should be relaxed for all cases when the first let in the chain (but not the last let in the chain) binds a variable, regardless of whether there is drop glue on its type.

Scoping a variable locally is convenient regardless of whether or not there is drop glue. It would be confusing if the warning allowed it for some types and not for others.

Example:

if let potential_answer = calculate() &&
        valid_answer(&potential_answer) {
    return potential_answer;
}

With your proposal, this would be a lint for i32 but not for Vec<i32>. I think if the lint is relaxed, it should be relaxed in this case for all types.

The alternative is to either keep potential_answer in scope longer than necessary, or use extra braces, similarly to your handle example:

{
    let potential_answer = calculate();
    if valid_answer(&potential_answer) {
        return potential_answer;
    }
}

This is already covered by the warning on the last let in the chain being irrefutable. That never makes sense, and shouldn't be relaxed.

3 Likes

I see. I misunderstood what the lint is about -- I thought it was specifically about if let _ = ... && ..., i.e. discarding the result of the left-hand side of the &&, in which case why not write a plain if?

I'm in agreement with @tczajka (comment 40) that the warning should be relaxed for an irrefutable first let in a conditional chain, whenever it binds a name (as it already is in the middle of a chain).

1 Like