Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #532 by sorting table keys when processing content #974

Merged
merged 4 commits into from
Jul 23, 2020
Merged

Conversation

simoncozens
Copy link
Member

  • This could do with a test.
  • I tried to find other examples where we are using pairs() on AST content, but didn't see any. A review would be helpful.

@alerque
Copy link
Member

alerque commented Jul 22, 2020

This fixes #972, but not #532. Its likely the same problem but since it doesn't uses inputfilter there is another case somewhere. I'm trying to track it down.

@alerque
Copy link
Member

alerque commented Jul 22, 2020

I take back what I said, this does seem to fix #532. The project in question there has an elaborate work around for #355 that basically pre-processes everything with input filter fixing potential hyphenation issues at apostrophes. That does explain why it turned up regularly in a few projects and never in others.

As for tests ... I'm not to confident we have a way to reliably test non-determinism. The most likely candidate would be a Busted test unit that iterated over something a bunch of times and make sure it was the same every time. Hmmmm, yes let me try that....

@alerque
Copy link
Member

alerque commented Jul 22, 2020

I haven't come up a test that fails using pairs() yet.

Here'se what tried (plain in spec/utilities_spec.lua and run with busted -t utilities):

SILE = require("core/sile")

describe("The #utilities library ", function()
  it("should have a short alias", function() assert.is.equal(SU, SILE.utilities) end)

  describe("sortedpairs() function", function()
    local input = { "a", _ = true, "c", "b", foo = "bar", "E" }
    it("avoid Lua’s non-determinism", function ()
      local testit = function ()
        for _ = 1, 1000 do
          local i = 1
          -- for k, v in SU.sortedpairs(input) do
          for k, v in pairs(input) do
            if type(k) == "number" and k ~= i then return false
            elseif k == 1 and v ~= "a" then return false
            elseif k == 2 and v ~= "c" then return false
            elseif k == 3 and v ~= "b" then return false
            elseif k == 4 and v ~= "E" then return false
            elseif k == "_" and v ~= true then return false
            elseif k == "foo" and v ~= "bar" then return false
            end
            i = i + 1
          end
        end
        return true
      end
      assert.is.truthy(testit())
    end)
  end)

end)

@alerque
Copy link
Member

alerque commented Jul 23, 2020

So I've still completely failed to create a test that reproduces the problem. It's hard to test that you're doing it right when you can't get a test you know in wrong to reliably fail as a starting point.

However I did some digging and came up with a few more instances of pairs() that either are or could be problematic. Some of them likely don't matter but technically were out of whack (defining hyphenation definition patterns in order, applying them randomly) others could clearly have tripped us up without warning (outputYourself() iterating nodes out of order).

For reference if this comes up again, I found these by first adding a wrapper to pairs():

debugpairs = function (var)
  if var[1] then
    utilities.error("called pairs() on what looks like a sequential table")
  end
  return pairs(var)
end

Then aggressively replaced our use of pairs() with debugpairs():

$ git ls-files '*.lua' | xargs -n1 sed -i -e 's/in pairs/in debugpairs/g'

Then I‌ worked through running tests & building the manual replacing the bad lines with ipairs() or SU.sortedpairs() as appropriate until it stopped dying.

@alerque
Copy link
Member

alerque commented Jul 23, 2020

In retrospect we could/should probably have just used pl.tablex.sort() for this. 🤷‍♂️

@alerque

This comment has been minimized.

@alerque alerque added this to the v0.10.9 milestone Jul 23, 2020
@alerque alerque added the bug Software bug issue label Jul 23, 2020
@alerque alerque merged commit a77d627 into master Jul 23, 2020
@alerque alerque deleted the fix-532 branch July 23, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Software bug issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants