Modernize lua read (part 2 & 3): C++ templating assurance (#7410)

* Modernize lua read (part 2 & 3): C++ templating assurance

Implement the boolean reader
Implement the string reader
Also remove unused & unimplemented script_error_handler
Add a reader with default value

build/android/jni/Android.mk

@@ -316,6 +316,7 @@ LOCAL_SRC_FILES += \
 		jni/src/script/common/c_converter.cpp     \
 		jni/src/script/common/c_internal.cpp      \
 		jni/src/script/common/c_types.cpp         \
+		jni/src/script/common/helper.cpp          \
 		jni/src/script/cpp_api/s_async.cpp        \
 		jni/src/script/cpp_api/s_base.cpp         \
 		jni/src/script/cpp_api/s_client.cpp       \

src/script/common/CMakeLists.txt

@@ -3,6 +3,7 @@ set(common_SCRIPT_COMMON_SRCS
 	${CMAKE_CURRENT_SOURCE_DIR}/c_converter.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/c_types.cpp
 	${CMAKE_CURRENT_SOURCE_DIR}/c_internal.cpp
+	${CMAKE_CURRENT_SOURCE_DIR}/helper.cpp
 	PARENT_SCOPE)
 
 set(client_SCRIPT_COMMON_SRCS

src/script/common/c_internal.h

@@ -95,11 +95,10 @@ enum RunCallbacksMode
 	// after seeing the first true value
 	RUN_CALLBACKS_MODE_OR_SC,
 	// Note: "a true value" and "a false value" refer to values that
-	// are converted by lua_toboolean to true or false, respectively.
+	// are converted by readParam<bool> to true or false, respectively.
 };
 
 std::string script_get_backtrace(lua_State *L);
-int script_error_handler(lua_State *L);
 int script_exception_wrapper(lua_State *L, lua_CFunction f);
 void script_error(lua_State *L, int pcall_result, const char *mod, const char *fxn);
 void script_run_callbacks_f(lua_State *L, int nargs,

src/script/common/helper.cpp

@@ -0,0 +1,73 @@
+/*
+Minetest
+Copyright (C) 2018 nerzhul, Loic Blot <loic.blot@unix-experience.fr>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU Lesser General Public License as published by
+the Free Software Foundation; either version 2.1 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public License along
+with this program; if not, write to the Free Software Foundation, Inc.,
+51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+#include "helper.h"
+#include <cmath>
+#include <sstream>
+#include "c_types.h"
+
+bool LuaHelper::isNaN(lua_State *L, int idx)
+{
+	return lua_type(L, idx) == LUA_TNUMBER && std::isnan(lua_tonumber(L, idx));
+}
+
+/*
+ * Read template functions
+ */
+template <> bool LuaHelper::readParam(lua_State *L, int index)
+{
+	return lua_toboolean(L, index) != 0;
+}
+
+template <> bool LuaHelper::readParam(lua_State *L, int index, const bool &default_value)
+{
+	if (lua_isnil(L, index))
+		return default_value;
+
+	return lua_toboolean(L, index) != 0;
+}
+
+template <> float LuaHelper::readParam(lua_State *L, int index)
+{
+	if (isNaN(L, index))
+		throw LuaError("NaN value is not allowed.");
+
+	return (float)luaL_checknumber(L, index);
+}
+
+template <> std::string LuaHelper::readParam(lua_State *L, int index)
+{
+	std::string result;
+	const char *str = luaL_checkstring(L, index);
+	result.append(str);
+	return result;
+}
+
+template <>
+std::string LuaHelper::readParam(
+		lua_State *L, int index, const std::string &default_value)
+{
+	std::string result;
+	const char *str = lua_tostring(L, index);
+	if (str)
+		result.append(str);
+	else
+		result = default_value;
+	return result;
+}

src/script/common/helper.h

@@ -0,0 +1,54 @@
+/*
+Minetest
+Copyright (C) 2018 nerzhul, Loic Blot <loic.blot@unix-experience.fr>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU Lesser General Public License as published by
+the Free Software Foundation; either version 2.1 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public License along
+with this program; if not, write to the Free Software Foundation, Inc.,
+51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+
+#pragma once
+
+extern "C" {
+#include <lua.h>
+#include <lauxlib.h>
+}
+
+class LuaHelper
+{
+protected:
+	static bool isNaN(lua_State *L, int idx);
+
+	/**
+	 * Read a value using a template type T from Lua State L and index
+	 *
+	 *
+	 * @tparam T type to read from Lua
+	 * @param L Lua state
+	 * @param index Lua Index to read
+	 * @return read value from Lua
+	 */
+	template <typename T> static T readParam(lua_State *L, int index);
+
+	/**
+	 * Read a value using a template type T from Lua State L and index
+	 *
+	 * @tparam T type to read from Lua
+	 * @param L Lua state
+	 * @param index Lua Index to read
+	 * @param default_value default value to apply if nil
+	 * @return read value from Lua or default value if nil
+	 */
+	template <typename T>
+	static T readParam(lua_State *L, int index, const T &default_value);
+};

src/script/cpp_api/s_base.cpp

@@ -133,7 +133,7 @@ int ScriptApiBase::luaPanic(lua_State *L)
 {
 	std::ostringstream oss;
 	oss << "LUA PANIC: unprotected error in call to Lua API ("
-		<< lua_tostring(L, -1) << ")";
+		<< readParam<std::string>(L, -1) << ")";
 	FATAL_ERROR(oss.str().c_str());
 	// NOTREACHED
 	return 0;
@@ -184,7 +184,7 @@ void ScriptApiBase::loadScript(const std::string &script_path)
 	}
 	ok = ok && !lua_pcall(L, 0, 0, error_handler);
 	if (!ok) {
-		std::string error_msg = lua_tostring(L, -1);
+		std::string error_msg = readParam<std::string>(L, -1);
 		lua_pop(L, 2); // Pop error message and error handler
 		throw ModError("Failed to load and run script from " +
 				script_path + ":\n" + error_msg);
@@ -286,10 +286,10 @@ void ScriptApiBase::stackDump(std::ostream &o)
 		int t = lua_type(m_luastack, i);
 		switch (t) {
 			case LUA_TSTRING:  /* strings */
-				o << "\"" << lua_tostring(m_luastack, i) << "\"";
+				o << "\"" << readParam<std::string>(m_luastack, i) << "\"";
 				break;
 			case LUA_TBOOLEAN:  /* booleans */
-				o << (lua_toboolean(m_luastack, i) ? "true" : "false");
+				o << (readParam<bool>(m_luastack, i) ? "true" : "false");
 				break;
 			case LUA_TNUMBER:  /* numbers */ {
 				char buf[10];

src/script/cpp_api/s_base.h

@@ -24,6 +24,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 #include <thread>
 #include <mutex>
 #include <unordered_map>
+#include "common/helper.h"
 #include "util/basic_macros.h"
 
 extern "C" {
@@ -74,7 +75,7 @@ class GUIEngine;
 class ServerActiveObject;
 struct PlayerHPChangeReason;
 
-class ScriptApiBase {
+class ScriptApiBase : protected LuaHelper {
 public:
 	ScriptApiBase(ScriptingType type);
 	// fake constructor to allow script API classes (e.g ScriptApiEnv) to virtually inherit from this one.

src/script/cpp_api/s_client.cpp

@@ -57,8 +57,7 @@ bool ScriptApiClient::on_sending_message(const std::string &message)
 	// Call callbacks
 	lua_pushstring(L, message.c_str());
 	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
-	bool ate = lua_toboolean(L, -1);
-	return ate;
+	return readParam<bool>(L, -1);
 }
 
 bool ScriptApiClient::on_receiving_message(const std::string &message)
@@ -71,8 +70,7 @@ bool ScriptApiClient::on_receiving_message(const std::string &message)
 	// Call callbacks
 	lua_pushstring(L, message.c_str());
 	runCallbacks(1, RUN_CALLBACKS_MODE_OR_SC);
-	bool ate = lua_toboolean(L, -1);
-	return ate;
+	return readParam<bool>(L, -1);
 }
 
 void ScriptApiClient::on_damage_taken(int32_t damage_amount)
@@ -186,8 +184,7 @@ bool ScriptApiClient::on_punchnode(v3s16 p, MapNode node)
 
 	// Call functions
 	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
-	bool blocked = lua_toboolean(L, -1);
-	return blocked;
+	return readParam<bool>(L, -1);
 }
 
 bool ScriptApiClient::on_placenode(const PointedThing &pointed, const ItemDefinition &item)
@@ -204,7 +201,7 @@ bool ScriptApiClient::on_placenode(const PointedThing &pointed, const ItemDefini
 
 	// Call functions
 	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
-	return lua_toboolean(L, -1);
+	return readParam<bool>(L, -1);
 }
 
 bool ScriptApiClient::on_item_use(const ItemStack &item, const PointedThing &pointed)
@@ -221,7 +218,7 @@ bool ScriptApiClient::on_item_use(const ItemStack &item, const PointedThing &poi
 
 	// Call functions
 	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
-	return lua_toboolean(L, -1);
+	return readParam<bool>(L, -1);
 }
 
 bool ScriptApiClient::on_inventory_open(Inventory *inventory)
@@ -242,7 +239,7 @@ bool ScriptApiClient::on_inventory_open(Inventory *inventory)
 	}
 
 	runCallbacks(1, RUN_CALLBACKS_MODE_OR);
-	return lua_toboolean(L, -1);
+	return readParam<bool>(L, -1);
 }
 
 void ScriptApiClient::setEnv(ClientEnvironment *env)

src/script/cpp_api/s_entity.cpp

@@ -257,7 +257,7 @@ bool ScriptApiEntity::luaentity_Punch(u16 id,
 	setOriginFromTable(object);
 	PCALL_RES(lua_pcall(L, 6, 1, error_handler));
 
-	bool retval = lua_toboolean(L, -1);
+	bool retval = readParam<bool>(L, -1);
 	lua_pop(L, 2); // Pop object and error handler
 	return retval;
 }
@@ -287,7 +287,7 @@ bool ScriptApiEntity::luaentity_run_simple_callback(u16 id,
 	setOriginFromTable(object);
 	PCALL_RES(lua_pcall(L, 2, 1, error_handler));
 
-	bool retval = lua_toboolean(L, -1);
+	bool retval = readParam<bool>(L, -1);
 	lua_pop(L, 2); // Pop object and error handler
 	return retval;
 }

src/script/cpp_api/s_env.cpp

@@ -116,12 +116,12 @@ void ScriptApiEnv::initializeEnvironment(ServerEnvironment *env)
 			while (lua_next(L, table)) {
 				// key at index -2 and value at index -1
 				luaL_checktype(L, -1, LUA_TSTRING);
-				trigger_contents.emplace_back(lua_tostring(L, -1));
+				trigger_contents.emplace_back(readParam<std::string>(L, -1));
 				// removes value, keeps key for next iteration
 				lua_pop(L, 1);
 			}
 		} else if (lua_isstring(L, -1)) {
-			trigger_contents.emplace_back(lua_tostring(L, -1));
+			trigger_contents.emplace_back(readParam<std::string>(L, -1));
 		}
 		lua_pop(L, 1);
 
@@ -133,12 +133,12 @@ void ScriptApiEnv::initializeEnvironment(ServerEnvironment *env)
 			while (lua_next(L, table)) {
 				// key at index -2 and value at index -1
 				luaL_checktype(L, -1, LUA_TSTRING);
-				required_neighbors.emplace_back(lua_tostring(L, -1));
+				required_neighbors.emplace_back(readParam<std::string>(L, -1));
 				// removes value, keeps key for next iteration
 				lua_pop(L, 1);
 			}
 		} else if (lua_isstring(L, -1)) {
-			required_neighbors.emplace_back(lua_tostring(L, -1));
+			required_neighbors.emplace_back(readParam<std::string>(L, -1));
 		}
 		lua_pop(L, 1);
 
@@ -185,12 +185,12 @@ void ScriptApiEnv::initializeEnvironment(ServerEnvironment *env)
 			while (lua_next(L, table)) {
 				// key at index -2 and value at index -1
 				luaL_checktype(L, -1, LUA_TSTRING);
-				trigger_contents.insert(lua_tostring(L, -1));
+				trigger_contents.insert(readParam<std::string>(L, -1));
 				// removes value, keeps key for next iteration
 				lua_pop(L, 1);
 			}
 		} else if (lua_isstring(L, -1)) {
-			trigger_contents.insert(lua_tostring(L, -1));
+			trigger_contents.insert(readParam<std::string>(L, -1));
 		}
 		lua_pop(L, 1);
 

src/script/cpp_api/s_node.cpp

@@ -192,7 +192,7 @@ bool ScriptApiNode::node_on_flood(v3s16 p, MapNode node, MapNode newnode)
 	pushnode(L, newnode, ndef);
 	PCALL_RES(lua_pcall(L, 3, 1, error_handler));
 	lua_remove(L, error_handler);
-	return (bool) lua_isboolean(L, -1) && (bool) lua_toboolean(L, -1);
+	return readParam<bool>(L, -1, false);
 }
 
 void ScriptApiNode::node_after_destruct(v3s16 p, MapNode node)
@@ -231,7 +231,7 @@ bool ScriptApiNode::node_on_timer(v3s16 p, MapNode node, f32 dtime)
 	lua_pushnumber(L,dtime);
 	PCALL_RES(lua_pcall(L, 2, 1, error_handler));
 	lua_remove(L, error_handler);
-	return (bool) lua_isboolean(L, -1) && (bool) lua_toboolean(L, -1);
+	return readParam<bool>(L, -1, false);
 }
 
 void ScriptApiNode::node_on_receive_fields(v3s16 p,

src/script/cpp_api/s_player.cpp

@@ -74,7 +74,7 @@ bool ScriptApiPlayer::on_punchplayer(ServerActiveObject *player,
 	push_v3f(L, dir);
 	lua_pushnumber(L, damage);
 	runCallbacks(6, RUN_CALLBACKS_MODE_OR);
-	return lua_toboolean(L, -1);
+	return readParam<bool>(L, -1);
 }
 
 s16 ScriptApiPlayer::on_player_hpchange(ServerActiveObject *player,
@@ -111,8 +111,7 @@ bool ScriptApiPlayer::on_respawnplayer(ServerActiveObject *player)
 	// Call callbacks
 	objectrefGetOrCreate(L, player);
 	runCallbacks(1, RUN_CALLBACKS_MODE_OR);
-	bool positioning_handled_by_some = lua_toboolean(L, -1);
-	return positioning_handled_by_some;
+	return readParam<bool>(L, -1);
 }
 
 bool ScriptApiPlayer::on_prejoinplayer(
@@ -129,7 +128,7 @@ bool ScriptApiPlayer::on_prejoinplayer(
 	lua_pushstring(L, ip.c_str());
 	runCallbacks(2, RUN_CALLBACKS_MODE_OR);
 	if (lua_isstring(L, -1)) {
-		reason->assign(lua_tostring(L, -1));
+		reason->assign(readParam<std::string>(L, -1));
 		return true;
 	}
 	return false;

src/script/cpp_api/s_security.cpp

@@ -524,7 +524,7 @@ bool ScriptApiSecurity::checkPath(lua_State *L, const char *path,
 	// Get mod name
 	lua_rawgeti(L, LUA_REGISTRYINDEX, CUSTOM_RIDX_CURRENT_MOD_NAME);
 	if (lua_isstring(L, -1)) {
-		std::string mod_name = lua_tostring(L, -1);
+		std::string mod_name = readParam<std::string>(L, -1);
 
 		// Builtin can access anything
 		if (mod_name == BUILTIN_MOD_NAME) {
@@ -649,7 +649,7 @@ int ScriptApiSecurity::sl_g_loadfile(lua_State *L)
 	lua_pop(L, 1);
 
 	if (script->getType() == ScriptingType::Client) {
-		std:: string display_path = lua_tostring(L, 1);
+		std::string display_path = readParam<std::string>(L, 1);
 		const std::string *path = script->getClient()->getModFile(display_path);
 		if (!path) {
 			std::string error_msg = "Coudln't find script called:" + display_path;