search

raviqqe / melior

The rustic MLIR bindings in Rust
https://raviqqe.github.io/melior/melior/
Apache License 2.0
326 stars 40 forks source link

Safety of `OperationRef::to_ref()` #522

Open tanmay-bakshi opened 7 months ago

tanmay-bakshi commented 7 months ago

We hit some interesting lifetime quirks when trying to loop through the operations in a block, so I was digging through the code for OperationRef, and found this:

https://github.com/raviqqe/melior/blob/0b2a9bd033a472c4cf750a13802ef6ae2c30c758/melior/src/ir/operation.rs#L375

My question is why this function is marked unsafe. In theory, isn't this safe because the &'a Operation<'c> is still bounded by the borrow checker to be live as long as the OperationRef<'c, 'a> would?

It makes sense that the Deref trait doesn't allow us to express that reference lifetime, but doesn't that make the Deref lifetime (the "safe" one) actually incorrect and unnecessarily strict/short? As far as I can tell, the Operation should be bounded in lifetime by the lifetime of the value that the OperationRef was itself accessed from (in our case, a Block), and that's better described by &'a Operation<'c>.

raviqqe commented 7 months ago

In theory, isn't this safe because the &'a Operation<'c> is still bounded by the borrow checker to be live as long as the OperationRef<'c, 'a> would?

It makes sense that the Deref trait doesn't allow us to express that reference lifetime, but doesn't that make the Deref lifetime (the "safe" one) actually incorrect and unnecessarily strict/short?

In Rust, references correspond to pointers. So when we mark it safe, we can drop the OperationRef safely (wrongly,) which invalidates references to it.

As far as I can tell, the Operation should be bounded in lifetime by the lifetime of the value that the OperationRef was itself accessed from (in our case, a Block), and that's better described by &'a Operation<'c>.

We can't do this because the C API doesn't allow this... MlirOperation's internal representation in the C API is void*. So we can assume it to be &'a Operation<'c>. But then that might break the entire design of Operation and OperationRef in the Rust API in the future when they change the representation.

https://github.com/llvm/llvm-project/blob/a309c07ad3fc4a9b0bf41a4e66e812b54338aa02/mlir/include/mlir-c/IR.h#L45

Moxinilian commented 6 months ago

I think it would be nice if OperationRef<'c, 'a> had more documentation on what the lifetimes model. I work with MLIR in C++ a lot yet I am not super sure what they represent. I assume 'c refers to the lifetime of the MLIRContext, but I'm not sure what 'a is about.