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

Solving variance with Opaque and OpaqueMut #225

Open
QnnOkabayashi opened this issue Aug 23, 2022 · 7 comments
Open

Solving variance with Opaque and OpaqueMut #225

QnnOkabayashi opened this issue Aug 23, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@QnnOkabayashi
Copy link
Collaborator

Problem

With support for lifetimes, Diplomat is now vulnerable to dangling references. This can be caused when we modify the contents of an opaque that we have a mutable reference to, since we have no way of ensuring that something else in FFI land isn't holding a reference into it. The classic example of this is holding a reference into a Vec, and then clearing it in another function call, resulting in a dangling reference.

Solution

When working with FFI, we don't have access to the full scope that types are passed around in. However, we do have full access to the declarations of types. Since only opaques can be passed by reference, we can mark them either as "mutable" or "immutable" in their declarations. Something like #[diplomat::opaque] vs #[diplomat::opaque_mut]?

Here are the key differences I imagine:

  1. Only mutable opaques can be passed as mutable references in arguments, but both can be passed immutably.
  2. You can only "return" borrows from immutable opaques, where "return" means either in the return position or in "out" parameters.
    • This also means that when a mutable opaque is passed to a method as a ref (cannot be passed by value), regardless of whether the reference is mutable or immutable, the lifetime of the reference and any lifetimes that the opaque is generic over must not appear anywhere else in the method signature. This ensures that nothing borrows from it. This would also mean we would have to check that no lifetimes can coerce into them. The easiest solution would be to enforce anonymous lifetimes, and check that any output lifetime elision isn't tied to it. I'm working on elision inference right now and this shouldn't be too difficult to add later.

Pros

  • This seems like the "Rusty" solution, and allows for maximum flexibility.

Cons

  • Many changes to the HIR, particularly splitting opaques into Opaque and OpaqueMut.

Alternatives

Just ban mutability altogether. The downside of this is that updating non-node based data structures becomes impractical, and using node based data structures in Rust is hell.

@Manishearth
Copy link
Contributor

Oh, this is a neat idea. I like it!

@Manishearth
Copy link
Contributor

Actually, would we have to split Opaque and OpaqueMut as types? We could start off with a bool that's checked and later splti types if we really need (I'm wary of having a giant pile of HIR types that are almost the same but not quite)

@Manishearth
Copy link
Contributor

An annoying thing about those rules is that iterators that return opaque references won't work. That seems useful to have.

@QnnOkabayashi
Copy link
Collaborator Author

An annoying thing about those rules is that iterators that return opaque references won't work. That seems useful to have.

It works as long as the iterator is a non-mut opaque. Do you have examples in mind where the iterator would need to be opaque_mut?

@Manishearth
Copy link
Contributor

Iterators have to be mut though :)

@QnnOkabayashi
Copy link
Collaborator Author

oh

@QnnOkabayashi
Copy link
Collaborator Author

QnnOkabayashi commented Aug 23, 2022

We can slightly relax the above idea by ensuring that the lifetime of a reference to a mutable opaque is unique, while the generics don't have to be unique. This solves the iterator problem, since they can still hold immutable references to data via a lifetime which is different from the lifetime that the iterator is borrowed for when returning the next object.

fn next<'a, 'unique>(iter: &'unique mut Iter<'a>) -> &'a MyOpaque { .. }

Then the problem is, where does the iterator borrow from? It has to be some immutable collection. If you have some list that shrinks and grows and you decide you want to iterate over it, this would require cloning all the contents into an immutable "snapshot", which could then iterate over things.

A potentially more efficient implementation might be to have the mutable collection be Option<Collection<T>>, and when you "freeze" it, just Option::takes the collection and put it into the immutable collection. That way, if the mutable collection is used again, you can see the None and err. Or you could just Arc all the contents.

Also I'm convinced there's a better (and still safe) solution, but this is what I've got now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants