Add more impls of PartialEq and PartialOrd for strings #135536
Conversation
joshtriplett
added
T-libs-api
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
rustbot
added
the
S-waiting-on-review
|
@bors try |
Add more impls of PartialEq and PartialOrd for strings
Currently, some combinations of `&String` and `&str` don't support
comparison operators. For instance, this:
```rust
fn main() {
let s1 = String::from("hello");
let s2 = "world";
_ = s1 < s2;
}
```
will fail with:
```
error[E0308]: mismatched types
--> src/main.rs:4:14
|
4 | _ = s1 < s2;
| -- ^^- help: try using a conversion method: `.to_string()`
| | |
| | expected `String`, found `&str`
| expected because this is `String`
```
Other combinations only work because the compiler implicitly relies on
impls on different reference types, and that makes such combinations
fragile, breaking if any other impls of `PartialOrd` show up.
Add some additional impls to make such cases work, and to improve
robustness when adding other impls in the future. In particular, I'm hoping that adding these makes it possible to add comparisons with other stringy types without creating as many inference issues.
This comment has been minimized.
This comment has been minimized.
|
Some of the changes in Clippy look good; for instance: --- a/src/tools/clippy/tests/ui/iter_overeager_cloned.fixed
+++ b/src/tools/clippy/tests/ui/iter_overeager_cloned.fixed
@@ -13,13 +13,13 @@ fn main() {
let _: Option<String> = vec.iter().chain(vec.iter()).next().cloned();
- let _: usize = vec.iter().filter(|x| x == &"2").count();
+ let _: usize = vec.iter().filter(|x| x == "2").count();--- a/src/tools/clippy/tests/ui/op_ref.fixed
+++ b/src/tools/clippy/tests/ui/op_ref.fixed
@@ -18,7 +18,7 @@ fn main() {
let a = "a".to_string();
let b = "a";
- if b < &a {
+ if b < a {It's good that this code doesn't need to write However, some of them look bad, and seem to need a non-trivial fix in a Clippy lint: --- a/src/tools/clippy/tests/ui/cmp_owned/with_suggestion.fixed
+++ b/src/tools/clippy/tests/ui/cmp_owned/with_suggestion.fixed
@@ -2,18 +2,18 @@
#[allow(clippy::unnecessary_operation, clippy::no_effect, unused_must_use, clippy::eq_op)]
fn main() {
fn with_to_string(x: &str) {
- x != "foo";
+ x != *"foo";
- "foo" != x;
+ *"foo" != x;--- a/src/tools/clippy/tests/ui/cmp_owned/with_suggestion.stderr
+++ b/src/tools/clippy/tests/ui/cmp_owned/with_suggestion.stderr
@@ -2,7 +2,7 @@ error: this creates an owned instance just for comparison
--> tests/ui/cmp_owned/with_suggestion.rs:5:14
|
LL | x != "foo".to_string();
- | ^^^^^^^^^^^^^^^^^ help: try: `"foo"`
+ | ^^^^^^^^^^^^^^^^^ help: try: `*"foo"`
|
= note: `-D clippy::cmp-owned` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::cmp_owned)]`This should not be suggesting Could someone from @rust-lang/clippy help me figure out how to get clippy to not make that suggestion for |
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
Nevermind, I managed to figure it out. |
|
@bors try |
Add more impls of PartialEq and PartialOrd for strings
Currently, some combinations of `&String` and `&str` don't support
comparison operators. For instance, this:
```rust
fn main() {
let s1 = String::from("hello");
let s2 = "world";
_ = s1 < s2;
}
```
will fail with:
```
error[E0308]: mismatched types
--> src/main.rs:4:14
|
4 | _ = s1 < s2;
| -- ^^- help: try using a conversion method: `.to_string()`
| | |
| | expected `String`, found `&str`
| expected because this is `String`
```
Other combinations only work because the compiler implicitly relies on
impls on different reference types, and that makes such combinations
fragile, breaking if any other impls of `PartialOrd` show up.
Add some additional impls to make such cases work, and to improve
robustness when adding other impls in the future. In particular, I'm hoping that adding these makes it possible to add comparisons with other stringy types without creating as many inference issues.
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
joshtriplett
force-pushed
the
str-impls
branch
from
f7320e2 to
9485326
Compare
rustbot
added
the
T-bootstrap
|
Rebasing and reblessing. |
joshtriplett
force-pushed
the
str-impls
branch
from
63c622d to
1d12da8
Compare
rustbot
added
the
T-bootstrap
joshtriplett
removed
the
T-bootstrap
|
☔ The latest upstream changes (presumably #136209) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to wait to fix further conflicts on this PR until there are more checkboxes on the FCP. |
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
joshtriplett
force-pushed
the
str-impls
branch
from
1d12da8 to
7ac37c9
Compare
rustbot
added
the
T-bootstrap
Currently, some combinations of `&String` and `&str` don't support
comparison operators. For instance, this:
```rust
fn main() {
let s1 = String::from("hello");
let s2 = "world";
_ = s1 < s2;
}
```
will fail with:
```
error[E0308]: mismatched types
--> src/main.rs:4:14
|
4 | _ = s1 < s2;
| -- ^^- help: try using a conversion method: `.to_string()`
| | |
| | expected `String`, found `&str`
| expected because this is `String`
```
Other combinations only work because the compiler implicitly relies on
impls on different reference types, and that makes such combinations
fragile, breaking if any other impls of `PartialOrd` show up.
Add some additional impls to make such cases work, and to improve
robustness when adding other impls in the future.
This avoids making a suggestion to write `*x` if `x` would also work.
In a couple of cases, adjust the test file, because otherwise the file produces: ``` error: failed to apply suggestions for tests/ui/iter_overeager_cloned.rs with rustfix cannot replace slice of data that was already replaced Add //@no-rustfix to the test file to ignore rustfix suggestions ```
This makes such comparisons more robust against the availability of other impls.
joshtriplett
force-pushed
the
str-impls
branch
from
7ac37c9 to
5f3c8ef
Compare
|
@ibraheemdev Does this now look good, other than waiting on FCP to complete? |
|
Yes the implementation looks good. r=me after the FCP. |
| @@ -2530,12 +2531,51 @@ macro_rules! impl_eq { | |||
|
|
|||
| impl_eq! { String, str } | |||
| impl_eq! { String, &'a str } | |||
| impl_eq! { String, &String } | |||
| impl_eq! { &String, str } | |||
There was a problem hiding this comment.
These two impls end up with incorrect stability attributes, which means Clippy msrv autodetection can't be added later.
There was a problem hiding this comment.
That's fixable, but is that worth complicating the macro for? There are many other examples of macro-generated instances with the same discrepancy.
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
ibraheemdev
added
S-waiting-on-author
| @@ -28,6 +28,17 @@ LL | let _: usize = vec.iter().filter(|x| x == &"2").cloned().count(); | |||
| = note: `-D clippy::redundant-clone` implied by `-D warnings` | |||
| = help: to override `-D warnings` add `#[allow(clippy::redundant_clone)]` | |||
|
|
|||
| error: taken reference of right operand | |||
There was a problem hiding this comment.
This lint shouldn't trigger in the test file. Please fix the two new occurrences by removing the ref.
|
☔ The latest upstream changes (presumably #137752) made this pull request unmergeable. Please resolve the merge conflicts. |
Currently, some combinations of
&Stringand&strdon't supportcomparison operators. For instance, this:
will fail with:
Other combinations only work because the compiler implicitly relies on
impls on different reference types, and that makes such combinations
fragile, breaking if any other impls of
PartialOrdshow up.Add some additional impls to make such cases work, and to improve
robustness when adding other impls in the future. In particular, I'm hoping that adding these makes it possible to add comparisons with other stringy types without creating as many inference issues.