rxi / lume

Lua functions geared towards gamedev
MIT License
995 stars 80 forks source link

`lume.isarray` doesn't return correct results for mixed tables or empty arrays #39

Open appgurueu opened 2 years ago

appgurueu commented 2 years ago

{} is considered not an array, as ({})[1] == nil.

{1, a = 1, b = 2} is considered an array as the first element is not nil. This will make getiter loop over it using ipairs, skipping the hash part containing a and b keys.

I'm not sure how this should be fixed because checking the hash part usually involves iterating over it using pairs, which has a linear time as it might first visit the entire list part. The current check may be wrong in these "edge cases", but at least runs in constant time.

jaythomas commented 2 years ago

I think this behavior should be noted, but personally I don't consider it a bug in my own use cases. Sometimes I have an array that I iterate over and for all intents and purposes is an array, but I may attach methods or metadata to it via static keys.

Kind of how Arrays in javascript are actually Objects with methods and other keys on them too. They just happen to be iterable and function like you would expect an array to.

appgurueu commented 2 years ago

The empty case could actually trivially be covered in constant time by changing the expression to next(table) == nil or table[1] ~= nil. The mixed case is actually the more problematic one.

idbrii commented 9 months ago

I've seen enough issues with isarray incorrectly detecting tables with integer keys as arrays that I think it's fundamentally incorrect to pass lists with holes to lume. I've been thinking that checking for an item after the last numeric index might be a better test:

function lume.isarray(x)
  local is_array = type(x) == "table" and rawget(x, 1) ~= nil
  assert(not is_array or next(x, #x) == nil, "Do not use integer indexed tables with isarray.")
  return is_array
end

tests["lume.isarray"] = function()
  testeq(lume.isarray({1,2,3}))
  testeq(not lume.isarray({
        cat = 12,
        dog = 34,
    }))

  -- This should assert but doesn't for me.
  tester.test.error(lume.isarray({
        [73] = 12,
        [54] = 34,
        [1] = 345,
    }))

  -- This correctly asserts for me!
  tester.test.error(not lume.isarray({
        cat = 12,
        dog = 34,
        [1] = 345,
    }))
end

rawget to prevent metatable errors when using strict tables and check that there's no key after the supposed last numeric one.

I think this is better as an assert that isarray isn't being used with integer key dictionaries rather than a runtime check to rely on. It's still possible the ordering of the dictionary puts all keys before the last numeric one as shown by the failure to assert of that third test.

appgurueu commented 9 months ago

There is no constant-time (or even logarithmic-time) way to check for holes I'm afraid. #t is not the largest numeric index, but rather any index such that t[#t] ~= nil and t[#t + 1] == nil (with the special case of #t == 0 if t[1] == nil). This means that #{[1] = 1, [3] = 3} for example can be either 1 or 3 (which one it is depends on where the binary search that Lua uses internally uses).

Ultimately, to check for holes, you have to scan the entire array (or hash part, in which sequences may (partially) live as well) in the worst case. It can't be done efficiently :/