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

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

Open
appgurueu opened this issue Feb 13, 2022 · 4 comments

Comments

@appgurueu
Copy link

appgurueu commented Feb 13, 2022

{} 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
Copy link
Contributor

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
Copy link
Author

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
Copy link

idbrii commented Nov 19, 2023

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
Copy link
Author

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 :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants