Add `ExpiredUserData` error and avoid what was previously a panic

Also make sure that panic messages clearly state that they are internal errors,
so people report them as a bug.  Since the only panics left are all internal
errors, just move the internal error message into the panic / assert macros.

src/error.rs

@@ -31,6 +31,12 @@ pub enum Error {
     /// This is an error, since `rlua` callbacks are FnMut an the functions can only be mutably
     /// borrowed once.
     RecursiveCallbackError,
+    /// Lua code has accessed a [`UserData`] value that was already garbage collected
+    ///
+    /// This can happen when a [`UserData`] has a custom `__gc` metamethod, this method resurrects
+    /// the [`UserData`], and then the [`UserData`] is subsequently accessed.
+    /// [`UserData`]: trait.UserData.html
+    ExpiredUserData,
     /// A Rust value could not be converted to a Lua value.
     ToLuaConversionError {
         /// Name of the Rust type that could not be converted.
@@ -116,6 +122,10 @@ impl fmt::Display for Error {
                 write!(fmt, "garbage collector error: {}", msg)
             }
             Error::RecursiveCallbackError => write!(fmt, "callback called recursively"),
+            Error::ExpiredUserData => write!(
+                fmt,
+                "access of userdata which has already been garbage collected"
+            ),
             Error::ToLuaConversionError {
                 from,
                 to,
@@ -157,6 +167,7 @@ impl StdError for Error {
             Error::RuntimeError(_) => "runtime error",
             Error::GarbageCollectorError(_) => "garbage collector error",
             Error::RecursiveCallbackError => "callback called recursively",
+            Error::ExpiredUserData => "access of userdata which has already been garbage collected",
             Error::ToLuaConversionError { .. } => "conversion error to lua",
             Error::FromLuaConversionError { .. } => "conversion error from lua",
             Error::CoroutineInactive => "attempt to resume inactive coroutine",

src/lua.rs

@@ -567,7 +567,7 @@ impl Lua {
                 &LUA_USERDATA_REGISTRY_KEY as *const u8 as *mut c_void,
             );
             ffi::lua_gettable(self.state, ffi::LUA_REGISTRYINDEX);
-            let registered_userdata = get_userdata::<HashMap<TypeId, c_int>>(self.state, -1);
+            let registered_userdata = get_userdata::<HashMap<TypeId, c_int>>(self.state, -1)?;
             ffi::lua_pop(self.state, 1);
 
             if let Some(table_id) = (*registered_userdata).get(&TypeId::of::<T>()) {
@@ -798,7 +798,7 @@ impl Lua {
                     ephemeral: true,
                 };
 
-                let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1));
+                let func = get_userdata::<RefCell<Callback>>(state, ffi::lua_upvalueindex(1))?;
                 let mut func = (*func)
                     .try_borrow_mut()
                     .map_err(|_| Error::RecursiveCallbackError)?;

src/macros.rs

@@ -16,14 +16,14 @@ macro_rules! lua_panic {
     ($state:expr, $msg:expr) => {
         {
             $crate::ffi::lua_settop($state, 0);
-            panic!(concat!("rlua: ", $msg));
+            panic!(concat!("rlua internal error: ", $msg));
         }
     };
 
     ($state:expr, $fmt:expr, $($arg:tt)+) => {
         {
             $crate::ffi::lua_settop($state, 0);
-            panic!(concat!("rlua: ", $fmt), $($arg)+);
+            panic!(concat!("rlua internal error: ", $fmt), $($arg)+);
         }
     };
 }
@@ -40,14 +40,14 @@ macro_rules! lua_assert {
     ($state:expr, $cond:expr, $msg:expr) => {
         if !$cond {
             $crate::ffi::lua_settop($state, 0);
-            panic!(concat!("rlua: ", $msg));
+            panic!(concat!("rlua internal error: ", $msg));
         }
     };
 
     ($state:expr, $cond:expr, $fmt:expr, $($arg:tt)+) => {
         if !$cond {
             $crate::ffi::lua_settop($state, 0);
-            panic!(concat!("rlua: ", $fmt), $($arg)+);
+            panic!(concat!("rlua internal error: ", $fmt), $($arg)+);
         }
     };
 }

src/tests.rs

@@ -448,13 +448,20 @@ fn test_recursive_callback_error() {
         Ok(())
     }).unwrap();
     lua.globals().set("f", f).unwrap();
-    assert!(
+    match lua.globals()
-        lua.globals()
+        .get::<_, Function>("f")
-            .get::<_, Function>("f")
+        .unwrap()
-            .unwrap()
+        .call::<_, ()>(false)
-            .call::<_, ()>(false)
+    {
-            .is_err()
+        Err(Error::CallbackError { ref cause, .. }) => match *cause.as_ref() {
-    )
+            Error::CallbackError { ref cause, .. } => match *cause.as_ref() {
+                Error::RecursiveCallbackError { .. } => {}
+                ref other => panic!("incorrect result: {:?}", other),
+            },
+            ref other => panic!("incorrect result: {:?}", other),
+        },
+        other => panic!("incorrect result: {:?}", other),
+    };
 }
 
 #[test]

src/thread.rs

@@ -142,6 +142,8 @@ impl<'lua> Thread<'lua> {
 
 #[cfg(test)]
 mod tests {
+    use std::panic::catch_unwind;
+
     use super::{Thread, ThreadStatus};
     use error::Error;
     use function::Function;
@@ -243,8 +245,8 @@ mod tests {
     }
 
     #[test]
-    #[should_panic]
     fn coroutine_panic() {
+        // check that coroutines propagate panics correctly
         let lua = Lua::new();
         let thrd_main = lua.create_function(|lua, ()| {
             // whoops, 'main' has a wrong type
@@ -253,6 +255,10 @@ mod tests {
         }).unwrap();
         lua.globals().set("main", thrd_main.clone()).unwrap();
         let thrd: Thread = lua.create_thread(thrd_main).unwrap();
-        thrd.resume::<_, ()>(()).unwrap();
+
+        match catch_unwind(|| thrd.resume::<_, ()>(())) {
+            Ok(r) => panic!("coroutine panic not propagated, instead returned {:?}", r),
+            Err(_) => {}
+        }
     }
 }

src/userdata.rs

@@ -358,7 +358,7 @@ impl<'lua> AnyUserData<'lua> {
     {
         unsafe {
             let lua = self.0.lua;
-            stack_guard(lua.state, 0, move || {
+            stack_err_guard(lua.state, 0, move || {
                 check_stack(lua.state, 3);
 
                 lua.push_ref(lua.state, &self.0);
@@ -379,7 +379,7 @@ impl<'lua> AnyUserData<'lua> {
                     ffi::lua_pop(lua.state, 3);
                     Err(Error::UserDataTypeMismatch)
                 } else {
-                    let res = func(&*get_userdata::<RefCell<T>>(lua.state, -3));
+                    let res = func(&*get_userdata::<RefCell<T>>(lua.state, -3)?);
                     ffi::lua_pop(lua.state, 3);
                     res
                 }
@@ -391,7 +391,7 @@ impl<'lua> AnyUserData<'lua> {
 #[cfg(test)]
 mod tests {
     use super::{MetaMethod, UserData, UserDataMethods};
-    use error::ExternalError;
+    use error::{Error, ExternalError};
     use string::String;
     use function::Function;
     use lua::Lua;
@@ -505,7 +505,6 @@ mod tests {
     }
 
     #[test]
-    #[should_panic]
     fn test_expired_userdata() {
         struct MyUserdata {
             id: u8,
@@ -526,7 +525,7 @@ mod tests {
             globals.set("userdata", MyUserdata { id: 123 }).unwrap();
         }
 
-        lua.eval::<()>(
+        match lua.eval::<()>(
             r#"
                 local tbl = setmetatable({
                     userdata = userdata
@@ -541,7 +540,13 @@ mod tests {
                 hatch:access()
             "#,
             None,
-        ).unwrap();
+        ) {
+            Err(Error::CallbackError { cause, .. }) => match *cause {
+                Error::ExpiredUserData { .. } => {}
+                ref other => panic!("incorrect result: {}", other),
+            },
+            other => panic!("incorrect result: {:?}", other),
+        }
     }
 
     #[test]

src/util.rs

@@ -28,7 +28,7 @@ where
     lua_assert!(
         state,
         expected >= 0,
-        "internal stack error: too many values would be popped"
+        "too many stack values would be popped"
     );
 
     let res = op();
@@ -37,7 +37,7 @@ where
     lua_assert!(
         state,
         ffi::lua_gettop(state) == expected,
-        "internal stack error: expected stack to be {}, got {}",
+        "expected stack to be {}, got {}",
         expected,
         top
     );
@@ -62,7 +62,7 @@ where
     lua_assert!(
         state,
         expected >= 0,
-        "internal stack error: too many values would be popped"
+        "too many stack values would be popped"
     );
 
     let res = op();
@@ -72,7 +72,7 @@ where
         lua_assert!(
             state,
             ffi::lua_gettop(state) == expected,
-            "internal stack error: expected stack to be {}, got {}",
+            "expected stack to be {}, got {}",
             expected,
             top
         );
@@ -80,7 +80,7 @@ where
         lua_assert!(
             state,
             top >= expected,
-            "internal stack error: {} too many values popped",
+            "{} too many stack values popped",
             top - expected
         );
         if top > expected {
@@ -164,12 +164,13 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
     if let Some(err) = pop_wrapped_error(state) {
         err
     } else if is_wrapped_panic(state, -1) {
-        let panic = get_userdata::<WrappedPanic>(state, -1);
+        let panic = get_userdata::<WrappedPanic>(state, -1)
+            .expect("WrappedPanic was somehow resurrected after garbage collection");
         if let Some(p) = (*panic).0.take() {
             ffi::lua_settop(state, 0);
             resume_unwind(p);
         } else {
-            lua_panic!(state, "internal error: panic was resumed twice")
+            lua_panic!(state, "panic was resumed twice")
         }
     } else {
         let err_string = gc_guard(state, || {
@@ -205,7 +206,7 @@ pub unsafe fn pop_error(state: *mut ffi::lua_State, err_code: c_int) -> Error {
                 process::abort()
             }
             ffi::LUA_ERRGCMM => Error::GarbageCollectorError(err_string),
-            _ => lua_panic!(state, "internal error: unrecognized lua error code"),
+            _ => lua_panic!(state, "unrecognized lua error code"),
         }
     }
 }
@@ -224,11 +225,14 @@ pub unsafe fn push_userdata<T>(state: *mut ffi::lua_State, t: T) -> Result<()> {
     })
 }
 
-pub unsafe fn get_userdata<T>(state: *mut ffi::lua_State, index: c_int) -> *mut T {
+// 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) -> Result<*mut T> {
     let ud = ffi::lua_touserdata(state, index) as *mut Option<T>;
-    lua_assert!(state, !ud.is_null());
+    lua_assert!(state, !ud.is_null(), "userdata pointer is null");
-    lua_assert!(state, (*ud).is_some(), "access of expired userdata");
+    (*ud)
-    (*ud).as_mut().unwrap()
+        .as_mut()
+        .map(|v| v as *mut T)
+        .ok_or(Error::ExpiredUserData)
 }
 
 pub unsafe extern "C" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c_int {
@@ -377,7 +381,8 @@ pub unsafe fn pop_wrapped_error(state: *mut ffi::lua_State) -> Option<Error> {
     if ffi::lua_gettop(state) == 0 || !is_wrapped_error(state, -1) {
         None
     } else {
-        let err = &*get_userdata::<WrappedError>(state, -1);
+        let err = &*get_userdata::<WrappedError>(state, -1)
+            .expect("WrappedError was somehow resurrected after garbage collection");
         let err = err.0.clone();
         ffi::lua_pop(state, 1);
         Some(err)
@@ -457,7 +462,8 @@ unsafe fn get_error_metatable(state: *mut ffi::lua_State) -> c_int {
     unsafe extern "C" fn error_tostring(state: *mut ffi::lua_State) -> c_int {
         callback_error(state, || {
             if is_wrapped_error(state, -1) {
-                let error = get_userdata::<WrappedError>(state, -1);
+                let error = get_userdata::<WrappedError>(state, -1)
+                    .expect("WrappedError was somehow resurrected after garbage collection");
                 let error_str = (*error).0.to_string();
                 gc_guard(state, || {
                     ffi::lua_pushlstring(
@@ -470,7 +476,7 @@ unsafe fn get_error_metatable(state: *mut ffi::lua_State) -> c_int {
 
                 Ok(1)
             } else {
-                panic!("internal error: userdata mismatch in Error metamethod");
+                panic!("userdata mismatch in Error metamethod");
             }
         })
     }