Explicit error type for destructed callbacks

Also removes some cleverness if debug_assertions was disabled, as it really
doesn't make much of a performance difference.

src/error.rs

@@ -30,7 +30,13 @@ pub enum Error {
     ///
     /// This is an error because `rlua` callbacks are FnMut and thus can only be mutably borrowed
     /// once.
-    RecursiveCallbackError,
+    RecursiveCallback,
+    /// Either a callback or a userdata method has been called, but the callback or userdata has
+    /// been destructed.
+    ///
+    /// This can happen either due to to being destructed in a previous __gc, or due to being
+    /// destructed from exiting a `Lua::scope` call.
+    CallbackDestructed,
     /// A Rust value could not be converted to a Lua value.
     ToLuaConversionError {
         /// Name of the Rust type that could not be converted.
@@ -117,7 +123,11 @@ impl fmt::Display for Error {
             Error::GarbageCollectorError(ref msg) => {
                 write!(fmt, "garbage collector error: {}", msg)
             }
-            Error::RecursiveCallbackError => write!(fmt, "callback called recursively"),
+            Error::RecursiveCallback => write!(fmt, "callback called recursively"),
+            Error::CallbackDestructed => write!(
+                fmt,
+                "a destructed callback or destructed userdata method was called"
+            ),
             Error::ToLuaConversionError {
                 from,
                 to,

src/lua.rs

@@ -966,11 +966,6 @@ impl Lua {
         func: Callback<'callback>,
     ) -> Result<Function<'lua>> {
         unsafe extern "C" fn callback_call_impl(state: *mut ffi::lua_State) -> c_int {
-            if ffi::lua_type(state, ffi::lua_upvalueindex(1)) == ffi::LUA_TNIL {
-                ffi::lua_pushstring(state, cstr!("rust callback has been destructed"));
-                ffi::lua_error(state)
-            }
-
             callback_error(state, || {
                 let lua = Lua {
                     state: state,
@@ -978,10 +973,14 @@ impl Lua {
                     ephemeral: true,
                 };
 
+                if ffi::lua_type(state, ffi::lua_upvalueindex(1)) == ffi::LUA_TNIL {
+                    return Err(Error::CallbackDestructed);
+                }
+
                 let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1));
                 let mut func = (*func)
                     .try_borrow_mut()
-                    .map_err(|_| Error::RecursiveCallbackError)?;
+                    .map_err(|_| Error::RecursiveCallback)?;
 
                 let nargs = ffi::lua_gettop(state);
                 let mut args = MultiValue::new();

src/tests.rs

@@ -5,7 +5,8 @@ use std::cell::Cell;
 use std::sync::Arc;
 use std::panic::catch_unwind;
 
-use {Error, ExternalError, Function, Lua, Nil, Result, Table, UserData, Value, Variadic};
+use {Error, ExternalError, Function, Lua, Nil, Result, Table, UserData, UserDataMethods, Value,
+     Variadic};
 
 #[test]
 fn test_load() {
@@ -458,7 +459,7 @@ fn test_recursive_callback_error() {
     {
         Err(Error::CallbackError { ref cause, .. }) => match *cause.as_ref() {
             Error::CallbackError { ref cause, .. } => match *cause.as_ref() {
-                Error::RecursiveCallbackError { .. } => {}
+                Error::RecursiveCallback { .. } => {}
                 ref other => panic!("incorrect result: {:?}", other),
             },
             ref other => panic!("incorrect result: {:?}", other),
@@ -619,19 +620,24 @@ fn scope_func() {
     assert_eq!(rc.get(), 42);
     assert_eq!(Rc::strong_count(&rc), 1);
 
-    assert!(
+    match lua.globals()
-        lua.globals()
+        .get::<_, Function>("bad")
-            .get::<_, Function>("bad")
+        .unwrap()
-            .unwrap()
+        .call::<_, ()>(())
-            .call::<_, ()>(())
+    {
-            .is_err()
+        Err(Error::CallbackError { .. }) => {}
-    );
+        r => panic!("improper return for destructed function: {:?}", r),
+    };
 }
 
 #[test]
 fn scope_drop() {
     struct MyUserdata(Rc<()>);
-    impl UserData for MyUserdata {}
+    impl UserData for MyUserdata {
+        fn add_methods(methods: &mut UserDataMethods<Self>) {
+            methods.add_method("method", |_, _, ()| Ok(()));
+        }
+    }
 
     let rc = Rc::new(());
 
@@ -646,6 +652,11 @@ fn scope_drop() {
         assert_eq!(Rc::strong_count(&rc), 2);
     });
     assert_eq!(Rc::strong_count(&rc), 1);
+
+    match lua.exec::<()>("test:method()", None) {
+        Err(Error::CallbackError { .. }) => {}
+        r => panic!("improper return for destructed userdata: {:?}", r),
+    };
 }
 
 #[test]

src/util.rs

@@ -24,29 +24,25 @@ pub unsafe fn stack_guard<F, R>(state: *mut ffi::lua_State, change: c_int, op: F
 where
     F: FnOnce() -> R,
 {
-    if cfg!(debug_assertions) {
+    let expected = ffi::lua_gettop(state) + change;
-        let expected = ffi::lua_gettop(state) + change;
+    lua_internal_assert!(
-        lua_internal_assert!(
+        state,
-            state,
+        expected >= 0,
-            expected >= 0,
+        "too many stack values would be popped"
-            "too many stack values would be popped"
+    );
-        );
 
-        let res = op();
+    let res = op();
 
-        let top = ffi::lua_gettop(state);
+    let top = ffi::lua_gettop(state);
-        lua_internal_assert!(
+    lua_internal_assert!(
-            state,
+        state,
-            ffi::lua_gettop(state) == expected,
+        ffi::lua_gettop(state) == expected,
-            "expected stack to be {}, got {}",
+        "expected stack to be {}, got {}",
-            expected,
+        expected,
-            top
+        top
-        );
+    );
 
-        res
+    res
-    } else {
-        op()
-    }
 }
 
 // Run an operation on a lua_State and automatically clean up the stack before
@@ -62,45 +58,36 @@ pub unsafe fn stack_err_guard<F, R>(state: *mut ffi::lua_State, change: c_int, o
 where
     F: FnOnce() -> Result<R>,
 {
-    if cfg!(debug_assertions) {
+    let expected = ffi::lua_gettop(state) + change;
-        let expected = ffi::lua_gettop(state) + change;
+    lua_internal_assert!(
+        state,
+        expected >= 0,
+        "too many stack values would be popped"
+    );
+
+    let res = op();
+
+    let top = ffi::lua_gettop(state);
+    if res.is_ok() {
         lua_internal_assert!(
             state,
-            expected >= 0,
+            ffi::lua_gettop(state) == expected,
-            "too many stack values would be popped"
+            "expected stack to be {}, got {}",
+            expected,
+            top
         );
-
-        let res = op();
-
-        let top = ffi::lua_gettop(state);
-        if res.is_ok() {
-            lua_internal_assert!(
-                state,
-                ffi::lua_gettop(state) == expected,
-                "expected stack to be {}, got {}",
-                expected,
-                top
-            );
-        } else {
-            lua_internal_assert!(
-                state,
-                top >= expected,
-                "{} too many stack values popped",
-                top - expected
-            );
-            if top > expected {
-                ffi::lua_settop(state, expected);
-            }
-        }
-        res
     } else {
-        let prev = ffi::lua_gettop(state) + change;
+        lua_internal_assert!(
-        let res = op();
+            state,
-        if res.is_err() {
+            top >= expected,
-            ffi::lua_settop(state, prev);
+            "{} too many stack values popped",
+            top - expected
+        );
+        if top > expected {
+            ffi::lua_settop(state, expected);
         }
-        res
     }
+    res
 }
 
 // Call a function that calls into the Lua API and may trigger a Lua error (longjmp) in a safe way.
@@ -253,7 +240,6 @@ pub unsafe fn push_userdata<T>(state: *mut ffi::lua_State, t: T) -> Result<()> {
     })
 }
 
-// Returns None in the case that the userdata has already been garbage collected.
 pub unsafe fn get_userdata<T>(state: *mut ffi::lua_State, index: c_int) -> *mut T {
     let ud = ffi::lua_touserdata(state, index) as *mut T;
     lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
@@ -269,9 +255,10 @@ pub unsafe fn take_userdata<T>(state: *mut ffi::lua_State) -> T {
     // after the first call to __gc.
     get_destructed_userdata_metatable(state);
     ffi::lua_setmetatable(state, -2);
-    let ud = &mut *(ffi::lua_touserdata(state, -1) as *mut T);
+    let ud = ffi::lua_touserdata(state, -1) as *mut T;
+    lua_internal_assert!(state, !ud.is_null(), "userdata pointer is null");
     ffi::lua_pop(state, 1);
-    mem::replace(ud, mem::uninitialized())
+    ptr::read(ud)
 }
 
 pub unsafe extern "C" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c_int {
@@ -611,7 +598,7 @@ unsafe fn get_destructed_userdata_metatable(state: *mut ffi::lua_State) -> c_int
     static DESTRUCTED_USERDATA_METATABLE: u8 = 0;
 
     unsafe extern "C" fn destructed_error(state: *mut ffi::lua_State) -> c_int {
-        ffi::lua_pushstring(state, cstr!("userdata has been destructed"));
+        push_wrapped_error(state, Error::CallbackDestructed);
         ffi::lua_error(state)
     }