[[ Bug 20931 ]] Bridge values in LCB assign-array and assign-list ops

runrevmark · runrevmark · commit a7125ee9a4a6 · 2018-02-01T13:27:59.000Z
This patch fixes a bug where the behavior of code built using the
[ ... ] and { ... } syntax in LCB is different from that when using
explicit code.

A new method 'Bridge' has been added to the VM's execute context.
This method performs a 'convert to optional any' on the input value
resulting in bridgeable foreign values being imported, and all other
values being retained.

This method is now called on the input values in the assign-list
and assign-array opcodes meaning that foreign values being built
into lists will bridge.

Additionally, an extra compiler check has been added to variadic
arguments (i.e. arguments to the variadic portion of a C variadic
handler). This check restricts such arguments to being variable
identifiers where the variable has an explicit type declaration.
This is necessary to ensure that code explicitly states the type
which is being passed in variadic positions as there is no other
means for the compiler or the VM to know what the type should be.
diff --git a/libscript/src/script-bytecode.hpp b/libscript/src/script-bytecode.hpp
@@ -753,7 +753,7 @@ struct MCScriptBytecodeOp_AssignList
 	
 	static void Execute(MCScriptExecuteContext& ctxt)
 	{
-		MCAutoArray<MCValueRef> t_elements;
+		MCAutoValueRefArray t_elements;
 		if (!t_elements.Resize(ctxt.GetArgumentCount() - 1))
 		{
 			ctxt.Rethrow();
@@ -762,16 +762,26 @@ struct MCScriptBytecodeOp_AssignList
 		
 		for(uindex_t t_element_idx = 0; t_element_idx < t_elements.Size(); t_element_idx++)
 		{
-			t_elements[t_element_idx] = ctxt.CheckedFetchRegister(ctxt.GetArgument(t_element_idx + 1));
-			if (t_elements[t_element_idx] == nil)
-				return;
+            MCValueRef t_raw_value = ctxt.CheckedFetchRegister(ctxt.GetArgument(t_element_idx + 1));
+            if (t_raw_value == nullptr)
+            {
+                return;
+            }
+
+            /* When assigning to a normal slot, Convert is called which bridges
+             * foreign values when being placed into an explicitly typed non-
+             * foreign slot. The case of assigning to an array element is the
+             * same, so we must explicitly bridge. */
+            if (!ctxt.Bridge(t_raw_value,
+                             t_elements[t_element_idx]))
+            {
+                return;
+            }
 		}
 		
 		MCAutoProperListRef t_list;
-		if (!MCProperListCreate(t_elements.Ptr(),
-								t_elements.Size(),
-								&t_list))
-		{
+        if (!t_elements.TakeAsProperList(&t_list))
+        {
 			ctxt.Rethrow();
 			return;
 		}
@@ -828,6 +838,7 @@ struct MCScriptBytecodeOp_AssignArray
 			{
 				return;
 			}
+
 			if (MCValueGetTypeCode(t_raw_key) != kMCValueTypeCodeString)
 			{
 				ctxt.ThrowNotAStringValue(t_raw_key);
@@ -842,17 +853,28 @@ struct MCScriptBytecodeOp_AssignArray
 				return;
 			}
 			
-			MCValueRef t_value;
-			t_value = ctxt.CheckedFetchRegister(ctxt.GetArgument(t_arg_idx + 1));
-			if (t_value == nil)
+			MCValueRef t_raw_value;
+			t_raw_value = ctxt.CheckedFetchRegister(ctxt.GetArgument(t_arg_idx + 1));
+			if (t_raw_value == nil)
 			{
 				return;
 			}
+            
+            /* When assigning to a normal slot, Convert is called which bridges
+             * foreign values when being placed into an explicitly typed non-
+             * foreign slot. The case of assigning to an array element is the
+             * same, so we must explicitly bridge. */
+            MCAutoValueRef t_value;
+            if (!ctxt.Bridge(t_raw_value,
+                             &t_value))
+            {
+                return;
+            }
 			
 			if (!MCArrayStoreValue(*t_array,
 								   false,
 								   *t_key,
-								   t_value))
+								   *t_value))
 			{
 				ctxt.Rethrow();
 				return;
diff --git a/libscript/src/script-execute.cpp b/libscript/src/script-execute.cpp
@@ -888,6 +888,47 @@ MCScriptExecuteContext::InvokeForeign(MCScriptInstanceRef p_instance,
 	}
 }
 
+bool
+MCScriptExecuteContext::Bridge(MCValueRef p_value,
+                               MCValueRef& r_output_value)
+{
+    // Get resolved typeinfo for the value.
+    MCResolvedTypeInfo t_resolved_type;
+    if (!ResolveTypeInfo(MCValueGetTypeInfo(p_value),
+                         t_resolved_type))
+    {
+        return false;
+    }
+ 
+    // Get the foreign type descriptor for the value's type, if any.
+    const MCForeignTypeDescriptor *t_desc = nullptr;
+    if (MCTypeInfoIsForeign(t_resolved_type.type))
+    {
+        t_desc = MCForeignTypeInfoGetDescriptor(t_resolved_type.type);
+    }
+    
+    // If the type is not foreign or is not bridgeable foreign, just retain;
+    // otherwise use doimport.
+    if (t_desc == nullptr ||
+        t_desc->doimport == nullptr)
+    {
+        r_output_value = MCValueRetain(p_value);
+    }
+    else
+    {
+        if (!t_desc->doimport(t_desc,
+                              MCForeignValueGetContentsPtr(p_value),
+                              false,
+                              r_output_value))
+        {
+            Rethrow();
+            return false;
+        }
+    }
+    
+    return true;
+}
+
 bool
 MCScriptExecuteContext::Convert(MCValueRef p_value,
 								MCTypeInfoRef p_to_type,
diff --git a/libscript/src/script-execute.hpp b/libscript/src/script-execute.hpp
@@ -179,7 +179,16 @@ class MCScriptExecuteContext
 	// Leave the LCB VM. If a execution completed successfully then true is
 	// returned, otherwise false is returned.
 	bool Leave(void);
-	
+    
+    // Attempt to bridge the input value. If the input value is foreign and
+    // the foreign type is bridgeable, doimport is used to create the output
+    // value. Otherwise, the input value is retained and returned as the output
+    // value.
+    // Note: This is a specialization of Convert where the to_type is 'optional
+    // any'.
+    bool Bridge(MCValueRef value,
+                MCValueRef& r_bridged_value);
+    
 	// Attempt to convert the input value to the specified type. If the conversion
 	// cannot be performed because the type does not conform, then 'true' will
 	// be returned, but r_new_value will be nil; allowing the context to throw
diff --git a/toolchain/lc-compile/src/check.g b/toolchain/lc-compile/src/check.g
@@ -1463,7 +1463,7 @@
 
     -- variadic parameter 'sticks' and matches the rest of the argument list
     'rule' CheckCallArguments(Position, ParamRest:parameterlist(parameter(_, variadic, _, _), _), expressionlist(Argument, ArgRest)):
-        CheckExpressionIsEvaluatable(Argument)
+        CheckExpressionIsExplicitlyTypedVariable(Argument)
         CheckCallArguments(Position, ParamRest, ArgRest)
 
     'rule' CheckCallArguments(Position, parameterlist(parameter(_, variadic, _, _), _), nil):
@@ -1507,6 +1507,20 @@
         |]
         CheckInvokeArguments(Position, SigTail, Arguments)
 
+'action' CheckExpressionIsExplicitlyTypedVariable(EXPRESSION)
+
+    'rule' CheckExpressionIsExplicitlyTypedVariable(slot(Position, Id)):
+        -- Only expressions binding to a slot which has a specified type are
+        -- 'explicitly typed'
+        QueryKindOfSymbolId(Id -> variable)
+        QuerySymbolId(Id -> Info)
+        Info'Type -> Type
+        ne(Type, unspecified)
+
+    'rule' CheckExpressionIsExplicitlyTypedVariable(Expr):
+        GetExpressionPosition(Expr -> Position)
+        Error_VariadicArgumentNotExplicitlyTyped(Position)
+
 'action' CheckExpressionIsEvaluatable(EXPRESSION)
 
     'rule' CheckExpressionIsEvaluatable(invoke(Position, Info, Arguments)):
diff --git a/toolchain/lc-compile/src/support.g b/toolchain/lc-compile/src/support.g
@@ -357,6 +357,7 @@
     Error_InvalidNameForNamespace
     Error_VariadicParametersOnlyAllowedInForeignHandlers
     Error_VariadicParameterMustBeLast
+    Error_VariadicArgumentNotExplicitlyTyped
 
     Warning_MetadataClausesShouldComeAfterUseClauses
     Warning_DeprecatedTypeName
@@ -788,6 +789,7 @@
 
 'action' Error_VariadicParametersOnlyAllowedInForeignHandlers(Position: POS)
 'action' Error_VariadicParameterMustBeLast(Position: POS)
+'action' Error_VariadicArgumentNotExplicitlyTyped(Position: POS)
 
 'action' Warning_MetadataClausesShouldComeAfterUseClauses(Position: POS)
 'action' Warning_DeprecatedTypeName(Position: POS, NewType: STRING)
diff --git a/toolchain/libcompile/src/report.c b/toolchain/libcompile/src/report.c
@@ -403,6 +403,7 @@ DEFINE_ERROR_I(UnsafeHandlerCallNotAllowedInSafeContext, "Unsafe handler '%s' ca
 
 DEFINE_ERROR(VariadicParameterMustBeLast, "Variadic parameter must be the last")
 DEFINE_ERROR(VariadicParametersOnlyAllowedInForeignHandlers, "Variadic parameters only allowed in foreign handlers")
+DEFINE_ERROR(VariadicArgumentNotExplicitlyTyped, "Variadic arguments must be an explicitly-typed variable")
 
 #define DEFINE_WARNING(Name, Message) \
     void Warning_##Name(intptr_t p_position) { _Warning(p_position, Message); }
