opened
estebank
username_0
Fix #63987.
Add check for overlapping ranges to unreachable patterns lint
issue
created
rust-highfive
username_1
r? @username_9

(rust_highfive has picked a reviewer for you, use r? to override)
comment
created
estebank
username_0
Should this be separated to new lint instead of overloading `unreachable_patterns`?

I'm explicitly not handling the "multiple holes caught by `_`" case because it has the potential of being extremely annoying. It likely belongs in clippy complaining about `_` being bad style.

CC @username_2 @oli-obk @username_5
comment
created
varkor
username_2
I'll look at the code more closely later, but I think this should be pulled out into its own lint, as I think there are legitimate reasons to want overlapping ranges (though overlapping by a single element seems less intentional).
comment
created
rust-highfive
username_1
The job `x86_64-gnu-llvm-6.0` of your PR [failed](https://dev.azure.com/rust-lang/rust/_build/results?buildId=7001&view=logs&jobId=d0d954b5-f111-5dc4-4d76-03b6c9d0cf7e) ([raw log](https://dev.azure.com/rust-lang/e71b0ddf-dd27-435a-873c-e30f86eea377/_apis/build/builds/7001/logs/49)). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

<details><summary><i>Click to expand the log.</i></summary>

```plain
2019-08-29T23:24:43.6838383Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-29T23:24:43.7040191Z ##[command]git config gc.auto 0
2019-08-29T23:24:43.7124890Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-29T23:24:43.7174202Z ##[command]git config --get-all http.proxy
2019-08-29T23:24:43.7315436Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64007/merge:refs/remotes/pull/64007/merge
---
2019-08-30T00:00:53.2149461Z    Compiling syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
2019-08-30T00:00:56.8983572Z error: multiple patterns covering the same range
2019-08-30T00:00:56.8984603Z    --> src/librustc_target/abi/mod.rs:494:13
2019-08-30T00:00:56.8985035Z     |
2019-08-30T00:00:56.8985977Z 493 |             -0x0000_0000_0000_0080..=0x0000_0000_0000_007f => I8,
2019-08-30T00:00:56.8986705Z     |             ---------------------------------------------- this range overlaps on `-128i128..=127i128`
2019-08-30T00:00:56.8987629Z 494 |             -0x0000_0000_0000_8000..=0x0000_0000_0000_7fff => I16,
2019-08-30T00:00:56.8988345Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:56.8989197Z     = note: `-D unreachable-patterns` implied by `-D warnings`
2019-08-30T00:00:56.8993751Z 
2019-08-30T00:00:56.9038160Z error: multiple patterns covering the same range
2019-08-30T00:00:56.9038720Z    --> src/librustc_target/abi/mod.rs:495:13
2019-08-30T00:00:56.9038720Z    --> src/librustc_target/abi/mod.rs:495:13
2019-08-30T00:00:56.9039143Z     |
2019-08-30T00:00:56.9039771Z 493 |             -0x0000_0000_0000_0080..=0x0000_0000_0000_007f => I8,
2019-08-30T00:00:56.9040370Z     |             ---------------------------------------------- this range overlaps on `-128i128..=127i128`
2019-08-30T00:00:56.9041465Z 494 |             -0x0000_0000_0000_8000..=0x0000_0000_0000_7fff => I16,
2019-08-30T00:00:56.9042043Z     |             ---------------------------------------------- this range overlaps on `-32768i128..=32767i128`
2019-08-30T00:00:56.9042972Z 495 |             -0x0000_0000_8000_0000..=0x0000_0000_7fff_ffff => I32,
2019-08-30T00:00:56.9043460Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:56.9118088Z error: multiple patterns covering the same range
2019-08-30T00:00:56.9118386Z    --> src/librustc_target/abi/mod.rs:496:13
2019-08-30T00:00:56.9118603Z     |
2019-08-30T00:00:56.9118603Z     |
2019-08-30T00:00:56.9118937Z 493 |             -0x0000_0000_0000_0080..=0x0000_0000_0000_007f => I8,
2019-08-30T00:00:56.9119300Z     |             ---------------------------------------------- this range overlaps on `-128i128..=127i128`
2019-08-30T00:00:56.9119641Z 494 |             -0x0000_0000_0000_8000..=0x0000_0000_0000_7fff => I16,
2019-08-30T00:00:56.9120177Z     |             ---------------------------------------------- this range overlaps on `-32768i128..=32767i128`
2019-08-30T00:00:56.9120647Z 495 |             -0x0000_0000_8000_0000..=0x0000_0000_7fff_ffff => I32,
2019-08-30T00:00:56.9121083Z     |             ---------------------------------------------- this range overlaps on `-2147483648i128..=2147483647i128`
2019-08-30T00:00:56.9121385Z 496 |             -0x8000_0000_0000_0000..=0x7fff_ffff_ffff_ffff => I64,
2019-08-30T00:00:56.9121834Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:56.9167451Z error: multiple patterns covering the same range
2019-08-30T00:00:56.9167785Z    --> src/librustc_target/abi/mod.rs:505:13
2019-08-30T00:00:56.9168028Z     |
2019-08-30T00:00:56.9168028Z     |
2019-08-30T00:00:56.9168323Z 504 |             0..=0x0000_0000_0000_00ff => I8,
2019-08-30T00:00:56.9169093Z     |             ------------------------- this range overlaps on `0u128..=255u128`
2019-08-30T00:00:56.9169393Z 505 |             0..=0x0000_0000_0000_ffff => I16,
2019-08-30T00:00:56.9169904Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:56.9227068Z error: multiple patterns covering the same range
2019-08-30T00:00:56.9227399Z    --> src/librustc_target/abi/mod.rs:506:13
2019-08-30T00:00:56.9227670Z     |
2019-08-30T00:00:56.9227670Z     |
2019-08-30T00:00:56.9227990Z 504 |             0..=0x0000_0000_0000_00ff => I8,
2019-08-30T00:00:56.9228377Z     |             ------------------------- this range overlaps on `0u128..=255u128`
2019-08-30T00:00:56.9228672Z 505 |             0..=0x0000_0000_0000_ffff => I16,
2019-08-30T00:00:56.9229243Z     |             ------------------------- this range overlaps on `0u128..=65535u128`
2019-08-30T00:00:56.9229827Z 506 |             0..=0x0000_0000_ffff_ffff => I32,
2019-08-30T00:00:56.9230170Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:56.9261946Z error: multiple patterns covering the same range
2019-08-30T00:00:56.9262273Z    --> src/librustc_target/abi/mod.rs:507:13
2019-08-30T00:00:56.9262814Z     |
2019-08-30T00:00:56.9262814Z     |
2019-08-30T00:00:56.9263142Z 504 |             0..=0x0000_0000_0000_00ff => I8,
2019-08-30T00:00:56.9263509Z     |             ------------------------- this range overlaps on `0u128..=255u128`
2019-08-30T00:00:56.9263856Z 505 |             0..=0x0000_0000_0000_ffff => I16,
2019-08-30T00:00:56.9264226Z     |             ------------------------- this range overlaps on `0u128..=65535u128`
2019-08-30T00:00:56.9264709Z 506 |             0..=0x0000_0000_ffff_ffff => I32,
2019-08-30T00:00:56.9265162Z     |             ------------------------- this range overlaps on `0u128..=4294967295u128`
2019-08-30T00:00:56.9265467Z 507 |             0..=0xffff_ffff_ffff_ffff => I64,
2019-08-30T00:00:56.9265827Z     |             ^^^^^^^^^^^^^^^^^^^^^^^^^ overlapping patterns
2019-08-30T00:00:58.4044166Z error: aborting due to 6 previous errors
2019-08-30T00:00:58.4049163Z 
2019-08-30T00:00:58.4526332Z error: Could not compile `rustc_target`.
2019-08-30T00:00:58.4545539Z warning: build failed, waiting for other jobs to finish...
---
2019-08-30T00:01:00.2008025Z == clock drift check ==
2019-08-30T00:01:00.2025703Z   local time: Fri Aug 30 00:01:00 UTC 2019
2019-08-30T00:01:00.2905893Z   network time: Fri, 30 Aug 2019 00:01:00 GMT
2019-08-30T00:01:00.2910698Z == end clock drift check ==
2019-08-30T00:01:01.6587570Z ##[error]Bash exited with code '1'.
2019-08-30T00:01:01.6630166Z ##[section]Starting: Checkout
2019-08-30T00:01:01.6631826Z ==============================================================================
2019-08-30T00:01:01.6631877Z Task         : Get sources
2019-08-30T00:01:01.6631927Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
```

</details><p></p>

[I'm a bot](https://github.com/rust-ops/rust-log-analyzer)! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact **`@TimNN`**. ([Feature Requests](https://github.com/rust-ops/rust-log-analyzer/issues?q=is%3Aopen+is%3Aissue+label%3Afeature-request))
comment
created
joshtriplett
username_3
Conclusions from the lang team meeting:

- We think there are legitimate use cases for some kinds of overlaps. For instance, matching a couple of specific letters and then matching a whole range that covers those letters as a fallback.
- Thus, we think this would be prone to false positives.
- We think some heuristics might help with that. For instance, overlapping ranges that don't *completely* overlap (0..10 and 5..15) seem more reasonable to warn about, rather than 5..10 and 0..20.
comment
created
scottmcm
username_4
I feel strongly that this shouldn't be `unreachable_patterns` because the arm is reachable.

My interpretation here is that this needs to be heuristic-heavy, so it being in clippy sounds fine.  But if there's some obvious simple heuristic that's essentially always correct I have no objection to having it in the compiler.  (Maybe something like "the fix is just to change a literal to a different literal".)
comment
created
estebank
username_0
I could restrict this new lint to single element overlap (`0..=10`/`10..=20`) and disjointed partial overlaps (`0..10`/`5..15`) and ignore the common desired case (`100..102`/`0..=255`). That would cover the cases I was worried about to begin with. Is this reasonable? I don't mind dropping this and relying on clippy instead :)
comment
created
Centril
username_5
I think going for the single element overlap (just for ranges) is something I'd be OK with in rustc.
comment
created
bors
username_6
:umbrella: The latest upstream changes (presumably #64271) made this pull request unmergeable. Please resolve the merge conflicts.
comment
created
hdhoang
username_7
ping from triage @username_0, any update after the reviewers' comment?
comment
created
JohnCSimon
username_8
Pinging again from triage. 
@username_0  Can you please address the merge conflicts. 
Thanks!
comment
created
JohnCSimon
username_8
Hello from triage:
@username_0 -
This PR has sat idle for awhile, so I'm closing it as inactive, 
Please reopen this when you have time to resolve the merge conflicts. 

Thank you!
comment
created
estebank
username_0
Rebased and changed logic to only be emitted in the following cases:

```
1 ---------
2     ----------

1     ---------
2  ----------

1  ----
2     ----

1     ----
2 -----
```
comment
created
Centril
username_5
But that's not a single element overlap?
comment
created
estebank
username_0
But I can further restrict it of course.
comment
created
Centril
username_5
Yes, let's start conservative for now with the consensus that is expressed in the PR discussion and we can consider expanding the scope later on.
comment
created
estebank
username_0
@username_5 [done](https://github.com/rust-lang/rust/pull/64007/commits/593cdcccf28361df155c37916dcfcbe1bf19d9a5) and [done](https://github.com/rust-lang/rust/issues/65477).
comment
created
matthewjasper
username_9
@username_6 r+
comment
created
bors
username_6
:pushpin: Commit 593cdcccf28361df155c37916dcfcbe1bf19d9a5 has been approved by `username_9`

<!-- @username_6 r=username_9 593cdcccf28361df155c37916dcfcbe1bf19d9a5 -->
<!-- homu: {"type":"Approved","sha":"593cdcccf28361df155c37916dcfcbe1bf19d9a5","approver":"username_9"} -->
comment