Add parent checking to BB methods. Fixes rust-lang#93

Author: TheDan64

Diff for: src/basic_block.rs

@@ -137,7 +137,7 @@ impl BasicBlock {
         if self.get_parent().is_none() {
             return None;
         }
- 
+
         let bb = unsafe {
             LLVMGetNextBasicBlock(self.basic_block)
         };
@@ -146,6 +146,7 @@ impl BasicBlock {
     }
 
     /// Prepends one `BasicBlock` before another.
+    /// It returns `Err(())` when either `BasicBlock` has no parent, as LLVM assumes it has a parent.
     ///
     /// # Example
     /// ```no_run
@@ -168,13 +169,21 @@ impl BasicBlock {
     /// assert_eq!(basic_block2.get_next_basic_block().unwrap(), basic_block1);
     /// ```
     // REVIEW: What happens if blocks are from different scopes?
-    pub fn move_before(&self, basic_block: &BasicBlock) {
+    pub fn move_before(&self, basic_block: &BasicBlock) -> Result<(), ()> {
+        // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+        if self.get_parent().is_none() || basic_block.get_parent().is_none() {
+            return Err(());
+        }
+
         unsafe {
             LLVMMoveBasicBlockBefore(self.basic_block, basic_block.basic_block)
         }
+
+        Ok(())
     }
 
     /// Appends one `BasicBlock` after another.
+    /// It returns `Err(())` when either `BasicBlock` has no parent, as LLVM assumes it has a parent.
     ///
     /// # Example
     /// ```no_run
@@ -197,10 +206,17 @@ impl BasicBlock {
     /// assert_eq!(basic_block2.get_next_basic_block().unwrap(), basic_block1);
     /// ```
     // REVIEW: What happens if blocks are from different scopes?
-    pub fn move_after(&self, basic_block: &BasicBlock) {
+    pub fn move_after(&self, basic_block: &BasicBlock) -> Result<(), ()> {
+        // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+        if self.get_parent().is_none() || basic_block.get_parent().is_none() {
+            return Err(());
+        }
+
         unsafe {
             LLVMMoveBasicBlockAfter(self.basic_block, basic_block.basic_block)
         }
+
+        Ok(())
     }
 
     /// Prepends a new `BasicBlock` before this one.
@@ -340,6 +356,7 @@ impl BasicBlock {
     }
 
     /// Removes this `BasicBlock` from its parent `FunctionValue`. Does nothing if it has no parent.
+    /// It returns `Err(())` when it has no parent to remove from, as LLVM assumes it has a parent.
     ///
     /// # Example
     /// ```no_run
@@ -364,16 +381,21 @@ impl BasicBlock {
     // by taking ownership of self (though BasicBlock's are not uniquely obtained...)
     // might have to make some methods do something like -> Result<..., BasicBlock<Orphan>> for BasicBlock<HasParent>
     // and would move_before/after make it no longer orphaned? etc..
-    pub fn remove_from_function(&self) {
+    pub fn remove_from_function(&self) -> Result<(), ()> {
         // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
-        if self.get_parent().is_some() {
-            unsafe {
-                LLVMRemoveBasicBlockFromParent(self.basic_block)
-            }
+        if self.get_parent().is_none() {
+            return Err(());
         }
+
+        unsafe {
+            LLVMRemoveBasicBlockFromParent(self.basic_block)
+        }
+
+        Ok(())
     }
 
     /// Removes this `BasicBlock` completely from memory. This is unsafe because you could easily have other references to the same `BasicBlock`.
+    /// It returns `Err(())` when it has no parent to delete from, as LLVM assumes it has a parent.
     ///
     /// # Example
     /// ```no_run
@@ -393,11 +415,15 @@ impl BasicBlock {
     /// }
     /// assert!(function.get_basic_blocks().is_empty());
     /// ```
-    // REVIEW: Could potentially be unsafe if there are existing references. Might need a global ref counter
-    pub unsafe fn delete(self) {
-        // unsafe {
-        LLVMDeleteBasicBlock(self.basic_block)
-        // }
+    pub unsafe fn delete(self) -> Result<(), ()> {
+        // This method is UB if the parent no longer exists, so we must check for parent (or encode into type system)
+        if self.get_parent().is_none() {
+            return Err(());
+        }
+
+        LLVMDeleteBasicBlock(self.basic_block);
+
+        Ok(())
     }
 
     /// Obtains the `ContextRef` this `BasicBlock` belongs to.

Diff for: tests/all/test_basic_block.rs

@@ -33,8 +33,8 @@ fn test_basic_block_ordering() {
     assert_eq!(basic_blocks[2], basic_block3);
     assert_eq!(basic_blocks[3], basic_block4);
 
-    basic_block3.move_before(&basic_block2);
-    basic_block.move_after(&basic_block4);
+    assert!(basic_block3.move_before(&basic_block2).is_ok());
+    assert!(basic_block.move_after(&basic_block4).is_ok());
 
     let basic_block5 = basic_block.prepend_basic_block("block5");
     let basic_blocks = function.get_basic_blocks();
@@ -62,7 +62,7 @@ fn test_basic_block_ordering() {
     assert!(basic_block3.get_previous_basic_block().is_none());
 
     unsafe {
-        bb4.delete();
+        assert!(bb4.delete().is_ok());
     }
 
     let bb2 = basic_block5.get_previous_basic_block().unwrap();
@@ -153,21 +153,13 @@ fn test_no_parent() {
     assert_eq!(basic_block.get_parent().unwrap(), function);
     assert_eq!(basic_block.get_next_basic_block().unwrap(), basic_block2);
 
-    // TODO: Test if this method is unsafe if parent function was hard deleted
-    basic_block.remove_from_function();
+    assert!(basic_block.remove_from_function().is_ok());
 
     assert!(basic_block.get_next_basic_block().is_none());
     assert!(basic_block2.get_previous_basic_block().is_none());
 
-    // The problem here is that calling the function more than once becomes UB
-    // for some reason so we have to manually check for the parent function (in the call)
-    // until we have SubTypes to solve this at compile time by doing something like:
-    // impl BasicBlock<HasParent> { fn remove_from_function(self) -> BasicBlock<Orphan> }
-    // though having to take ownership does raise some flags for when you just want to make
-    // everything borrowable. I'm not sure it's possible to swap in place since they have
-    // different subtypes
-    basic_block.remove_from_function();
-    basic_block.remove_from_function();
+    assert!(basic_block.remove_from_function().is_err());
+    assert!(basic_block.remove_from_function().is_err());
 
     assert!(basic_block.get_parent().is_none());
 }