Skip to content
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

Additional stringy PartialEq implementations for Box<str> #129852

Closed
wants to merge 1 commit into from

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Sep 1, 2024

Box<str> lacks PartialEq implementations for comparisons with other string types. It can't even be compared to a string literal:

error[E0277]: can't compare `&str` with `Box<str>`
 --> src/lib.rs:6:21
  |
6 |     assert!("boxed" == boxed);
  |                     ^^ no implementation for `&str == Box<str>`
  |
  = help: the trait `PartialEq<Box<str>>` is not implemented for `&str`
help: consider dereferencing both sides of the expression
  |
6 |     assert!(*"boxed" == *boxed);

This PR adds the same PartialEq implementations to Box<str> as Cow<str> has, making it comparable with str and String in many more cases.

This is a risky change, as it may affect type inference. AFAIK PartialEq implementations can't be marked as unstable, so this is insta-stable.

Forum discussion.

This makes Box<str> comparable with str and String in more cases, on par with Cow<str>
@rustbot
Copy link
Collaborator

rustbot commented Sep 1, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 1, 2024
@jieyouxu jieyouxu added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 1, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Sep 1, 2024

This will definitely need a crater run

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-17 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:20d3b4d4a2629cbf7865cdbf92fe47512a7c96658c24253a045ff38e8075cd7fb37ca6fcadfa6e6d093333943ad24f6fc4f163ec5b74fd940de9d5bb03eb4d3b:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-17]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-17', '--enable-llvm-link-shared', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-17/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.thin-lto-import-instr-limit := 10
configure: change-id            := 99999999
---

---- [ui] tests/ui/inference/issue-72616.rs stdout ----
diff of stderr:

9    = note: multiple `impl`s satisfying `String: PartialEq<_>` found in the `alloc` crate:
10            - impl PartialEq for String;
11            - impl<'a, 'b> PartialEq<&'a str> for String;
+            - impl<'a, 'b> PartialEq<Box<str>> for String;
12            - impl<'a, 'b> PartialEq<Cow<'a, str>> for String;
13            - impl<'a, 'b> PartialEq<str> for String;


The actual stderr differed from the expected stderr.
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference/issue-72616/issue-72616.stderr
Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference/issue-72616/issue-72616.stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args inference/issue-72616.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/inference/issue-72616.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference/issue-72616" "-A" "unused" "-A" "internal_features" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/inference/issue-72616/auxiliary"
--- stderr -------------------------------
error[E0283]: type annotations needed
##[error]  --> /checkout/tests/ui/inference/issue-72616.rs:22:37
   |
   |
LL |         if String::from("a") == "a".try_into().unwrap() {}
   |                              |
   |                              type must be known at this point
   |
   |
   = note: multiple `impl`s satisfying `String: PartialEq<_>` found in the `alloc` crate:
           - impl PartialEq for String;
           - impl<'a, 'b> PartialEq<&'a str> for String;
           - impl<'a, 'b> PartialEq<Box<str>> for String;
           - impl<'a, 'b> PartialEq<Cow<'a, str>> for String;
           - impl<'a, 'b> PartialEq<str> for String;
   |
   |
LL |         if String::from("a") == <&str as TryInto<T>>::try_into("a").unwrap() {}
   |                                 +++++++++++++++++++++++++++++++   ~
error[E0283]: type annotations needed
##[error]  --> /checkout/tests/ui/inference/issue-72616.rs:22:37
   |
   |
LL |         if String::from("a") == "a".try_into().unwrap() {}
   |
   |
   = note: multiple `impl`s satisfying `_: TryFrom<&str>` found in the following crates: `core`, `std`:
           - impl TryFrom<&str> for std::sys_common::net::LookupHost;
           - impl<T, U> TryFrom<U> for T
             where U: Into<T>;
   = note: required for `&str` to implement `TryInto<_>`
   |
   |
LL |         if String::from("a") == <&str as TryInto<T>>::try_into("a").unwrap() {}
   |                                 +++++++++++++++++++++++++++++++   ~
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0283`.
------------------------------------------

@kornelski
Copy link
Contributor Author

kornelski commented Sep 1, 2024

if String::from("a") == "a".try_into().unwrap()
note: multiple impls satisfying String: PartialEq<_> found in the alloc crate:

I think the useless_conversions lint needs to be adopted first to make crates less sensitive to type inference: #129249

@kornelski kornelski closed this Sep 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2024
…,dtolnay

Add str.as_str() for easy Deref to string slices

Working with `Box<str>` is cumbersome, because in places like `iter.filter()` it can end up being `&Box<str>` or even `&&Box<str>`, and such type doesn't always get auto-dereferenced as expected.

Dereferencing such box to `&str` requires ugly syntax like `&**boxed_str` or `&***boxed_str`, with the exact amount of `*`s.

`Box<str>` is [not easily comparable with other string types](rust-lang#129852) via `PartialEq`. `Box<str>` won't work for lookups in types like `HashSet<String>`, because `Borrow<String>` won't take types like `&Box<str>`. OTOH `set.contains(s.as_str())` works nicely regardless of levels of indirection.

`String` has a simple solution for this: the `as_str()` method, and `Box<str>` should too.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 23, 2024
…,dtolnay

Add str.as_str() for easy Deref to string slices

Working with `Box<str>` is cumbersome, because in places like `iter.filter()` it can end up being `&Box<str>` or even `&&Box<str>`, and such type doesn't always get auto-dereferenced as expected.

Dereferencing such box to `&str` requires ugly syntax like `&**boxed_str` or `&***boxed_str`, with the exact amount of `*`s.

`Box<str>` is [not easily comparable with other string types](rust-lang#129852) via `PartialEq`. `Box<str>` won't work for lookups in types like `HashSet<String>`, because `Borrow<String>` won't take types like `&Box<str>`. OTOH `set.contains(s.as_str())` works nicely regardless of levels of indirection.

`String` has a simple solution for this: the `as_str()` method, and `Box<str>` should too.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 23, 2024
…,dtolnay

Add str.as_str() for easy Deref to string slices

Working with `Box<str>` is cumbersome, because in places like `iter.filter()` it can end up being `&Box<str>` or even `&&Box<str>`, and such type doesn't always get auto-dereferenced as expected.

Dereferencing such box to `&str` requires ugly syntax like `&**boxed_str` or `&***boxed_str`, with the exact amount of `*`s.

`Box<str>` is [not easily comparable with other string types](rust-lang#129852) via `PartialEq`. `Box<str>` won't work for lookups in types like `HashSet<String>`, because `Borrow<String>` won't take types like `&Box<str>`. OTOH `set.contains(s.as_str())` works nicely regardless of levels of indirection.

`String` has a simple solution for this: the `as_str()` method, and `Box<str>` should too.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2024
Rollup merge of rust-lang#129550 - kornelski:boxasstr, r=joshtriplett,dtolnay

Add str.as_str() for easy Deref to string slices

Working with `Box<str>` is cumbersome, because in places like `iter.filter()` it can end up being `&Box<str>` or even `&&Box<str>`, and such type doesn't always get auto-dereferenced as expected.

Dereferencing such box to `&str` requires ugly syntax like `&**boxed_str` or `&***boxed_str`, with the exact amount of `*`s.

`Box<str>` is [not easily comparable with other string types](rust-lang#129852) via `PartialEq`. `Box<str>` won't work for lookups in types like `HashSet<String>`, because `Borrow<String>` won't take types like `&Box<str>`. OTOH `set.contains(s.as_str())` works nicely regardless of levels of indirection.

`String` has a simple solution for this: the `as_str()` method, and `Box<str>` should too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants