I've added these tests. Of course, they are failing. I'm just thinking, what is better to do here:

• Add a special case for an empty table (not very efficient).
• Add a isArray-like test (same as in __TS__ArrayConcat).

The problem is there is not really a way to distinguish an empty table from an empty object in lua (they are both translated to {}). I am thinking we could assume for this case objects won't be empty and extend the existing check to something like ... && (1 in value || (next(value, undefined) == undefined)) To efficiently check the object has no keys. I would like to discuss your and my suggestions with some other contributors in the discord before making a final decision. I will come back to this after.

Alright, we discussed it and came to the following conclusion: For now we would be fine if the way you solve this is by assuming that if an object has no keys, it is an array. This is not entirely correct, but there is no straightforward (or even a complex one) way to deal with this perfectly, so we would rather go for the very simple non-perfect fix than to go for a very complex non-perfect fix. So then the check could indeed be modified as I suggested above by adding a special case to the if statement. Please add a comment to the LuaLib explaining what the reasoning behind (1 in value || (next(value, undefined) == undefined)) is and in which case it will fail (objects without keys in list).

It didn't help as next(value, undefined) == undefined (using === works the same) translates to

({ next(value, nil) }) == nil

and not, as expected, to

next(value, nil) == nil

Any idea why?

EDIT: It's a more general problem, as next(x, undefined) translates to ({ next(x, nil) }).

That sounds very strange, can you just push your changes so I can have a look?

It seems to be caused by @tupleReturn directive on next definition - removing it fixes the problem (but it's there for a reason).

EDIT: All tests pass even without this directive.

That does explain the behavior. This turned out a bit more complicated than I expected. To get rid of the wrapping we need a bit of typing. I feel like there must be a better way but for now you can put this in declarations/global.d.ts:

// Override next declaration so we can omit extra return values
type NextEmptyCheck = (this: void, table: any, index: undefined) => unknown | undefined;

Then you change the check to

if (type(value) === "table" && (1 in value || (next as NextEmptyCheck)(value, undefined) === undefined)) {

Edit: oops, accidentally added some incorrect stuff to the NextEmptyCheck type, removed it now.

I used if (pcall(() => (arg as any[]).length) && type(arg) !== "string") in ArrayConcat to determine if a value is an array.
Maybe it could be used here as well.
Im not so sure tho since this new workaround might be better, because it doesn't rely on pcall.

In any case we should keep it consistent and use one of the methods for an isArray workaround across lualib.

I also thought about that at first, but {a: 1} also pass that pcall test. In general this may be a bigger problem, as [].concat({a: 1}) === [].

👍 for a general isArray.

EDIT: Maybe use Array.isArray for that?

You are right! The array concat test do not include a case with an object... yikes.
So good thing we found that issue #738 here.

Anyway I think we should merge this and address isArray and array.concat in a different issue/pr.

Also related: #262

I agree @lolleko, maybe we implement Array.isArray in the lualib and add some lualib dependencies? We can discuss it a bit more. That's a PR for another day.