Add more impls of PartialEq and PartialOrd for strings #135536

joshtriplett · 2025-01-15T14:49:31Z

Currently, some combinations of &String and &str don't support
comparison operators. For instance, this:

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.

rustbot · 2025-01-15T14:49:39Z

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

joshtriplett · 2025-01-15T14:51:08Z

@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.

bors · 2025-01-15T14:52:20Z

⌛ Trying commit 8b933dd with merge 7abdc84...

joshtriplett · 2025-01-15T16:00:06Z

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?

rustbot · 2025-01-15T16:37:13Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

joshtriplett · 2025-01-15T16:37:38Z

Nevermind, I managed to figure it out.

joshtriplett · 2025-01-15T21:37:31Z

@bors try

bors · 2025-01-15T21:38:41Z

⌛ Trying commit d972108 with merge da49290...

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.

bors · 2025-01-15T23:24:37Z

☀️ Try build successful - checks-actions
Build commit: da49290 (da4929024a7261062885cb3209c9d7fdc919d233)

joshtriplett · 2025-01-16T05:52:38Z

@craterbot check

craterbot · 2025-01-16T05:52:57Z

👌 Experiment pr-135536 created and queued.
🤖 Automatically detected try build da49290
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-01-17T05:13:39Z

🚧 Experiment pr-135536 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2025-01-18T01:58:32Z

🎉 Experiment pr-135536 is completed!
📊 45 regressed and 3 fixed (569212 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

joshtriplett · 2025-01-24T07:17:39Z

Rebasing and reblessing.

bors · 2025-01-29T02:59:19Z

☔ The latest upstream changes (presumably #136209) made this pull request unmergeable. Please resolve the merge conflicts.

joshtriplett · 2025-01-29T12:33:09Z

I'm going to wait to fix further conflicts on this PR until there are more checkboxes on the FCP.

rfcbot · 2025-01-29T16:57:25Z

🔔 This is now entering its final comment period, as per the review above. 🔔

library/alloc/src/string.rs

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 · 2025-02-01T10:58:50Z

@ibraheemdev Does this now look good, other than waiting on FCP to complete?

ibraheemdev · 2025-02-01T17:30:11Z

Yes the implementation looks good. r=me after the FCP.

riking · 2025-02-08T00:23:24Z

library/alloc/src/string.rs

 impl_eq! { String, str }
 impl_eq! { String, &'a str }
+impl_eq! { String, &String }
+impl_eq! { &String, str }


These two impls end up with incorrect stability attributes, which means Clippy msrv autodetection can't be added later.

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 · 2025-02-08T16:58:03Z

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.

flip1995 · 2025-02-14T15:40:34Z

src/tools/clippy/tests/ui/iter_overeager_cloned.stderr

   = 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


This lint shouldn't trigger in the test file. Please fix the two new occurrences by removing the ref.

Alright, will fix.

bors · 2025-03-02T02:15:25Z

☔ The latest upstream changes (presumably #137752) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot mentioned this pull request Jan 15, 2025

Tracking issue for release notes of #135536: Add more impls of PartialEq and PartialOrd for strings #135537

Open

3 tasks

rustbot assigned ibraheemdev Jan 15, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2025

This comment has been minimized.

Sign in to view

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2025

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 18, 2025

This comment has been minimized.

Sign in to view

joshtriplett force-pushed the str-impls branch from f7320e2 to 9485326 Compare January 18, 2025 12:59

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2025

joshtriplett force-pushed the str-impls branch from 63c622d to 1d12da8 Compare January 24, 2025 08:27

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 24, 2025

joshtriplett removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 24, 2025

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 29, 2025

ibraheemdev reviewed Jan 31, 2025

View reviewed changes

library/alloc/src/string.rs Outdated Show resolved Hide resolved

joshtriplett force-pushed the str-impls branch from 1d12da8 to 7ac37c9 Compare January 31, 2025 11:34

rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 31, 2025

joshtriplett added 4 commits January 31, 2025 12:39

clippy: In cmp_owned, don't use Deref via * if non-Deref impl exists

b6ac0dc

This avoids making a suggestion to write `*x` if `x` would also work.

Add PartialEq and PartialOrd impls for comparing String and &String

5f3c8ef

This makes such comparisons more robust against the availability of other impls.

joshtriplett force-pushed the str-impls branch from 7ac37c9 to 5f3c8ef Compare January 31, 2025 11:39

riking reviewed Feb 8, 2025

View reviewed changes

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 8, 2025

rfcbot added the to-announce Announce this issue on triage meeting label Feb 8, 2025

ibraheemdev added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2025

apiraino removed the to-announce Announce this issue on triage meeting label Feb 13, 2025

flip1995 reviewed Feb 14, 2025

View reviewed changes

djc mentioned this pull request Feb 28, 2025

smartstring breaks std::string::String? bodil/smartstring#7

Open

Add more impls of PartialEq and PartialOrd for strings #135536

Are you sure you want to change the base?

Add more impls of PartialEq and PartialOrd for strings #135536

Uh oh!

Conversation

joshtriplett commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jan 15, 2025

Uh oh!

joshtriplett commented Jan 15, 2025

Uh oh!

bors commented Jan 15, 2025

Uh oh!

This comment has been minimized.

joshtriplett commented Jan 15, 2025

Uh oh!

rustbot commented Jan 15, 2025

Uh oh!

joshtriplett commented Jan 15, 2025

Uh oh!

joshtriplett commented Jan 15, 2025

Uh oh!

bors commented Jan 15, 2025

Uh oh!

bors commented Jan 15, 2025

Uh oh!

joshtriplett commented Jan 16, 2025

Uh oh!

craterbot commented Jan 16, 2025

Uh oh!

craterbot commented Jan 17, 2025

Uh oh!

craterbot commented Jan 18, 2025

Uh oh!

This comment has been minimized.

joshtriplett commented Jan 24, 2025

Uh oh!

bors commented Jan 29, 2025

Uh oh!

joshtriplett commented Jan 29, 2025

Uh oh!

rfcbot commented Jan 29, 2025

Uh oh!

Uh oh!

joshtriplett commented Feb 1, 2025

Uh oh!

ibraheemdev commented Feb 1, 2025

Uh oh!

riking Feb 8, 2025

Choose a reason for hiding this comment

Uh oh!

joshtriplett Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

rfcbot commented Feb 8, 2025

Uh oh!

flip1995 Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

joshtriplett Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

bors commented Mar 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

joshtriplett commented Jan 15, 2025 •

edited

Loading