Fix LuaStandalone nil handling

In Lua, a table entry with a nil value is the same as a table entry that
doesn't exist. So when serializing for transfer to PHP, these keys will
be skipped. For a table as an associative array this isn't much of a
problem, but for a table as a list it means we have missing indexes.
Some of Lua's functions for handling "lists" (i.e. tables with numeric
keys) also have a problem when the list contains nils.

To work around these issues when passing argument lists and return value
lists, pass the number of elements along with the sparse list. On the
PHP end we can use this to fill in the missing nulls, and on the Lua
end we can pass this count to unpack() to avoid the problems on the Lua
side.

Change-Id: I858e3905a06e377693301da2b8bc534808f00e3e

engines/LuaStandalone/LuaStandaloneEngine.php

@@ -174,6 +174,7 @@ class Scribunto_LuaStandaloneInterpreter extends Scribunto_LuaInterpreter {
 		$result = $this->dispatch( array(
 			'op' => 'call',
 			'id' => $func->id,
+			'nargs' => count( $args ),
 			'args' => $args ) );
 		// Convert return values to zero-based
 		return array_values( $result );
@@ -212,7 +213,23 @@ class Scribunto_LuaStandaloneInterpreter extends Scribunto_LuaInterpreter {
 		return $result[1];
 	}
 
+	/**
+	 * Fill in missing nulls in a list received from Lua
+	 *
+	 * @param $array array List received from Lua
+	 * @param $count integer Number of values that should be in the list
+	 * @return array Non-sparse array
+	 */
+	private static function fixNulls( array $array, $count ) {
+		if ( count( $array ) === $count ) {
+			return $array;
+		} else {
+			return array_replace( array_fill( 1, $count, null ), $array );
+		}
+	}
+
 	protected function handleCall( $message ) {
+		$message['args'] = self::fixNulls( $message['args'], $message['nargs'] );
 		try {
 			$result = $this->callback( $message['id'], $message['args'] );
 		} catch ( Scribunto_LuaError $e ) {
@@ -230,6 +247,7 @@ class Scribunto_LuaStandaloneInterpreter extends Scribunto_LuaInterpreter {
 
 		return array(
 			'op' => 'return',
+			'nvalues' => count( $result ),
 			'values' => $result
 		);
 	}
@@ -258,7 +276,7 @@ class Scribunto_LuaStandaloneInterpreter extends Scribunto_LuaInterpreter {
 
 			switch ( $msgFromLua['op'] ) {
 				case 'return':
-					return $msgFromLua['values'];
+					return self::fixNulls( $msgFromLua['values'], $msgFromLua['nvalues'] );
 				case 'call':
 					$msgToLua = $this->handleCall( $msgFromLua );
 					$this->sendMessage( $msgToLua );

engines/LuaStandalone/MWServer.lua

@@ -34,21 +34,28 @@ function MWServer:execute()
 	self:debug( 'MWServer:execute: returning' )
 end
 
+-- Convert a multiple-return-value or a ... into a count and a table
+function MWServer:listToCountAndTable( ... )
+	return select( '#', ... ), { ... }
+end
+
 --- Call a PHP function
 -- Raise an error if the PHP handler requests it. May return any number
 -- of values.
 --
 -- @param id The function ID, specified by a registerLibrary message
+-- @param nargs Count of function arguments
 -- @param args The function arguments
 -- @return The return values from the PHP function
-function MWServer:call( id, args )
+function MWServer:call( id, nargs, args )
 	local result = self:dispatch( {
 		op = 'call',
 		id = id,
+		nargs = nargs,
 		args = args
 	} )
 	if result.op == 'return' then
-		return unpack( result.values )
+		return unpack( result.values, 1, result.nvalues )
 	elseif result.op == 'error' then
 		-- Raise an error in the actual user code that called the function
 		-- The level is 3 since our immediate caller is a closure
@@ -63,19 +70,22 @@ end
 -- @param message The message from PHP
 -- @return A response message to send back to PHP
 function MWServer:handleCall( message )
-	local result = { xpcall( 
+	local n, result = self:listToCountAndTable( xpcall(
 		function ()
-			return self.chunks[message.id]( unpack( message.args ) )
+			return self.chunks[message.id]( unpack( message.args, 1, message.nargs ) )
 		end,
 		function ( err )
 			return MWServer:attachTrace( err )
 		end
-		) }
+	) )
 
 	if result[1] then
-		table.remove( result, 1 )
+		-- table.remove( result, 1 ) renumbers from 2 to #result. But #result
+		-- is not necessarily "right" if result contains nils.
+		result = { unpack( result, 2, n ) }
 		return {
 			op = 'return',
+			nvalues = n - 1,
 			values = result
 		}
 	else
@@ -124,6 +134,7 @@ function MWServer:handleLoadString( message )
 		local id = self:addChunk( chunk )
 		return {
 			op = 'return',
+			nvalues = 1,
 			values = {id}
 		}
 	else
@@ -161,7 +172,7 @@ function MWServer:handleRegisterLibrary( message )
 
 	for name, id in pairs( message.functions ) do
 		t[name] = function( ... )
-			return self:call( id, { ... } )
+			return self:call( id, self:listToCountAndTable( ... ) )
 		end
 		-- Protect the function against setfenv()
 		self.protectedFunctions[t[name]] = true
@@ -169,6 +180,7 @@ function MWServer:handleRegisterLibrary( message )
 
 	return {
 		op = 'return',
+		nvalues = 0,
 		values = {}
 	}
 end
@@ -181,13 +193,14 @@ end
 function MWServer:handleWrapPhpFunction( message )
 	local id = message.id
 	local func = function( ... )
-		return self:call( id, { ... } )
+		return self:call( id, self:listToCountAndTable( ... ) )
 	end
 	-- Protect the function against setfenv()
 	self.protectedFunctions[func] = true
 
 	return {
 		op = 'return',
+		nvalues = 1,
 		values = { func }
 	}
 end
@@ -199,6 +212,7 @@ end
 function MWServer:handleGetStatus( message )
 	local nullRet = {
 		op = 'return',
+		nvalues = 0,
 		values = {}
 	}
 	local file = io.open( '/proc/self/stat' )
@@ -216,6 +230,7 @@ function MWServer:handleGetStatus( message )
 	end
 	return {
 		op = 'return',
+		nvalues = 1,
 		values = {{
 			pid = tonumber(t[1]),
 			time = tonumber(t[14]) + tonumber(t[15]) + tonumber(t[16]) + tonumber(t[17]),

engines/LuaStandalone/protocol.txt

@@ -22,7 +22,8 @@ message. In this way, a stack of pending requests can be accumulated. This
 mechanism allows re-entrant and recursive calls. 
 
 All numerically-indexed arrays should start from index 1 unless otherwise
-specified.
+specified. Note that the number of values in an array may not match what Lua's
+'#' operator returns if the array contains nils.
 
 == Request messages sent from PHP to Lua ==
 
@@ -38,6 +39,7 @@ Message parameters:
 On success, the response message is:
 
 * op: "return"
+* nvalues: 1
 * values: An array with a single element with the ID in it
 
 On failure, the response message is:
@@ -52,11 +54,13 @@ Call a Lua function.
 Message parameters:
 * op: "call"
 * id: The chunk ID
+* nargs: Number of arguments, including nils
 * args: The argument array
 
 On success, the response message is:
 
 * op: "return"
+* nvalues: Number of return values, including nils
 * values: All return values as an array
 
 On failure, the response message is:
@@ -81,6 +85,7 @@ Message parameters:
 On success, the response message is:
 
 * op: "return"
+* nvalues: 0
 * values: An empty array
 
 On failure the response message is:
@@ -98,6 +103,7 @@ Message parameters:
 On success, the response message is:
 
 * op: "return"
+* nvalues: 1
 * values: An array with a single element, which is an associative array mapping
   status key to value. The status keys are:
 ** pid: The process identifier
@@ -108,6 +114,7 @@ On success, the response message is:
 On failure, the response message is:
 
 * op: "return"
+* nvalues: 0
 * values: An empty array
 
 === quit ===
@@ -128,11 +135,13 @@ Call a PHP function.
 Message parameters:
 * op: "call"
 * id: The function ID given by registerLibrary
+* nargs: Number of arguments, including nils
 * args: An array giving the function arguments
 
 On success, the response message is:
 
 * op: "return"
+* nvalues: Number of return values, including nils
 * values: All return values as an array
 
 On failure the response message is:

tests/engines/LuaCommon/LuaInterpreterTest.php

@@ -108,6 +108,10 @@ abstract class Scribunto_LuaInterpreterTest extends MediaWikiTestCase {
 			array( array( 'x' => 'foo', 'y' => 'bar', 'z' => array() ) ),
 			array( INF ),
 			array( -INF ),
+			array( 'ok', null, 'ok' ),
+			array( null, 'ok' ),
+			array( 'ok', null ),
+			array( null ),
 		);
 	}