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

Dereference datacenter of type &&str when comparing with &str #838

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

oeb25
Copy link
Contributor

@oeb25 oeb25 commented Oct 16, 2023

I encountered a compile error when updating from 0.7.0 to 0.10.1:

error[E0277]: can't compare `Option<&str>` with `Option<&&str>`
   --> /Users/oeb25/.cargo/registry/src/index.crates.io-6f17d22bba15001f/scylla-0.10.1/src/transport/locator/mod.rs:278:59
    |
278 |                 .filter(|node| node.datacenter.as_deref() == Some(datacenter))
    |                                                           ^^ no implementation for `Option<&str> == Option<&&str>`
    |
    = help: the trait `PartialEq<Option<&&str>>` is not implemented for `Option<&str>`
    = help: the following other types implement trait `PartialEq<Rhs>`:
              <Option<Box<U>> as PartialEq<rkyv::niche::option_box::ArchivedOptionBox<T>>>
              <Option<T> as PartialEq>
              <Option<U> as PartialEq<rkyv::option::ArchivedOption<T>>>
Two more similar errors
error[E0277]: can't compare `Option<&str>` with `Option<&&str>`
   --> /Users/oeb25/.cargo/registry/src/index.crates.io-6f17d22bba15001f/scylla-0.10.1/src/transport/locator/mod.rs:320:63
    |
320 |                     .filter(|node| node.datacenter.as_deref() == Some(datacenter))
    |                                                               ^^ no implementation for `Option<&str> == Option<&&str>`
    |
    = help: the trait `PartialEq<Option<&&str>>` is not implemented for `Option<&str>`
    = help: the following other types implement trait `PartialEq<Rhs>`:
              <Option<Box<U>> as PartialEq<rkyv::niche::option_box::ArchivedOptionBox<T>>>
              <Option<T> as PartialEq>
              <Option<U> as PartialEq<rkyv::option::ArchivedOption<T>>>

error[E0277]: can't compare `Option<&str>` with `Option<&mut &str>`
   --> /Users/oeb25/.cargo/registry/src/index.crates.io-6f17d22bba15001f/scylla-0.10.1/src/transport/locator/mod.rs:469:54
    |
469 |                     if replica.datacenter.as_deref() == Some(datacenter) {
    |                                                      ^^ no implementation for `Option<&str> == Option<&mut &str>`
    |
    = help: the trait `PartialEq<Option<&mut &str>>` is not implemented for `Option<&str>`
    = help: the following other types implement trait `PartialEq<Rhs>`:
              <Option<Box<U>> as PartialEq<rkyv::niche::option_box::ArchivedOptionBox<T>>>
              <Option<T> as PartialEq>
              <Option<U> as PartialEq<rkyv::option::ArchivedOption<T>>>

When building this repo by itself the error does not occur, which I do not understand, and I'd love to know why if anybody can figure it out. Here's the commit to reproduce.

Introducing these dereferences should not change anything for already working code, but should hopefully fix the issue encountered above.


Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@Lorak-mmk
Copy link
Collaborator

This is interesting. I managed to minimize the problem. New project with the following Cargo.toml works:

[package]
edition = "2021"
name = "member"
version = "0.1.0"

[[bin]]
name = "stract"
path = "src/main.rs"

[dependencies]
hashbrown = {version = "0.14.0"}
rkyv = {version = "0.7.42"}
scylla = "0.10.1"

But this one does not:

[package]
edition = "2021"
name = "member"
version = "0.1.0"

[[bin]]
name = "stract"
path = "src/main.rs"

[dependencies]
hashbrown = {version = "0.14.0", features = ["rkyv"]}
rkyv = {version = "0.7.42"}
scylla = "0.10.1"

The only difference in Cargo.lock is addition of rkyv to hashbrown dependencies:

diff -u10 Cargo.lock.bad Cargo.lock.good
--- Cargo.lock.bad	2023-10-16 18:24:01.167016053 +0200
+++ Cargo.lock.good	2023-10-16 18:24:36.327845711 +0200
@@ -347,21 +347,20 @@
 ]
 
 [[package]]
 name = "hashbrown"
 version = "0.14.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "7dfda62a12f55daeae5015f81b0baea145391cb4520f86c248fc615d72640d12"
 dependencies = [
  "ahash 0.8.3",
  "allocator-api2",
- "rkyv",
 ]
 
 [[package]]
 name = "heck"
 version = "0.3.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "6d621efb26863f0e9924c6ac577e8275e5e6b77455db64ffa6c65c904e9e132c"
 dependencies = [
  "unicode-segmentation",
 ]

I'm not sure yet why that causes any difference when compiling our driver. @piodul wdyt?

@piodul
Copy link
Collaborator

piodul commented Oct 17, 2023

AFAIK the standard library uses the hashbrown crate internally to implement the standard hashmap and hashset. Perhaps enabling the rkyv feature affects the std crate? The code that doesn't compile doesn't seem to directly interact with std::collections::HashMap or std::collections::HashSet, though.

@oeb25
Copy link
Contributor Author

oeb25 commented Oct 17, 2023

Apologizes for a lot of details, feel free to skip to the end.

Did some more digging and tried to replicate the impl's resulting from enabling the rkyv feature in hashbrown but could not reproduce the error. (playground)

Then tried multiple permutations of comparing Option<&str> with Option<&&str>, and found something interesting. (playground)

// Ok
fn test_1(a: Option<String>, b: &&str) -> bool {
    a.as_deref() == Some(b)
}
// Errors
fn test_2(a: Option<String>, b: &&str) -> bool {
    let c = Some(b);
    a.as_deref() == c
}
// Errors
fn test_3(a: Option<String>, b: &&str) -> bool {
    let c: Option<&&str> = Some(b);
    a.as_deref() == c
}
// Ok
fn test_4(a: Option<String>, b: &&str) -> bool {
    let c: Option<&str> = Some(b);
    a.as_deref() == c
}

This seems to indicate that the inference for performing the dereference in Some(b) requires the context of PartialEq<Option<&str>>. test_2 shows moving the construction of Some(b) out of the == loses inference of &str. test_4 shows we can introduce the dereference again by explicitly stating the target type.

I then tried adding hashbrown with the rkyv feature, but that doesn't error by itself, only when I add scylla do I get the error inside of scylla again. However, if I set scylla = { git = "https://github.com/oeb25/scylla-rust-driver", branch = "double-ref-str-deref" }, then I get none of the original errors, and surprisingly test_1 doesn't error either.

Turns out you have to trigger compilation of hashbrown for the error to appear, so adding let _ = hashbrown::HashSet::<()>::default(); anywhere to the code makes test_1 error.

I unsuccessfully tried to reproduce it in a single crate without deps, but failed. (playground)

However, I did manage to reduce the error to just having rkyv as a dependency with the following code:

use rkyv::AlignedVec;
fn does_not_compile() {
    assert!(Some("") == Some(&""));
}

I opened a repo for this and found rkyv/rkyv#328 which might be related.


In any case, I think it is clear that this is not an issue stemming from scylla, but it would be nice to have the "workaround" just to prevent such issues from occurring for users of rkyv.

@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Oct 17, 2023

Thanks for the effort. I think we can merge this workaround, but I'd like an issue to be opened in rkyv first, and I'm not sure the one you linked is the same problem. Do you think you could open a new one?
Also, I'm surprised that it's possible for rkyv to affect compilation of other crates (that don't use rkyv or any of it's types) this way. Maybe I'm just not seeing something, but to me it looks like a possible bug in rustc?

@oeb25
Copy link
Contributor Author

oeb25 commented Oct 17, 2023

Yeah I opened an issue on rkyv! rkyv/rkyv#434

And also managed to track down which impl PartialEq caused the error. This is a minimized example:

struct ArchivedOption<T>(T);

impl<T: PartialEq<U>, U> PartialEq<ArchivedOption<T>> for Option<U> {
    fn eq(&self, _: &ArchivedOption<T>) -> bool {
        todo!()
    }
}

fn test() {
    Some("") == Some(&"");
}

You can even skip one of the generics:

impl<T: PartialEq> PartialEq<ArchivedOption<T>> for Option<T> {
    fn eq(&self, _other: &ArchivedOption<T>) -> bool {
        todo!()
    }
}

@Lorak-mmk Lorak-mmk merged commit 3a26efc into scylladb:main Oct 17, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants