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

ouroboros is in conflict with noalias #33

Closed
ibraheemdev opened this issue Jun 26, 2021 · 4 comments
Closed

ouroboros is in conflict with noalias #33

ibraheemdev opened this issue Jun 26, 2021 · 4 comments

Comments

@ibraheemdev
Copy link

#[self_referencing]
struct MyStruct {
    float_data: Box<f32>,
    #[borrows(mut float_data)]
    float_reference: &'this mut f32,
}

See Kimundi/owning-ref-rs#49

@someguynamedjosh
Copy link
Owner

I don't think this has the same problem. When a field is mutably referenced, the functions generated prevent accessing the field directly. In your example, the data contained in float_data can only ever be accessed through float_reference, and never through float_dat directly.

@Kestrer
Copy link
Contributor

Kestrer commented Jun 26, 2021

Even if the field is not accessible in the public API, it is still considered mutably borrowed if the outer MyStruct is mutably borrowed, because float_data is stored inline and Box is noalias. You can recreate the exact example shown in the owning-ref issue using Ouroboros:

#[self_referencing]
struct MyStruct {
    float_data: Box<Cell<f32>>,
    #[borrows(float_data)]
    float_reference: &'this Cell<f32>,
}

fn helper(s: MyStruct) -> f32 {
    s.borrow_float_data().set(10);
    s.borrow_float_reference().set(20);
    s.borrow_float_data().get()
}

fn main() {
    let res = helper(MyStruct::new(Box::new(Cell::new(0)), |r| r));
    assert_eq!(res, 20);
}

I do not know if this particular example causes miscompilations (you should check on Nightly), but it is certainly UB, because the compiler thinks that:

  • Because it has unique access to s,
  • It must also have unique access to float_data,
  • And therefore must have unique access to the Cell<f32>.

But that assumption broken by this crate.

In terms of solutions, you could require the field which is borrowed from to implement AliasableDeref. That is a significant reduction in ergonomics but is the only option that I really know of, besides AliasableBoxing all the borrowed-from fields.

@someguynamedjosh
Copy link
Owner

@Kestrer thanks for your explanation, I think I understand the problem now. I prefer the solution of wrapping borrowed fields in AliasableBoxes. Thinking about it, this could also help streamline the library. Right now any borrowed field has to implement StableDeref. Wrapping every borrowed field automatically could allow semantics like this:

#[self_referencing]
struct Example {
    data: i32,
    #[borrows(data)]
    dref: &'this i32,
}

It would also resolve the issue that lead to the creation of chain_hack, so doing the following wouldn't require any special syntax or considerations:

#[self_referencing]
struct Example {
    data: String,
    #[borrows(data)]
    parser: Parser<'this>,
    #[borrows(parser)]
    parsed: Parsed<'this. 'this>,
}

@someguynamedjosh
Copy link
Owner

Version 0.10.0 is now published and uses aliasable boxes for all borrowed fields.

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

No branches or pull requests

3 participants