Created attachment 145205 [details] example code ready to run cargo build SUMMARY By storing the Emitter in a RefCell one can create a mutable reference to the struct the Emitter is associated with while an arbitrary number of immutable references to the same struct may exist. If I understand the Rustonomicon correctly, such a transmutation from & to &mut violates Rust's aliasing rules and is undefined behaviour. Let's assume a minimal example (also attached in a build-ready form): ------------------- bindings.json { "cppFile": "src/Bindings.cpp", "rust": { "dir": "rust", "interfaceModule": "interface", "implementationModule": "implementation" }, "objects": { "SomeQtObject" : { "type" : "Object", "properties" : { "some_property" : { "type" : "bool", "write": true } } } } } ------------------- implementation.rs: use interface::*; pub struct SomeQtObject { emit : SomeQtObjectEmitter, emit2: std::cell::RefCell<SomeQtObjectEmitter>, some_property: bool, } impl SomeQtObjectTrait for SomeQtObject { fn new(mut emit: SomeQtObjectEmitter) -> SomeQtObject { let emit2 = emit.clone(); SomeQtObject { emit, emit2 : std::cell::RefCell::new(emit2), some_property: false, } } fn emit(&mut self) -> &mut SomeQtObjectEmitter { &mut self.emit } fn some_property(&self) -> bool { self.emit2.borrow_mut().some_property_changed(); self.some_property } fn set_some_property(&mut self, value: bool) { self.some_property = value; self.emit.some_property_changed(); } } ------------------- OBSERVED RESULT The above (and attached) example compiles fine in Rust, even though it contains an emit() statement in SomeQtObject::some_property(&self). If on the QML (or C++) side a call to SomeQtObject::set_some_property(&mut self, value: bool) is connected to this signal, undefined behaviour is invoked, as (implicitly) &self gets transmuted into &mut self. EXPECTED RESULT The safe API of the rust-qt-binding-generator should ideally make such a construct impossible. ADDITIONAL INFORMATION This might be related to https://bugs.kde.org/show_bug.cgi?id=406178, but I think it's a different issue. I've been thinking a bit on how to possibly solve this, but it's a non-trivial issue that might not be solvable without API or behaviour changes... The best option I see would be to internally create QueuedConnections on the QObject that delay the update notifications and therefore ensure that there will be no Rust -> C++ -> Rust call stacks on the same object. (That would also fix #406178.)
I agree with the analysis. This is a fundamental problem that I did not realize before. One can get rid of the undefined behavior by changing all '&mut self' to '&self' in the interface and leave the problem for the implementer. (This is also what kdebuac.rhn describes in https://bugs.kde.org/show_bug.cgi?id=406178 ) They might then use a Mutex. That would then lead to a deadlock instead of undefined behavior. The idea of a QueuedConnection would not solve the problem general problem. One would need to guarantee that any code that calls into C++ does not call back in Rust. The only safe way is to have only immutable references. The read-only experimental branch might be the solution: https://gitlab.com/rhn/rust-qt-bindings-generator/-/commits/simplify_ro
I was not aware that there is already work ongoing in that regards. Is this bug report then really a duplicate of 406178? Using internal mutability with a Mutex indeed sounds like the most viable approach. It has the drawback of causing runtime errors, but they should be reproducible, so easy to find/fix. I'll grab that branch tomorrow and have a closer look. I'm also considering just sticking to current master for now, as I haven't had any actual issues in my current project, and just noticed that aliasing problem while writing a wrapper for an Emitter. (I'm getting &mut self -> &self in the call stacks, but I don't see how that could cause troubles, with no mutation happening on the aliased memory - but I might misunderstand something here.)
Sorry, this is not really relevant to the bug, but I just have to correct myself here. I was too tired yesterday to realize that also the &mut self -> &self situation can cause issues. Actually #406178 describes such a call stack. With Rust's aliasing rules the compiler can assume that the call to C++ (which does not take any references into the memory pointed to by &mut self) is independent of &mut self, so it need not guarantee that the memory referenced by &mut self is in a consistent (or up-to-date) state when it triggers the signal.
I've had a look at the linked branch. That does not only contain the fix for the mutability/aliasing issue, but functionality (replaceable objects) which I don't want/need for my use case. I've started a new branch based off current master, where I'm planning to make the following changes: o) Make every &mut self be &self instead. o) Remove RustByFunction. In an immuatable context RustByFunction is the only reasonable choice for complex (non-object) types, as one can't leak a reference out of a RefCell's borrow or Mutex' lock. Since RustByFunction anyhow only works for (non-object) complex types on master, removing that user-choice does not remove functionality (that would not get lost anyhow by the switch to immutable contexts). o) Remove mut function support, for obvious reasons. o) Make functions and list/tree item properties that return non-object complex types use a similar function signature as property getters with rust-by-function. This is again because one cannot leak references out of locks/borrows. I'm also considering to wrap the creation of rust objects (what is currently just a heap allocation) in a RefCell so it can catch aliasing violations at runtime (think: some broken C++ code calling _free() as reaction to a signal sent by the same object). That should not be necessary from a correctness standpoint (again: if I didn't misunderstand something), but it'd be a nice additional safeguard. I can't make any promises when (or even if) I will finish those changes though (it's after all a spare-time project for me, and today is the last day of my vacation...), but I aim to have a first draft (code + tests) pushed to gitlab at some point during next week.
That all seems a sensible. The code was originally written when I was less familiar with rust's rules. The rust_by_function as the only way to get data is more verbose in implementation.rs and might have performance overhead, but the binding to a GUI is not the place to worry about that. What would the RefCell code look like? I'm asking because I'm modifying a model from multiple threads, so a Mutex might be more appropriate.
To be honest I haven't spent much thought on the RefCell idea. The plan would have been to replace Box::into_raw(Box::new(d_{})) by Box::into_raw(Box::new(RefCell::new(d_{}))) and all usages of that pointer, which now typically are (&*ptr), by (*ptr).borrow(). As those methods are only ever called from C++, I had assumed that all calls originate from the UI thread, but that's a restriction that the Rust bindings probably shouldn't impose on the C++ side... A Mutex at that point in the bindings would again impose unnecessary restrictions, as it would deadlock in all situations where there currently is Undefined Behaviour. What might work would be an RwLock, but the more I think about it, the less I think doing that is worthwhile, given that the original issue, aliasing a mutable pointer, is already resolved by limiting the code to immutable references, and any kind of guard introduced at that location would probably just serve as a debugging tool. For the moment I think I'll just leave the calls from C++ to Rust as they are, with raw pointers/references.
I think the first draft of the changes is ready for an initial review, in case you would like to merge them: https://invent.kde.org/sdk/rust-qt-binding-generator/-/merge_requests/2 It's still marked as WIP, because there are a few things that still need attention before merging is possible: - First of all: Do you actually want to merge that change? If not, I'm perfectly happy with having it on a separate branch. It's quite a breaking change after all, given it touches the function signature of all setters and some getters, so everyone switching to the changed version needs to update a lot of Rust code... - Only the generator and the tests have been changed up to now. I guess I should update Demo and Todos example as well? Just so the cmake build does actually work again, and to see if the changes cause any serious road-blocks missed by the tests. - What about the documentation? Should I update the code samples included in the Tutorial texts as well? Or add a note that the examples were written for version 0.3.6 and newer versions might require slightly different function signatures? - While speaking about version numbers: With this breaking change the version number should probably be incremented to 0.4.0, I guess? - When merging, the commits should probably be squashed. The main program always "worked" more or less, but the tests were most of the time in broken state, and neither Demo nor Example are currently compiling.
Correction: The cmake build currently actually works. I didn't try it before, assuming it would just overwrite the Demo's Rust bindings, but obviously that's not the case.
Yes, the resulting master should be as much functional as the current one. I'll have a look this week to see what the means. I've two projects I'd like to test with these changes, one public (mailmodel) and one private. Commit squashing is not something I've a strong opinion on.
And yes to 0.4.0 in case of merge indeed.
Thanks! I'll start modifying Demo, ToDo and the tutorial code snippets then.
I think the MR is done for now. I've modified the Demo, Example and Templates to compile with the immutable API, modified the tutorial and readme and written a changelog. While working on the Demo I found another issue, with Tree Models, new_data_ready() and threading. The auto-generated C++ code was accessing the data model from the thread that was emitting new_data_ready() before sending the Signal, causing an unexpected access to data owned by the UI-thread. Projects could have worked around this by storing their data behind an RwLock, but given that the access to the data is unnecessary for the connection between new_data_ready() and fetch_more() to work (only the internal Id is forwarded to Rust for fetch_more(), all other data is discarded anyhow), I now removed the model-access from the worker-thread. Trees still have other Signals that access the model from the thread emitting the Signal, but those Signals are directly related to user-facing data. If those are to be emitted from a worker thread, the data has to be behind an RwLock/Mutex anyhow. In any case, I've removed the WIP status from the MR, and will now start using the modified version of the binding generator in my own project.