match_overlapping_arm: Does not seem valid

[Ploppz]

At least I can't make sense of it:

warning: some ranges overlap
   --> pdf/src/primitive.rs:187:17
    |
187 |                 b' ' ..= b'~' => write!(f, "{}", b as char)?,
    |                 ^^^^^^^^^^^^^
    |
    = note: `#[warn(clippy::match_overlapping_arm)]` on by default
note: overlaps with this
   --> pdf/src/primitive.rs:186:17
    |
186 |                 b'"' => write!(f, "\\\"")?,
    |                 ^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm

The code:

        for &b in &self.data {
            match b {
                b'"' => write!(f, "\\\"")?,
                b' ' ..= b'~' => write!(f, "{}", b as char)?,
                o @ 0 ..= 7  => write!(f, "\\{}", o)?,
                x => write!(f, "\\x{:02x}", x)?
            }
        }

Obviously I want the second arm only when the first arm was not matched.

Meta

• cargo clippy -V: clippy 0.0.212 (de521cb 2020-08-21)
• rustc -Vv:

rustc 1.47.0-nightly (de521cbb3 2020-08-21)
binary: rustc
commit-hash: de521cbb303c08febd9fa3755caccd4f3e491ea3
commit-date: 2020-08-21
host: x86_64-unknown-linux-gnu
release: 1.47.0-nightly
LLVM version: 10.0

[flip1995]

I think the lint is correct here. In the ASCII table " is between and ~, so the arms overlap. For me the question is rather if this lint should be warn-by-default 🤔

A possible fix would be to write the second arm like this: b' '..b'"' | b'<the sign after ">'..b'~'. But I wouldn't consider this better style.

[Ploppz]

I reported this issue because I have generally taken advantage of the fact that any match arm implicitly excludes any patterns that comes before it. I use this a lot in other cases like

                match n_updates {
                    Ok(1) => (),
                    Ok(n_updates) => error!(log, #"poller", "Updating orphan job";
                            "n_updates" => n_updates),
                    Err(e) => error!(log, #"poller", "Updating orphan job";
                            "error" => %e),
                }

So according to my personal experience I don't see this as an anti-pattern, and I think indeed the proposed fix is not good style.
I think my code is the best way to write it.

[Gadiguibou]

Hey, I'd like to contribute to this! I've had the same issue in the past and wonder if this really needs fixing as well or just shouldn't be warn-by-default. Is there anyone that could confirm if this is the way to go?

[ebroto]

Hey @Gadiguibou, I would say that the outcome of the discussion is still not clear :)

The example given in the issue that created this lint (#471) has more probability to be an error than to leverage the order of the arms:

let x = 5;
match x {
    1 ... 10 => println!("1 ... 10"),
    5 ... 15 => println!("5 ... 15"),
    _ => (),
}

IMO including 5..10 in the second arm is unnecessarily confusing.

What if the lint checked that the ranges overlap partially, but excluded cases where one pattern is completely included in the other? To make it more graphical; this would be linted:

|----------|
        |-----------|

but this would not be linted:

              |---|
|--------------------|

[Gadiguibou]

That sounds very nice indeed, the order of the arms would probably have to be considered as well. Or have an entirely new lint for cases where a match arm is unreachable because of overlap with a previous arm.

Edit: the simplicity of the implementation is probably something to consider as well. Adding many lints for very similar cases is probably unnecessary and inefficient.

[flip1995]

Or have an entirely new lint for cases where a match arm is unreachable because of overlap with a previous arm.

The compiler will already complain about this ;) Playground

---

I like the suggestion by @ebroto. I think this is the way to go, instead of downgrading the lint 👍