This is a copy/paste from a comment on Github to provide some feedback on let chains.
I started using let chains recently and I find myself running into "leading irrefutable pattern in let chain" warnings for cases like if let _ = rx.read_exact(&mut buf).await? && buf != info_hash a little more often than I'd like. Has there been any (re)consideration on disabling 'irrefutable pattern' warnings for pre-conditions in let chains?
The obvious question: why do you prefer
if let _ = rx.read_exact(&mut buf).await? && buf != info_hash {
code()
}
to
let _ = rx.read_exact(&mut buf).await?;
if buf != info_hash {
code()
}
or even (rustfmt hates this)
if { let _ = rx.read_exaxt(&mut buf).await?; buf != info_hash } {
code()
}
There's also been some talk on if .ok() (or more reasonably, a new .ignore()) to acknowledge a #[must_use] Result without a let _ is reasonable.
FWIW, this specific case also feels like what it actually wants is a .read_buf_exact(&mut _) -> &mut [u8], so you'd just write
if rx.read_buf_exact(&mut buf) != info_hash {
code()
{
Because I consider read() to be a precondition for the boolean expression. Splitting it out would be separating one conceptually grouped concept into two separate statements which hurts readability IMO. Ideally I would like to avoid the _ binding altogether but that is not possible with &&.
read_exact is defined by AsyncReadExt in tokio.
You early return if read() returns an error. There is no way to run any code below read() unless it is successful. In that sense literally every piece of preceding code is a precondition. It doesn't make sense to treat read() differently.
Your example would make more sense if you performed the buf != info_hash check only if read() was successful, and just go to the next statement otherwise. But that case is captured by the legitimate
if rx.read_exact().is_ok() && buf != info_hash { .. }
pattern.
Perhaps this pair of operations could be split into its own utility function, as check_hash(info_hash, &mut buf).await?. But rather than bikeshed this particular example...
I don't expect this particular lint catches many bugs in practice. The RFC doesn't really give much in the way of justification for the lint, but from context it seems like its intention may be educational. Is there a reason something like
#![allow(irrefutable_let_patterns)]
doesn't work for your codebase? I think it will require a few more motivational examples (both for and against the lint) to really capture its value, and I don't think any of the mistakes this lint captures while learning the feature are going to be present in "real code".
literally every piece of preceding code is a precondition
I get what you’re saying, but I feel like you’re being a little pedantic here and missing the point of how this code is logically structured. It’s not that read is treated differently, but rather what I am trying to express is different. To reiterate, I view this as one grouped concept not two separate statements.
Your example would make more sense…
I know you probably didn’t mean anything by it, but please don’t tell me how to write my own code. I appreciate feedback and suggestions, but nothing annoys me more than people assuming they, or the language in this case, know more about how to structure my own program than me. Tl;dr small suggestions: okay; unsolicited feedback: not okay. Thank you 
To address your feedback though, this isn’t about trying to restructure this particular example but a pattern I see that comes up in other languages with constructs similar to let chains. I don’t have a lot of significantly different Rust examples worth sharing, since this is fairly new for Rust, but another similar pattern from a Swift project if buffer.hasPrefix(prefix), let tok = fn() (if you’re not familiar with Swift, , is roughly && in let chains). I have a couple other examples from Go and Python, but I don’t feel comfortable sharing those since it is company code and not exactly public domain. (I hope the Swift example also clarifies what I mean by “pre-condition” a bit, I know I’m not explaining it really well here)
Is there a reason something like
#![allow(irrefutable_let_patterns)]doesn't work for your codebase?
I actually had this on for a while and just forgot about it haha. I was going to just leave it but I thought I would try to provide some feedback rather than just suffer silently lol. And it’s not something that comes up a lot, but it is a pain point I foresee myself running into more and more.
There’s an additional example from Swift from my reply above, it’s not exactly the same, but similar. if buffer.hasPrefix(prefix), let token = fn() { .... I'll try to find more if I can.
I don't think any of the mistakes this lint captures while learning the feature are going to be present in "real code".
I think I’d agree with this.
So thinking about this some more, and going off the RFC, I think this lint exists for the same reason we have unreachable code lints, or why there could be a lint against if true.
if true {
do_something();
}
This is always replaceable by just do_something(). I'm left wondering if some debugging code wasn't cleaned up or questioning the author's intent. Do they know what if true does?
Maybe that is a little uncharitable, but in the case of a more complicated (and at the moment new) feature like if let it's completely reasonable. Perhaps the author meant to destructure or use a value? Do they think the if let would fail? I would ask for if 1 > 0 && buf != info_hash { ... } to be rewritten, it is probably reasonable to rewrite if let _ = rx.read_exact(&mut buf).await? && buf != info_hash to better reflect our shared understanding.
Note that there is a lint against while true, so it wouldn't be unreasonable to have one for if true either.
(Assuming it's suppressed for conditions that are macro parameters and for things like if cfg!(foo).)
The whole point of the linter is to provide feedback on how to structure the program better. You're asking why the lint exists, so answers discussing better ways to structure the code are totally on topic. Also for the benefit of other forum participants (like me).
If you simply want to disable lints, you can do rustc --cap-lints allow.
So is your comment about whether or not the author knew how to “correctly” use let chains, and, specifically, the use of a _ binding in this case? If I were to desugar this it would read rx.read_exact(&mut buf).await?; if buf != info_hash {, the _ binding is only required because && is overloaded to mean different things based on context. Honestly I would rather replace && so it’s not ambiguous (among other reasons) and keep the unused _ binding warning, but that seems like a much harder conversation to have than just disabling this lint.
Also, unless let chains are significantly different than similar constructs in other languages, ya I think it is a little uncharitable. Go, Swift, and Python all consider the feature teachable/learnable in a paragraph or two at most.
The whole point of the linter is to provide feedback on how to structure the program better
I don’t think any linter does whole structure analysis, just individual statements. Even if it did, I think most novice to intermediate programmers understand the structure of their code better than most simple lints can.
You're asking why the lint exists
No, I think I know why the lint exists, I’m asking for it to be disabled with motivating examples from real code in Rust and other languages. Again this isn’t my ideal solution, but probably the most pragmatic one.
If you simply want to disable lints
I don’t.
It's only a convincing motivating example if it's better than the alternatives that the lint is hinting at. Hence, it makes no sense to complain that people are comparing your code to alternatives.
The lint makes a lot of sense to me. If the first let statement is irrefutable, it makes no sense to put it behind the if. Putting it behind if suggests that there is some condition being checked while there isn't any.
The example would be a bit more convincing if there was a variable introduced, because then this would allow to keep the variable local to the if while avoiding additional braces. E.g. this seems somewhat more reasonable:
if let y = x + 5 && y > 100 {
return y;
}
Kind of. I'm saying the original code contains a very similar smell to the code
if 0 <= rx.read_exact(&mut buf).await? && buf != info_hash { ... }
The 0 <= ... part is always true, and I expect the code's author to know this; it leads me to wonder if the code is doing what they intended. The lint is just asking, you know you don't need if here, right?
Now if this is an idiom in your codebase, it's just a lint disable it. But a lint which unconditionally simplifies code seems fine enough in my opinion.
This reply is to both of you since I think you’re talking about the same thing and I think this thread is going around in circles.
_ binding and would make more sense with a variable binding._ binding is only required only required to disambiguate && to which you just loop back to step 2.You both seem to view the _ binding as a code smell, which it is, but there is not a way around it in let chains. Again, ideally I would love to avoid the _ binding altogether, but that is not possible with the choice of &&.
Sequencing of operations is a very natural extension to the intuition I imagine most people have when introduced to let chains, and that is exactly what I’m trying to express here. (Arguably, it seems the choice of && along with irrefutable binding warnings might hinder having this view.)
It's only a convincing motivating example if it's better than the alternatives…
I wasn’t complaining in general, I was asking for unsolicited feedback specifically to be kept to a minimum. If you would like to discuss alternative ways to structure this specific example we can in a different thread, but it is off-topic here, and gets to be a little emotionally draining when you have to constantly defend yourself from “well meaning” drive-by comments. Also, please try to be mindful of accidentally invalidating other peoples opinions; I wouldn’t have shared those examples if I didn’t believe them to be good reasons for disabling this lint in let chains.
lint which unconditionally simplifies code
I don’t know if I agree with this point of view.
This thread is starting to veer awfully close to telling me how to write my own code again which I am NOT a fan of!
The lint works as expected. You're welcome to disable it in your crates and enforce whatever style you want, but it is quite unlikely that you will get many people to agree with you, or that the lint will be disabled.
To me, the takeaway from this entire thread is a nice worked example of why it's bad to relegate the happy return value of an API to an out-parameter. If read_exact returned the usable subslice of the buffer on success, OP would have naturally written
if rx.read_exact(&mut buf).await? != info_hash {
// ...
}
or perhaps
if let hash = rx.read_exact(&mut buf).await? && hash != info_hash {
// do something here that uses `hash`
}
and we wouldn't be having this argument.
This would hit the same lint and be pushed towards
let hash = rx.read_exact(&mut buf).await?;
if hash != info_hash {
// do something here that uses `hash`
}
So what you want is a comma operator, in order to write multiple expressions in one expression position (the if scruitinee).
Given the fairly universal derision of the C/C++ comma operator, I think it should be clear why you're getting pushback.
The equivalent in Rust for sequencing multiple expressions is to use a block, e.g.
if { rx.read_exact(&mut buf).await?; buf != info_hash } {
// code...
}
If this is what you want to write, then you should write it. Abusing if let as sequencing is not what if let should be used for. This is why the irrefutable patterns lint exists: the existence of an irrefutable if let is a likely bug, because if let should only be used for refutable patterns.
Rust has some opinionated lints (namely, everything under nonstandard_style). If your code style disagrees with some lints, #[allow] them (ideally with a comment explaining why).
This is ultimately a style question. If you ask to disable a lint, it's on you to show that the "false positive" rate of the lint is more problematic than the "true positive" rate is beneficial.
On the other hand, some other newer C-syntax-family languages also see the desire to have a setup statement in if, and if (setup; test) {} is somewhat common to see. This logical grouping of a setup and test is clearly desirable, so it's not a completely unreasonable ask.
Most Rust developers will say that the answer is to use a block and/or whitespace to indicate the grouping, though. I see no issue with the logical grouping implied by
// before...
// before...
rx.read_exact(&mut buf).await?;
if buf != info_hash {
// code...
// code...
}
// after...
// after...
that would be improved by writing
// before...
// before...
if do rx.read_exact(&mut buf).await?; buf != info_hash {
// code...
// code...
}
// after...
// after...
(imaginary the least bad syntax I could come up with for a sequencing operator which is not just a block.)
You'll be in good company, too; multiple notable Rust developers are known to write some interesting code that does things like
match match match {
... => ...
} {
... => ...
} {
... => ...
}
It's questionable to use a block in scruitinee position, but it has its merits for grouping in cases like this.
One thing I note doesn't trigger the irrefutable pattern warnings, is perhaps a bit odd but you can use () equality, and drop the let binding
if rx.read_exact(&mut buf).await? == () && buf != info_hash {
}