Module talk:Arguments
Iterator corruption
Template:Ping 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. Jackmcbarn (talk) 02:58, 13 April 2014 (UTC)
- Template:Ping That's a good find. (I assume that should be
pairs(args)on line 2 rather than justargs?) We're running into the undefined behaviour mentioned in the 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 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 Anomie would like to check the fix before we put it up live? — Mr. Stradivarius ♪ talk ♪ 16:02, 13 April 2014 (UTC)- Template:Ping 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. Jackmcbarn (talk) 17:47, 13 April 2014 (UTC)
- Template:Ping 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? 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. 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?) — Mr. Stradivarius ♪ talk ♪ 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? Jackmcbarn (talk) 18:28, 14 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?) — Mr. Stradivarius ♪ talk ♪ 05:25, 14 April 2014 (UTC)