Workaround for compilation error due to rkyv#434. #14863
Conversation
When datafusion is used in a workspace that enables the `rkyv-64` feature in the `chrono` crate, this triggered a Rust compilation error: ``` error[E0277]: can't compare `Option<&std::string::String>` with `Option<&mut std::string::String>`. ``` The root cause of the error is incorrect type unification in the Rust compiler, as explained in rkyv/rkyv#434. The workaround pushes the compiler in the right direction by converting the mutable reference to an immutable one manually. Signed-off-by: Leonid Ryzhyk <leonid@feldera.com>
| // this triggers a Rust compilation error: | ||
| // error[E0277]: can't compare `Option<&std::string::String>` with `Option<&mut std::string::String>`. |
There was a problem hiding this comment.
I tried to reproduce the error, but didn't:
fn main() {
let mut s = String::from("hello");
let ss = String::from("hello");
let option_ref: Option<&String> = Some(&ss);
let mut_ref = &mut s;
if option_ref == Some(mut_ref) //Option<&mut String> {
{
println!("They are equal!");
}
}Maybe I didn't notice anything?
There was a problem hiding this comment.
is it compiler dependent? I was not able to reproduce either on latest compiler
UPD: i was able to reproduce it when enable feature in Cargo.toml
chrono = { version = "0.4.38", default-features = false, features = ["rkyv-64"] }
There was a problem hiding this comment.
@xudong963 , please modify your example like this to reproduce:
- Add chrono as a dependency to Cargo.toml:
[dependencies]
chrono = { version = "0.4.38", default-features = false, features = ["rkyv-64"] }- Add chrono import to main.rs:
use chrono::*;|
I dont have a strong opinion here, we do not use |
The problem is Rust's feature unification: if this crate is used in a workspace that has at least one other crate that enables this chrono feature, it affects |
Indeed! It's triggered by something in |
|
as long as it is documented I'm fine, sort of surprising why clippy allows extra dereferencing. |
Yeah, me too -- but given the overhead of this workaround seems to have a low maintenance burden I think it is ok to include with DataFusion Thanks for the contribution @ryzhyk |
|
@alamb , @comphead , @xudong963 , thanks for the quick turnaround! |
Which issue does this PR close?
Rationale for this change
When datafusion is used in a workspace that enables the
rkyv-64feature in thechronocrate, this triggered a Rust compilation error:The root cause of the error is incorrect type unification in the Rust compiler, as explained in rkyv/rkyv#434.
What changes are included in this PR?
The workaround pushes the compiler in the right direction by converting the mutable reference to an immutable one manually.
Are these changes tested?
I don't think this requires additional tests.
Are there any user-facing changes?
no