starwing / lua-protobuf

A Lua module to work with Google protobuf
MIT License
1.75k stars 387 forks source link

maps with integer keys can be confusing #24

Closed seabadger closed 6 years ago

seabadger commented 6 years ago

In the old implementation, a protobuf map was representated in Lua as follows local f={ { key = "key1", value="value1" }, { key = "key2", value = "value2"} }

After the recent change, the implementation changed, unless I'm mistaken, to directly use table keys: local f = { key1 = "value1", key2 = "value2" }

I was unable to get this new implementation to work with integer keys consistently. I suspect this is because Lua treats integer keys specially (as array portion of the table, rather than the hash portion of the table). Unless I'm misunderstanding how tables are supposed to be represented...

Basically, encoding and then decoding this: local obj = { map_intstr = { [1] = "one", [2] = "two", [0] = "zero", [-1] = "minus one", } } results in { map_intstr = { "one", "two", [-1] = "minus one", [0] = "zero" }, } Note how the values outside of the normal range of array indices are treated as hash entries (accessed with 'pairs'), and integer values >=1 get treated as array values (accessed with ipairs).

Basically:

seabadger commented 6 years ago

Correction: looks like all keys are accessible with pairs(), and only non-negative are accessible via ipairs().

So this is only a little inconsistent, and may catch people if they either try to access the data using ipairs(), or if they try to use something that accesses tables in a generic way (e.g., the output above was produced via https://github.com/kikito/inspect.lua).

Thus, maybe not a bug, but a possible usability problem. I understand you may want to keep it the way it is, but I'll keep the report open just in case you have other thoughts.

starwing commented 6 years ago

Thanks for catching! But it’s really a bug. I will fix on next push (maybe tonight)

starwing commented 6 years ago

After a carefully review code and review your post, I'm very sorry I misunderstand your issue before :-(

Yes, it's not a bug. in Lua, { "one", "two" } is exactly just { [1] = "one", [2] = "two" }, and former is just a syntax sugar of latter.

and map's semantics indicate no order. means { [1] = "one", [2] = "two" } or { [2] = "two", [1] = "one" } are all legal result of decode.

starwing commented 6 years ago

Notice that's the semantics of Lua language. So I have no way to change this behavior :-)

just close, if you have new idea about map, you can re-open it. Thanks for feedback!

seabadger commented 6 years ago

I agree that this is semantics of Lua and not really a bug (thanks for taking a look anyway! :). It might still be worth mentioning this in the documentation that map keys, even when they are integers, should be accessed via 'pairs()' only, not ipairs() (since the latter might not give the full set of keys). I know it's just the language semantics, but it's one of the less obvious bits that might catch someone.

starwing commented 6 years ago

Okay I will change document :-)