-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Add more impls of PartialEq and PartialOrd for strings #135536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
r? @ibraheemdev
rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
@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 &"2".
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 *"foo", since "foo" works just fine.
Could someone from @rust-lang/clippy help me figure out how to get clippy to not make that suggestion for clippy::cmp-owned?
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
Build commit: da49290 (da4929024a7261062885cb3209c9d7fdc919d233)
@craterbot check
👌 Experiment pr-135536 created and queued.
🤖 Automatically detected try build da49290
🔍 You can check out the queue and this experiment's details.
i️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
🚧 Experiment pr-135536 is now running
i️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
🎉 Experiment pr-135536 is completed!
📊 45 regressed and 3 fixed (569212 total)
📰 Open the full report.
i️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
This comment has been minimized.
This comment has been minimized.
f7320e2 to
9485326
Compare
Rebasing and reblessing.
63c622d to
1d12da8
Compare
☔ 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
commented
Jan 29, 2025
🔔 This is now entering its final comment period, as per the review above. 🔔
1d12da8 to
7ac37c9
Compare
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.
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.
@riking
riking
Feb 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
commented
Feb 8, 2025
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lint shouldn't trigger in the test file. Please fix the two new occurrences by removing the ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, will fix.
☔ The latest upstream changes (presumably #137752) made this pull request unmergeable. Please resolve the merge conflicts.
Uh oh!
There was an error while loading. Please reload this page.
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.