Merge pull request livecode#6310 from runrevmark/bugfix-20931

livecodepanos · web-flow · commit 07474c77ffd1 · 2018-06-06T14:22:16.000+01:00
[[ Bug 20931 ]] Bridge values in LCB assign-array and assign-list ops
diff --git a/docs/guides/LiveCode Builder Language Reference.md b/docs/guides/LiveCode Builder Language Reference.md
@@ -1062,7 +1062,8 @@ Result expressions are not assignable.
       : '[' [ <Elements: ExpressionList> ] ']'
 
 A list expression evaluates all the elements in the expression list from
-left to right and constructs a list value with them as elements.
+left to right and constructs a list value with them as elements. Each 
+expression is converted to `optional any` when constructing the list.
 
 The elements list is optional, so the empty list can be specified as
 *[]*.
@@ -1080,7 +1081,8 @@ List expressions are not assignable.
 
 An array expression evaluates all of the key and value expressions
 from left to right, and constructs an **Array** value as appropriate.
-Each key expression must evaluate to a **String**.
+Each key expression must evaluate to a **String**. Each value expression 
+is converted to `optional any` when constructing the array.
 
 The contents are optional, so the empty array can be written as `{}`.
 
diff --git a/docs/lcb/notes/20931.md b/docs/lcb/notes/20931.md
@@ -0,0 +1,9 @@
+# LiveCode Builder Virtual Machine
+## Array and list assign ops
+
+Previously there was a difference between constructing a list or array
+using `push` or `put` and using list or array assigment expressions `[]` 
+and `{}`, namely values were converted to `optional any` only in the 
+latter case. For consistency, they are now converted in both cases.
+
+# [20931] Values should bridge to optional any in array and list assign
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/tests/lcb/compiler/frontend/variadic.compilertest b/tests/lcb/compiler/frontend/variadic.compilertest
@@ -42,14 +42,69 @@ end module
 %ERROR "Variadic parameters only allowed in foreign handlers" AT BEFORE_VARIADIC
 %ENDTEST
 
+%% Variadic arguments must be an explicitly-typed variable
+%TEST VariadicNotAllowedImplicitlyTypedArgument
+module compiler_test
+__safe foreign handler TestVariadic(in pA as any, ...) returns nothing binds to ""
+handler CallVariadic()
+	TestVariadic("pA", %{BEFORE_IMPLICIT_ARG}1)
+end handler
+end module
+%EXPECT PASS
+%ERROR "Variadic arguments must be an explicitly-typed variable" AT BEFORE_IMPLICIT_ARG
+%ENDTEST
+
+%% Variadic arguments can be local variables
+%TEST VariadicArgumentModuleLocal
+module compiler_test
+__safe foreign handler TestVariadic(in pA as any, ...) returns nothing binds to ""
+handler CallVariadic()
+	variable tVar as Integer
+	TestVariadic("pA", tVar)
+end handler
+end module
+%EXPECT PASS
+%SUCCESS
+%ENDTEST
+
+%% Variadic arguments can be module locals
+%TEST VariadicArgumentModuleLocal
+module compiler_test
+private variable mVar as Integer
+__safe foreign handler TestVariadic(in pA as any, ...) returns nothing binds to ""
+handler CallVariadic()
+	TestVariadic("pA", mVar)
+end handler
+end module
+%EXPECT PASS
+%SUCCESS
+%ENDTEST
+
+%% Variadic arguments can be parameter variables
+%TEST VariadicArgumentParameterVariable
+module compiler_test
+__safe foreign handler TestVariadic(in pA as any, ...) returns nothing binds to ""
+handler CallVariadic(in pVar as Integer)
+	TestVariadic("pA", pVar)
+end handler
+end module
+%EXPECT PASS
+%SUCCESS
+%ENDTEST
+
 %% You can have 0 to any number of variadic arguments
 %TEST VariadicParametersAnyCount
 module compiler_test
 __safe foreign handler TestVariadic(in pA as any, ...) returns nothing binds to ""
 handler CallVariadic()
-	TestVariadic(1)
-	TestVariadic(1, 2)
-	TestVariadic(1, 2, 3)
+	TestVariadic("pA")
+
+	variable tX as Integer
+	variable tY as Integer
+	variable tZ as Integer
+	TestVariadic("pA", tX)
+	TestVariadic("pA", tX, tY)
+	TestVariadic("pA", tX, tY, tZ)
 end handler
 end module
 %EXPECT PASS
diff --git a/tests/lcb/vm/assign-ops.lcb b/tests/lcb/vm/assign-ops.lcb
@@ -0,0 +1,48 @@
+module __VMTEST.assign_ops
+
+use com.livecode.foreign
+
+foreign handler MCValueGetTypeInfo(in pValue as Pointer) returns Pointer binds to "<builtin>"
+
+foreign handler MCTypeInfoIsForeign(in pTypeInfo as Pointer) returns CBool binds to "<builtin>"
+
+foreign handler MCArrayFetchValue(in pArray as Array, in pCaseSensitive as CBool, in pKey as Pointer, out rValue as Pointer) returns CBool binds to "<builtin>"
+
+foreign handler MCNameCreate(in pString as String, out rName as Pointer) returns CBool binds to "<builtin>"
+
+foreign handler MCProperListFetchElementAtIndex(in pList as List, in pIndex as LCUIndex) returns Pointer binds to "<builtin>"
+
+unsafe handler __IsForeignValue(in pValue as Pointer) returns Boolean
+	return MCTypeInfoIsForeign(MCValueGetTypeInfo(pValue))
+end handler
+
+public handler TestAssignArrayOpForeignBridge()
+	unsafe
+		variable tVar as CBool
+		put false into tVar
+
+		variable tKey as Pointer
+		MCNameCreate("key", tKey)
+	
+		variable tValue as Pointer
+		MCArrayFetchValue({"key": tVar}, false, tKey, tValue)
+
+		test "foreign value bridged to optional any in array assign" \
+			when not __IsForeignValue(tValue)
+	end unsafe
+end handler
+
+public handler TestAssignListOpForeignBridge()
+	unsafe
+		variable tVar as CBool
+		put false into tVar
+	
+		variable tValue as Pointer	
+		put MCProperListFetchElementAtIndex([tVar], 0) into tValue
+	
+		test "foreign value bridged to optional any in list assign" \
+			when not __IsForeignValue(tValue)
+	end unsafe
+end handler
+
+end module
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,33 @@
         |]
         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 -> Kind)
+
+        -- Expression must be a variable of some kind, i.e. one of:
+        (|
+            -- A local variable
+            eq(Kind, local)
+        ||
+            -- A parameter variable
+            eq(Kind, parameter)
+        ||
+            -- A module local variable
+            eq(Kind, 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); }
