Skip to content

regression: cant compare str with &str #130575

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

Closed
BoxyUwU opened this issue Sep 19, 2024 · 3 comments
Closed

regression: cant compare str with &str #130575

BoxyUwU opened this issue Sep 19, 2024 · 3 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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.

Comments

@BoxyUwU
Copy link
Member

BoxyUwU commented Sep 19, 2024

[INFO] [stdout] error[E0277]: can't compare `str` with `&str`
[INFO] [stdout]   --> src/lib.rs:29:40
[INFO] [stdout]    |
[INFO] [stdout] 29 |         assert!(input.is_none_or(|x| x == &"test"));
[INFO] [stdout]    |                                        ^^ no implementation for `str == &str`
[INFO] [stdout]    |
[INFO] [stdout]    = help: the trait `PartialEq<&str>` is not implemented for `str`, which is required by `&str: PartialEq<&&str>`
[INFO] [stdout]    = note: required for `&str` to implement `PartialEq<&&str>

I think this was caused by stabilizing is_none_or?

note: if this breakage was accepted by the relevant team then this issue can be closed.

@BoxyUwU BoxyUwU added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 19, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 19, 2024
@BoxyUwU BoxyUwU removed the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Sep 19, 2024
@saethlin saethlin added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 19, 2024
@steffahn
Copy link
Member

steffahn commented Sep 24, 2024

Indeed, this is caused by stabilization of is_none_or.

Both crates (moar_options, rs-std-ext) are crates that provide an extension trait for Option with a is_none_or method (1) (2), and in either case, that method works with &self and passes &T to the callback; unlike the stabilized Option::is_none_or which works with self and passes T to the callback.

// difference in signature, on `Option<T>`

// extension traits from broken crates
fn is_none_or(&self, f: impl FnOnce(&T) -> bool) -> bool

// standard library, stabilized
fn is_none_or(self, f: impl FnOnce(T) -> bool) -> bool

Stabilization of the inherent method Option::is_none_or makes the inherent method take precedence over the extension traits, causing the error in those crates.

The crates both still work on beta, just their tests fail, and most users of the crates will likely fail if they make use of is_none_or.

Since 1.81, users of the either crate that use is_none_or are already receiving the following warning.

warning: a method with this name may be added to the standard library in the future
 --> src/main.rs:5:14
  |
… |     …………………).is_none_or(|…| {
  |              ^^^^^^^^^^
  |
  = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
  = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
  = help: call with fully qualified syntax `is_none_or(...)` to keep using the current method
  = note: `#[warn(unstable_name_collisions)]` on by default

However, since unstable is_none_or was not present in 1.80 or earlier yet, this means there’s only a single release cycle that displays this warning.

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 25, 2024
@m-ou-se
Copy link
Member

m-ou-se commented Sep 25, 2024

@mztikk @Embers-of-the-Fire Heads up: the stablization of is_none_or in std will affect your crates. (The new is_none_or in std will pass the value by value rather than by reference.)

@m-ou-se
Copy link
Member

m-ou-se commented Sep 25, 2024

Discussed in libs meeting; we consider this acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. 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

No branches or pull requests

6 participants