Editing Module talk:Arguments (section)

== Iterator corruption ==
{{ping|Mr. Stradivarius}} I found a subtle iterator corruption bug in this module.<syntaxhighlight lang="lua">
local args = require('Module:Arguments').getArgs(frame)
for k, v in args do
 mw.log(k .. '=' .. (v or 'nil') .. ' ')
 if args[k .. 'somesuffix'] then
  mw.log('Found suffix for ' .. k)
 end
end
</syntaxhighlight> Attempting to read the somesuffix argument causes it to be memoized, adding it to the internal table, which apparently can corrupt the iterator and causes some arguments to be skipped. I've noticed this is only reproducible some of the time. [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 02:58, 13 April 2014 (UTC)

:{{ping|Jackmcbarn}} That's a good find. (I assume that should be <code>pairs(args)</code> on line 2 rather than just <code>args</code>?) We're running into the undefined behaviour mentioned in the [[mw:Extension:Scribunto/Lua reference manual#next|next function docs]]: "''Behavior is undefined if, when using next for traversal, any non-existing key is assigned a value.''" The way that the __pairs metamethod works in this module means that all the existing arguments will have been memoized before the user gets a chance to index the args table. So if a user queries an existing argument during the pairs iteration, there will be no problem, as it will already be present in the metaArgs table. The error occurs when the user queries a non-existing argument. The __index function is set up to memoize this in metaArgs as nilArg, a blank table. That means that it is possible to add these blank tables as new values to the metaArgs table, even after all the non-nil values have been copied over from the frame objects. I've put a [https://en.wikipedia.org/w/index.php?title=Module:Arguments/sandbox&diff=604023029&oldid=590465333 fix in the sandbox] for this that uses the metatable.donePairs flag to check whether or not the arguments have been copied across. If they have already been copied across, then the __index metamethod won't memoize nils at all. While this fixes the bug, not memoizing the nils might cause adverse performance for some modules. Take a look and see what you think. Also, maybe [[User:Anomie|Anomie]] would like to check the fix before we put it up live? — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 16:02, 13 April 2014 (UTC)
::{{ping|Mr. Stradivarius}} Yes, that should have been pairs(args). What about a flag that gets set while you're inside the pairs method, and while it's set, it memoizes nils to some other table, then when the flag gets unset, it moves them to where they really go? Also, related, if an argument is an empty string, it gets iterated over even if empty strings get converted to nils, which is unexpected. [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 17:47, 13 April 2014 (UTC)
::{{ping|Mr. Stradivarius}} I realized it's impossible for an iterator function to tell when it stops iterating (since the function calling it can return early, etc.), so that idea was out. Instead, I changed the way nils are memoized. They go to a different table now, which should solve that problem and the other problem at the same time. Thoughts? [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 23:14, 13 April 2014 (UTC)
::Once I got that implemented, I had another idea. Once pairs runs, we don't need to worry about memoizing at all anymore, because everything from argTables we'll ever look at is already part of metaArgs at that point. [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 23:51, 13 April 2014 (UTC)
:::I think we should memoize after pairs runs, because users might query new keys that have nil values, and also because memoizing things the same way every time is simpler. I like your idea of using a nilArgs table rather than just putting a blank table in metaArgs. That will solve the iterator problem and allow us to use the same memoization scheme whether we have used pairs or not. Also, blank strings shouldn't be iterated over unless they are explicitly allowed, due to the way the mergeArgs function works (unless you found a bug in that as well?) — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 05:25, 14 April 2014 (UTC)
::::After running pairs, though, you don't need to check argTables anymore, so it's not worth memoizing nil to nilArg, since you can just return nil either way. Won't the code in the sandbox right now work right? [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 18:28, 14 April 2014 (UTC)
:::::Ah yes, you're quite right. I wasn't registering the fact that the new check meant that we bypassed the argTables check. I've added a comment and updated the module - hopefully everything should work now. — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 08:12, 15 April 2014 (UTC)
::::::{{ping|Jackmcbarn}} Oops - we have been forgetting the problem of arguments being iterated over even if they are empty strings which get converted to nils. This would be solved by a nilArgs table, but is still present in the current version. I'll try and switch back to the nilArgs table version while keeping the formatting. — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 19:47, 15 April 2014 (UTC)
:::::::{{ping|Mr. Stradivarius}} Now that I think about nilArgs, I don't really like it since it's an extra table lookup. Maybe if nilArg is found while iterating, just skip it and go on to the next element (or change all nilArg to nil once we're in pairs). [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 19:49, 15 April 2014 (UTC)
{{od|7}} I've implemented the nilArgs version in the sandbox. I think it is quite an elegant solution, despite being an extra table lookup. Skipping nilArg tables while iterating isn't easy, as we would need to implement an iterator inside of an iterator for each of __pairs and __ipairs. And changing all nilArg tables to nil once we are in pairs would mean we would have to run pairs on metaArgs after running mergeArgs to catch all of the nilArg tables that have been introduced by __index and __newindex. Using nilArgs to memoize avoids these problems and makes the code quite a bit shorter (take a look at the new __pairs and __ipairs functions). — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 20:24, 15 April 2014 (UTC)
:{{ping|Mr. Stradivarius}} Okay, I guess I'm sold on it. I think I see a few subtle bugs, though; let me see if I can track them down. [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 20:35, 15 April 2014 (UTC)
::Thanks for taking a look at it. If I have time tomorrow, I may rewrite the test cases in [[mw:Extension:Scribunto/Lua reference manual#frame:newChild|the way foretold in the fine manual]]. That should make tracking these subtle bugs slightly less hit-and-miss. — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 20:59, 15 April 2014 (UTC)
::Also, I [https://en.wikipedia.org/w/index.php?title=Module%3AArguments%2Fsandbox&diff=604357645&oldid=604349477 found a bug] in my code: __newindex wasn't properly overwriting nil arguments in metaArgs, which would have caused problems for both __pairs and __index. — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 21:11, 15 April 2014 (UTC)
:::{{ping|Jackmcbarn}} I've finished rewriting [[Module:Arguments/testcases]], and I've also added some bad input tests and some iterator tests. I've tried my best to break it, but all the tests have passed so far. As expected, the main module fails four of the iterator tests. Are there any other ways you can think to break it? If not, I think it is time to update the main module. — '''''[[User:Mr. Stradivarius|<span style="color: #194D00; font-family: Palatino, Times, serif">Mr. Stradivarius</span>]]''''' <sup>[[User talk:Mr. Stradivarius|♪ talk ♪]]</sup> 13:10, 17 April 2014 (UTC)
::::{{ping|Mr. Stradivarius}} Looks good. I did add one extra check for performance reasons. [[User:Jackmcbarn|Jackmcbarn]] ([[User talk:Jackmcbarn|talk]]) 18:47, 17 April 2014 (UTC)