From a7125ee9a4a6285eded84cb5591cd3119bc85e09 Mon Sep 17 00:00:00 2001 From: Mark Waddingham Date: Thu, 1 Feb 2018 13:17:35 +0000 Subject: [PATCH 1/7] [[ Bug 20931 ]] Bridge values in LCB assign-array and assign-list ops 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. --- libscript/src/script-bytecode.hpp | 46 ++++++++++++++++++++++-------- libscript/src/script-execute.cpp | 41 ++++++++++++++++++++++++++ libscript/src/script-execute.hpp | 11 ++++++- toolchain/lc-compile/src/check.g | 16 ++++++++++- toolchain/lc-compile/src/support.g | 2 ++ toolchain/libcompile/src/report.c | 1 + 6 files changed, 103 insertions(+), 14 deletions(-) diff --git a/libscript/src/script-bytecode.hpp b/libscript/src/script-bytecode.hpp index 9d787add89b..d0992f83358 100644 --- 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 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 index f1b0182084b..9243efef6cd 100644 --- 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 index f1073d62d6e..1085404b85f 100644 --- 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 index 7b144cd8a1f..b2aa1454c26 100644 --- 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 index 6e614ca639f..127636ead8b 100644 --- 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 index adc42bcf337..430cb9d1bd7 100644 --- 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); } From 9ab9b12d8e0268028d35ca986c04676b743f900c Mon Sep 17 00:00:00 2001 From: livecodeali Date: Thu, 8 Mar 2018 08:45:54 +0000 Subject: [PATCH 2/7] [[ Bug 20931 ]] Cover the various types of 'variable' symbol kind --- toolchain/lc-compile/src/check.g | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/toolchain/lc-compile/src/check.g b/toolchain/lc-compile/src/check.g index b2aa1454c26..b93b7428a41 100644 --- a/toolchain/lc-compile/src/check.g +++ b/toolchain/lc-compile/src/check.g @@ -1512,7 +1512,20 @@ 'rule' CheckExpressionIsExplicitlyTypedVariable(slot(Position, Id)): -- Only expressions binding to a slot which has a specified type are -- 'explicitly typed' - QueryKindOfSymbolId(Id -> variable) + 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) From 2e50bc2e8a650ee0dc39bbbab779067f1e152b3d Mon Sep 17 00:00:00 2001 From: livecodeali Date: Thu, 8 Mar 2018 09:06:07 +0000 Subject: [PATCH 3/7] [[ Bug 20931 ]] Add tests for explicitly typed variadic args --- .../compiler/frontend/variadic.compilertest | 61 ++++++++++++++++++- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/tests/lcb/compiler/frontend/variadic.compilertest b/tests/lcb/compiler/frontend/variadic.compilertest index b6007049246..27ef45b89a2 100644 --- 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 From 409a6283fd9cfdca8a1470047f250d82c14243fc Mon Sep 17 00:00:00 2001 From: livecodeali Date: Thu, 8 Mar 2018 12:34:26 +0000 Subject: [PATCH 4/7] [[ Bug 20931 ]] Add tests for array and list assign op bridging --- tests/lcb/vm/assign-ops.lcb | 48 +++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 tests/lcb/vm/assign-ops.lcb diff --git a/tests/lcb/vm/assign-ops.lcb b/tests/lcb/vm/assign-ops.lcb new file mode 100644 index 00000000000..1132d15e332 --- /dev/null +++ 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 "" + +foreign handler MCTypeInfoIsForeign(in pTypeInfo as Pointer) returns CBool binds to "" + +foreign handler MCArrayFetchValue(in pArray as Array, in pCaseSensitive as CBool, in pKey as Pointer, out rValue as Pointer) returns CBool binds to "" + +foreign handler MCNameCreate(in pString as String, out rName as Pointer) returns CBool binds to "" + +foreign handler MCProperListFetchElementAtIndex(in pList as List, in pIndex as LCUIndex) returns Pointer binds to "" + +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], 1) into tValue + + test "foreign value bridged to optional any in list assign" \ + when not __IsForeignValue(tValue) + end unsafe +end handler + +end module From 545fef55b45842dc54ac5156f9a7ca22024f5cc6 Mon Sep 17 00:00:00 2001 From: livecodeali Date: Thu, 8 Mar 2018 17:16:07 +0000 Subject: [PATCH 5/7] [[ Bug 20931 ]] Update LCB language reference --- docs/guides/LiveCode Builder Language Reference.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/guides/LiveCode Builder Language Reference.md b/docs/guides/LiveCode Builder Language Reference.md index 66e85dcdfb5..708fe7a2aa7 100644 --- a/docs/guides/LiveCode Builder Language Reference.md +++ b/docs/guides/LiveCode Builder Language Reference.md @@ -1060,7 +1060,8 @@ Result expressions are not assignable. : '[' [ ] ']' 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 *[]*. @@ -1078,7 +1079,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 `{}`. From d973160216480ccd0fb13bae5168abdb66ca3ccd Mon Sep 17 00:00:00 2001 From: livecodeali Date: Thu, 8 Mar 2018 17:26:21 +0000 Subject: [PATCH 6/7] [[ Bug 20931 ]] Add release note --- docs/lcb/notes/20931.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 docs/lcb/notes/20931.md diff --git a/docs/lcb/notes/20931.md b/docs/lcb/notes/20931.md new file mode 100644 index 00000000000..eb19358a062 --- /dev/null +++ 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 \ No newline at end of file From 615a87b9b1db006e61b6d34d44e53ac78f7477c1 Mon Sep 17 00:00:00 2001 From: Ali Lloyd Date: Fri, 1 Jun 2018 22:01:46 +0100 Subject: [PATCH 7/7] [[ Bug 20931 ]] Use correct list index --- tests/lcb/vm/assign-ops.lcb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/lcb/vm/assign-ops.lcb b/tests/lcb/vm/assign-ops.lcb index 1132d15e332..15f55faa9b7 100644 --- a/tests/lcb/vm/assign-ops.lcb +++ b/tests/lcb/vm/assign-ops.lcb @@ -38,7 +38,7 @@ public handler TestAssignListOpForeignBridge() put false into tVar variable tValue as Pointer - put MCProperListFetchElementAtIndex([tVar], 1) into tValue + put MCProperListFetchElementAtIndex([tVar], 0) into tValue test "foreign value bridged to optional any in list assign" \ when not __IsForeignValue(tValue)