helper.literal, .parsed etc semantics discussion

Discussion of Lua and LuaWML support, development, and ideas.

Moderators: Forum Moderators, Developers

Post Reply
Anonymissimus
Inactive Developer
Posts: 2458
Joined: August 15th, 2008, 8:46 pm
Location: Germany

helper.literal, .parsed etc semantics discussion

Post by Anonymissimus » October 21st, 2010, 9:27 pm

Ok, now it did happen to me that I accidentally "passed by reference" and thereby modified a passed wml table at the position where helper.parsed was called, although I wanted to modify only the table I got from helper.parsed. In case that userdata is passed to those functions, all of the __literal, __shallow_literal, __parsed, __shallow_parsed fields create a copy, making it impossible to accidentally modify the table at the calling position, right ? I think that in case that a table is passed it would be better to create a copy instead of returning the table itsself...
At least this is very tricky and should be mentioned in the wiki then, "advanced lua traps" or so...

Also, the [move_unit] tag, as it is now in wml-tags.lua, is already dangerous btw: I did set several attributes to nil, modifying a table at where the function is called if it has been called from lua by passing a wml table. I should instead have created a copy (but only in case that a table has been passed).

EDIT
Adding a helper.shallow_copy function makes at least sense; so that when a function wants to modify its parameter table which could be a table from some other lua code, it must create a copy with it, and that the helper.literal etc functions do not automatically create copies all of the time, even in case it's not needed.
Last edited by Anonymissimus on October 22nd, 2010, 12:44 pm, edited 1 time in total.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

silene
Posts: 1109
Joined: August 28th, 2004, 10:02 pm

Re: helper.literal, .parsed etc syntax discussion

Post by silene » October 22nd, 2010, 5:48 am

(Note that your post doesn't have anything to do with syntax, but with semantic.)

There was a tradeoff between simplicity and performances when I designed these functions and I have changed my mind a few times about them, as can be seen in the svn logs. In the end, I decided for performances: since most usages of wml actions don't reuse the parameter for subsequent actions, there is no point in preserving it. The current implementation is such that there is no inefficiency when tags are called the following way:

Code: Select all

wml_actions.do_something { foo = 17, bar = _ "Baz" }
Still, there are some cases where the same parameter will be reused over and over. Then it's up to the user to take care of preserving the parameter so that it is still there after invoking a tag. There are two ways of doing it. One is to call wesnoth.tovconfig to abstract the table away. The other is to call wesnoth.fire instead of wesnoth.wml_actions. (The two ways are in fact exactly equivalent, by definition of wesnoth.fire.) I thought I had written something to that intent in the wiki, but I can only find the part about variable substitution, not the one about parameter decay.

So [move_unit] doesn't need to be modified. And helper.shallow_copy is useful in general, but not because of the semantic of helper.literal.

Anonymissimus
Inactive Developer
Posts: 2458
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: helper.literal, .parsed etc syntax discussion

Post by Anonymissimus » October 22nd, 2010, 12:43 pm

OK, so you are suggesting that whenever some lua code calls wml_actions.something and reuses the passed table (if it's a table at all) afterwards, it should take care for that the called tag may modify the passed table ? (We could look whether it actually does, but once its code gets changed it may do and introduce a bug...)
One is to call wesnoth.tovconfig to abstract the table away.
What about a deep copy instead, would that be less efficient ? Since the intention here is not to enforce variable substitution but only to make the parameter "const".

EDIT
In some earlier post, you suggested that the called function should be reponsible for not modifying its parameters/creating a copy:
silene wrote:Either a function documents that it modifies some of its arguments, or it should perform a shallow copy of all the tables (possibly nested ones) it modifies.
For anyone trying to understand the issue I've created a small example demonstrating it:

Code: Select all

local function modify_table(t)
	t[1] = 2
end
local t = { 1, 1}
print(t[1]) -- "1"
modify_table(t)
print(t[1]) -- "2" !!
Similar issue can happen when passing a wml table to e.g. wml_actions.move_unit, and it can one make searching bugs for hours...
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

silene
Posts: 1109
Joined: August 28th, 2004, 10:02 pm

Re: helper.literal, .parsed etc syntax discussion

Post by silene » October 22nd, 2010, 4:10 pm

Anonymissimus wrote:OK, so you are suggesting that whenever some lua code calls wml_actions.something and reuses the passed table (if it's a table at all) afterwards, it should take care for that the called tag may modify the passed table ?
Yes. In other words, it should not call two tags in a row with the same tables.
Anonymissimus wrote:
One is to call wesnoth.tovconfig to abstract the table away.
What about a deep copy instead, would that be less efficient ? Since the intention here is not to enforce variable substitution but only to make the parameter "const".
Efficiency will depend on what the innermost code expects. For pure Lua tags (e.g. never passing a table to C++), the deep copy is probably faster. But if the table ends up being used as a filter, then tovconfig is faster (since that's what the C++ code will do anyway, and only the first tovconfig call is costly).
Anonymissimus wrote:In some earlier post, you suggested that the called function should be reponsible for not modifying its parameters/creating a copy:
silene wrote:Either a function documents that it modifies some of its arguments, or it should perform a shallow copy of all the tables (possibly nested ones) it modifies.
It should either be responsible or document it isn't. As I said, I thought I had documented that calling tags directly through wml_actions could cause the tables to decay.

I understand that the current situation is not ideal and I'm open to suggestions. My goal was to ensure that the straightforward way of invoking tags was also the efficient one. Perhaps an improvement would be to document that only the toplevel fields might be modified. Then a shallow copy (or creation) would be sufficient to ensure that tables have to be reused for several tag invocations. Another possibility would be to introduce a __protect field: if there is one in a table, the helper functions would actually perform the shallow copy.

Anonymissimus
Inactive Developer
Posts: 2458
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: helper.literal, .parsed etc semantics discussion

Post by Anonymissimus » October 22nd, 2010, 5:47 pm

Imo the case that wml_actions.something is called with a freshly constructed table which is not reused afterwards is much more common, like so

Code: Select all

wml_actions.move_unit({ id = "executioner", to_x = udPrisoner.x, to_y = udPrisoner.y})
Often there are variable values assigned, but they are mostly primitive types (like here; numbers, strings, nil and boolean) - these are non-pointer types and do value-assignment when used with "=" so it's uncritical. For the variable substitution part we already decided that the caller should be reponsible for deciding whether there may be variables to substitute...

Would this be an appropriate deep copy function ?
It runs into stack overflow for a table whose entry points to itsself however:
local t = { 1 }
t[2] = t
(Substitute the call to helper.deep_copy with helper.shallow_copy to allow the table to be modified.)

Code: Select all

function helper.shallow_copy(t)
	local u = {}
	for k,v in pairs(t) do
		u[k] = v
	end
	setmetatable(u, getmetatable(t))
	return u
end

function helper.deep_copy(table)
	local result = {}
	for current_key, current_value in pairs(table) do
		if type(current_value) ~= "table" then
			result[current_key] = current_value
		else
			result[current_key] = helper.deep_copy(current_value)
		end
	end
	setmetatable(result, getmetatable(table))
	return result
end


		local function modify_table(t)
			t[2][2] = 2
		end
		local function some_fun() end
		local t = { 1, { 1, some_fun} }
		wesnoth.message(tostring(t[2][2]))
		modify_table(helper.deep_copy(t))
		wesnoth.message(tostring(t[2][2]))	
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

silene
Posts: 1109
Joined: August 28th, 2004, 10:02 pm

Re: helper.literal, .parsed etc semantics discussion

Post by silene » October 28th, 2010, 6:20 am

Anonymissimus wrote:Would this be an appropriate deep copy function ?
It runs into stack overflow for a table whose entry points to itsself however:
This is fine. Note that the stack overflow (and more generally the loss of structure for cyclic objects) can be easily avoided. It would look like: (untested)

Code: Select all

function helper.deep_copy(source)
  local copied = {}
  local function doit(src)
    local dst = {}
    for k,v in pairs(src) do
      if type(v) ~= "table" then
        dst[k] = v
      else
        dst[k] = copied[v] or doit(v)
      end
    end
    copied[src] = dst
    return setmetatable(dst, getmetatable(src))
  end
  if type(source) ~= "table" then
    return source
  else
    return doit(source)
  end
end

Anonymissimus
Inactive Developer
Posts: 2458
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: helper.literal, .parsed etc semantics discussion

Post by Anonymissimus » October 30th, 2010, 3:54 pm

Your code doesn't work, that is, it doesn't prevent the stack overflow. Also I don't understand the semantics, do you want to do something like
"execute the recursion until the overflow (would) occur but at that point cease and use the previous result" ?
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

silene
Posts: 1109
Joined: August 28th, 2004, 10:02 pm

Re: helper.literal, .parsed etc semantics discussion

Post by silene » October 30th, 2010, 4:19 pm

Anonymissimus wrote:Your code doesn't work, that is, it doesn't prevent the stack overflow.
The 15th line was too late. Moving it earlier in the function fixes it. (And this time I tested it.)

Code: Select all

function helper.deep_copy(source)
  local copied = {}
  local function doit(src)
    local dst = {}
    copied[src] = dst
    for k,v in pairs(src) do
      if type(v) ~= "table" then
        dst[k] = v
      else
        dst[k] = copied[v] or doit(v)
      end
    end
    return setmetatable(dst, getmetatable(src))
  end
  if type(source) ~= "table" then
    return source
  else
    return doit(source)
  end
end
Anonymissimus wrote:Also I don't understand the semantics, do you want to do something like
"execute the recursion until the overflow (would) occur but at that point cease and use the previous result" ?
Not at all. The code just preserves the original structure. For instance, if you pass it

Code: Select all

local t = { 1 }
t[2] = t
local u = helper.deep_copy(t)
then you get a table such that the following assertion holds

Code: Select all

assert(u ~= t and u[1] == t[1] and u[2] == u)

Anonymissimus
Inactive Developer
Posts: 2458
Joined: August 15th, 2008, 8:46 pm
Location: Germany

Re: helper.literal, .parsed etc semantics discussion

Post by Anonymissimus » October 31st, 2010, 4:33 pm

I think I got it.
copied[src] = dst remembers for every encountered table a pointer to its copy.
dst[k] = copied[v] or doit(v) checks whether any value encountered, which we know is a table at that point, points to one of these tables from above. If no, copied[v] is nil and doit(v) is called, if yes, the pointer to the copy of that table is used for assignment.
Neat, actually. So the cyclic recursion is not started at all. Also strange and confusing that indices can be a table.
projects (BfW 1.12):
A Simple Campaign: campaign draft for wml startersPlan Your Advancements: mp mod
The Earth's Gut: sp campaignSettlers of Wesnoth: mp scenarioWesnoth Lua Pack: lua tags and utils
updated to 1.8 and handed over: A Gryphon's Tale: sp campaign

silene
Posts: 1109
Joined: August 28th, 2004, 10:02 pm

Re: helper.literal, .parsed etc semantics discussion

Post by silene » October 31st, 2010, 6:37 pm

Anonymissimus wrote:copied[src] = dst remembers for every encountered table a pointer to its copy.
dst[k] = copied[v] or doit(v) checks whether any value encountered, which we know is a table at that point, points to one of these tables from above. If no, copied[v] is nil and doit(v) is called, if yes, the pointer to the copy of that table is used for assignment.
Neat, actually. So the cyclic recursion is not started at all. Also strange and confusing that indices can be a table.
In fact, anything can be an index; copied[v] would have worked whether v was a table or not. For instance, if the doit function knew how to copy some other types than just tables, the inner loop could be as simple as

Code: Select all

for k,v in pairs(src) do
  dst[k] = copied[v] or doit(v)
end
Obviously, such a code would be quite inefficient, since all the encountered scalar values would end up in the copied table, e.g. copied[5] == 5. That's why my code uses the associative map only for storing tables.

Post Reply