[Lua datamodel] Function 'evalAsData' requires correction

[alexzhornyak]

Hello, 1)I am not fully satisfied that content executes a couple of times. You are trying to do everything by 1 function. Why not to create separate functions such as evalExpr, evalScript?

2)Imagine, that we have very difficult script and trying to debug it.

<scxml datamodel="lua" initial="Main" name="ScxmlShape1" version="1.0" xmlns="http://www.w3.org/2005/07/scxml">
    <state id="Main">
        <onentry>
            <script>local s_text=&quot;xxx&quot;

-- a lot of code

validVar = &quot;zzz&quot;
nonValidVar
            </script>
        </onentry>
        <transition event="error.*" target="FinalShape1"/>
    </state>
    <final id="FinalShape1"/>
</scxml>

After executing its content we see next message: "cause": "[string \"return(local s_text=\"xxx\"...\"]:1: unexpected symbol near 'local'", And we can not understand, what's happend in real.

But if we called it without 'return' we would see next: "[string \"local s_text=\"xxx\"...\"]:8: '=' expected near '<eof>'" In this case we can very easy find where the problem is

[sradomski]

Hmm, I dislike the idea to disregard the return value and I also dislike the idea of two distinct functions that evaluate valid Lua statements and expressions (i.e. your evalAsScript). Merely evaluating these expressions, however, will not push their return value onto the stack. I am open to suggestions within the LuaDataModel but would, very much, like to retain the public API.

I'll give it some more thoughts.

[alexzhornyak]

You tell that "I dislike the idea to disregard the return value", but you actually disregard it in processScript function, so why is it not correct to inform DataModel about it?

void BasicContentExecutor::processScript(XERCESC_NS::DOMElement* content) {
    // download as necessary
    std::string scriptContent(X(content->getTextContent()));
    _callbacks->evalAsData(scriptContent);
}

[sradomski]

I meant to disregard the return type via luaEval.

Can you please, motivate once more, why evalAsBool or evalAsData and ignoring the return type as is done now is insufficient?

[alexzhornyak]

Ok, I will use charts for better understanding :)

1) Your variant

2) My variant

[alexzhornyak]

So, my motivation is that in present code there are next issues: 1) weak performance (same code executes 3 times and execution of unnecessary code for this operation) 2) wrong handling of errors 3) possible execution of incorrect scripts as valid

[alexzhornyak]

Look, how can I greatly reduce LuaDataModel::assign function with evalAsScript! Compare it with your variant! P.S. Your variant of this function has en error, when we are using nested tables! I will describe it in separate issue #128.

void LuaDataModel::assign(const std::string& location, const Data& data, const std::map<std::string, std::string>& attr) {
    if (location.length() == 0) {
        ERROR_EXECUTION_THROW("Assign element has neither id nor location");
    }

    // flags on attribute are ignored?
    if (location.compare("_sessionid") == 0) // test 322
        ERROR_EXECUTION_THROW("Cannot assign to _sessionId");
    if (location.compare("_name") == 0)
        ERROR_EXECUTION_THROW("Cannot assign to _name");
    if (location.compare("_ioprocessors") == 0)  // test 326
        ERROR_EXECUTION_THROW("Cannot assign to _ioprocessors");
    if (location.compare("_invokers") == 0)
        ERROR_EXECUTION_THROW("Cannot assign to _invokers");
    if (location.compare("_event") == 0)
        ERROR_EXECUTION_THROW("Cannot assign to _event");

    if (data.node) {
        ERROR_EXECUTION_THROW("Cannot assign xml nodes in lua datamodel");

    } else {

        luabridge::LuaRef lua = getDataAsLua(_luaState, data);

        luabridge::setGlobal(_luaState, lua, "__tmpAssign");
        evalAsScript(location + "= __tmpAssign");
    }
}

[sradomski]

Hehe, it might look absurd .. but there was a perfectly valid reason for it and iirc, it had to do with undeclared variables in foreach elements. I will investigate as soon as I have some more time.

[alexzhornyak]

With my code undeclared variables will definetely detected, I've checked w3c\lua\test286.scxml

[Error] 
  Platform Event 
    "name": error.execution
    "data": {
      "cause": "[string \"foo.bar.baz = __tmpAssign\"]:1: attempt to index global 'foo' (a nil value)", 
      "file":  "G:\\SDK\\uscxml_2_0\\src\\uscxml\\plugins\\datamodel\\lua\\LuaDataModel.cpp", 
      "line":  73
    }
Entering: s0
Platform Event: error.execution
Exiting: s0
Entering: pass
[Log] Outcome: "pass"

w3c\lua\test311.scxml

Entering: s0
[Error]   Platform Event 
    "name": error.execution
    "data": {
      "cause": "[string \"foo.bar.baz = __tmpAssign\"]:1: attempt to index global 'foo' (a nil value)", 
      "file":  "G:\\SDK\\uscxml_2_0\\src\\uscxml\\plugins\\datamodel\\lua\\LuaDataModel.cpp", 
      "line":  73
    }
Entering: s0
Platform Event: error.execution
Exiting: s0
Entering: pass
[Log] Outcome: "pass"

[sradomski]

If all your changes are local to LuaDataModel.cpp and the tests are still passed, by all means - send a push request. Would you need changes in the public API of DataModel? If that's the case, can we separate them from the changes to LuaDataModel.cpp for now, because there are other users relying on the current API.

[alexzhornyak]

I spent a couple of hours and invented the best variant (how I thought) of evalAsData function handling processing of script and processing of expression in one function (written in Lua). And even this "best variant", which works in 99% can bring serious mistake. I finally understood why script must be in separate function and why it MUSTN'T BE EXECUTED MORE THAN 1 TIME Suppose that we increment some data in script or add some value to table, etc. So, now I'm 100% sure that for Lua (I didn't check other models, maybe there is the same problem) we need two separate functions EvalAsData, EvalAsScript. If it is impossible to implement it in DataModel API, I will have to stay in parallel branch.

function EvalAsData(s_expr)
    print("Full expression [" .. s_expr .. "]")    
    local res = nil
    --loadstring will only check for syntax error without execution
    local res_script,err_script = load(s_expr) --for 5.1 loadstring

    if res_script then
        --this is 90% script, but there are couple of exceptions
        --for example: 'tostring(5555)'
        res = res_script()
    end

    if res==nil then
        print("2) eval now with return")
        res_expr,err_expr = load("return("..s_expr..")")
        if res_expr~=nil then
            print("This is expr") --PASS
            return res_expr()
        else
            -- if syntax was previously good
            if res_script then
               print("This is script") --PASS  
               return nil
            end
        end
    else
        return res
    end
    -- if we come here, this is syntax error  
    print(string.format("Syntax error is here! "..
          "if script=%s,"..
          "if expr=%s",err_script,err_expr)) 
    return nil    
end

t_variants = {
 [[5]], --pass
 [[25.555]], --pass
 [[function() print("Special error") return 1..b end]], --pass (correct handling of error)
 [["testing string value"]], --pass
 [[{ {0,0}, {1,1} }]], --pass
 [[tostring(555555)]], --pass
 [[type(25.55)]], --pass
 [[nil]], --pass
 [[b=2]], --pass
 [[function XXX() print("You mustn't see it here now!!!") end]], --pass
 [[XXX()]], --pass (now you can see it)
 [[35+45]], --pass (result is 80)
 [[b-2]], --pass (result is 0)
 [[t_XXXXX={ {0,0}, {1,1}, {2,2}, {3,3} }]], --pass
 "t_XXXXX[2][1]", --pass
 [[print("1.table size="..#t_XXXXX)]], --pass (size is 4)
 [[table.insert(t_XXXXX, {4,4})]], --FAIL!!! absolutely wrong!!! Data inserted twice!!!
 [[print("2.table size="..#t_XXXXX)]] --TABLE SIZE IS 6, but must be 5
}

for k,v in pairs(t_variants) do
    print("*************************")
    print("Variant " .. k)
    print("Result:[" .. tostring(EvalAsData(v)) .. "]")
end

[sradomski]

I see the point, and here is a minimal intrusive change to solve it. First, please call the function eval as the suffix qualifies the return type which cannot be overridden (evalAsData returns a Data object, evalAsBool a boolean, but evalAsScript won't return a Script). Then, implement eval as a virtual but non-abstract function in the base class as follows:

// we need an explicit 'eval' function for the LuaDataModel, see issue 118
virtual void eval(const std::string& expr) {
   evalAsData(expr);
}

and override it in the LuaDataModel only. Then adapt the BasicContentExecutor to call eval in place of evalAsData at the respective place. This way, we won't have to implement it in every other datamodel but can rely on the default implementation of the common base class to call evalAsData as it is the case now and you can override it for the LuaDataModel. Please send a pull request if this works or comment again if it does not.

P.S.: The other datamodels ought to be fine, but I will check.

[alexzhornyak]

Understood

[sradomski]

And feel free to add yourself to the copyright notices of the files you edit! See doxygen manual on multiple authors.