vberlier / nbtlib

A python library to read and edit nbt data.
MIT License
119 stars 16 forks source link

List subtype inference doesn't consider `List` as a possible subtype #9

Closed ch-yx closed 5 years ago

ch-yx commented 5 years ago

it cannot parse this snbt: {c:[ [] , [[],[]] ]}

while minecraft does allow it.

>>> nbtlib.parse_nbt("{c:[[] ,[[],[]] ]}")

Traceback (most recent call last): File "<pyshell#2>", line 1, in nbtlib.parse_nbt("{c:[[] ,[[],[]] ]}") File "C:\Users\doctc\AppData\Local\Programs\Python\Python37\lib\site-packages\nbtlib\literal.py", line 53, in parse_nbt tag = parser.parse() File "C:\Users\doctc\AppData\Local\Programs\Python\Python37\lib\site-packages\nbtlib\literal.py", line 122, in parse return handler() File "C:\Users\doctc\AppData\Local\Programs\Python\Python37\lib\site-packages\nbtlib\literal.py", line 175, in parse_compound compound_tag[item_key] = self.parse() File "C:\Users\doctc\AppData\Local\Programs\Python\Python37\lib\site-packages\nbtlib\literal.py", line 122, in parse return handler() File "C:\Users\doctc\AppData\Local\Programs\Python\Python37\lib\site-packages\nbtlib\literal.py", line 209, in parse_list raise self.error(f'Item {str(item)!r} is not a ' nbtlib.literal.InvalidLiteral: Item '[[],[]]' is not a List[End] tag at position 15

vberlier commented 5 years ago

Fixed in 6cb3b41608d3e018ea5c43e729e8403cd6293a5d. This was due to the fact that empty list tags don't have a concrete subtype. Now the subtype of empty list tags wil be inferred from the other list tags in the list.

vberlier commented 5 years ago

Closing the issue as the fix is now in release 1.3.0.

ch-yx commented 5 years ago

it seems like it is not fixed completely image_156 lists of a list may contain different type of value.

image_157 (by the way,keys start with a number is allowed. )

vberlier commented 5 years ago

Hmm I think the list problem is going to be a bit tricky to work with. List tags are supposed to have a uniform subtype, meaning that each list element must be of the same type:

>>> parse_nbt("[1,2,hello]")
Traceback (most recent call last):
...
nbtlib.literal.InvalidLiteral: Item 'hello' is not a Int tag at position 10

The thing is that lists containing different types of elements have themselves a different type:

>>> integers = parse_nbt('[1,2,3]')
>>> strings = parse_nbt('[a,b,c]')
>>> type(integers) is List[Int]
True
>>> type(strings) is List[String]
True
>>> List[Int] is not List[String]
True

This means that lists that contain different types of elements can't be placed in another list:

>>> boom = List([integers, strings])
Traceback (most recent call last):
...
nbtlib.tag.IncompatibleItemType: String('a') should be a Int tag

This is not a parsing problem, it has to do with the fact that nbtlib requires list subtypes to be deeply uniform. Changing this behavior will break nbtlib.schema so I'm afraid I'm not going to be able to fix it. Schemas rely on being able to deeply inspect the subtype of lists to cast everything to the appropriate type:

>> Thing = schema('Thing', {
...     'bigList': List[List[schema('ThingElement', {
...         'value': Int
...     })]]
... })
>>> thing = Thing({'bigList': [[{'value': 5}], [], [], [{'value': -1}, {'value': -2}]]})
>>> type(thing['bigList'])
<class 'nbtlib.tag.List[List[ThingElement]]'>

In think that this problem is kind of an edge-case, though. I don't think you would ever run into it, I'm pretty sure Minecraft doesn't use lists that contain lists of different subtypes. IMO the advantages of being able to define schemas outweigh the slight spec mismatch.

vberlier commented 5 years ago

However, I can do something about compound keys starting with a number.

ch-yx commented 5 years ago

I am 100% agree with you. I guess that anyone who uses list this way must be mad.XD

I just try to find how reliable your amazing work is.

thanks a lot for fixing my issues these days though all of them are minor bug and not getting annoyed at my poor English.

vberlier commented 5 years ago

Haha no problem, thanks for taking the time to test everything so thoroughly! English isn't my native language either so don't worry about it ;)

vberlier commented 5 years ago

Alright so I fixed a few issues with number and string parsing and in 94f990681d2ca1af409f0dad0d4491281accd0d0 you can now use compound keys that would usually be parsed as numbers.

vberlier commented 5 years ago

Version 1.3.2 has the fix for compound keys that were interpreted as numbers so I'm closing the issue as I won't be fixing the list problem. I might be adding a "caveats" or "notes" section to the README later that will warn users that as opposed to the spec, nbtlib assumes lists to be deeply uniform.

ch-yx commented 5 years ago

I come across some thing interesting:

while boom = List([integers, strings]) leads to a Error, boom = List[List]([integers, strings]) works well.

Have you fixed it already?

ch-yx commented 5 years ago

i have not found anything wrong with the List[List] object it return yet.

vberlier commented 5 years ago

Oh that's interesting. I never really thought about it but the behavior actually makes perfect sense.

1. List[List]([integers, strings])

List tags can only contain instances of their respective subtype. If you're putting an object that's not an nbt tag into a list, nbtlib will attempt to cast the object to the list subtype (i.e.: subtype(obj)).

>>> integers = List[Int]([1, 2, 3])
>>> integers[0]
Int(1)

The reason why List[List]([integers, strings]) works is because integers and strings are already nbt tags so nbtlib simply checks that they're both instances of the combined list subtype.

>>> integers = List[Int]([1, 2, 3])
>>> strings = List[String](['a', 'b', 'c'])
>>> combined = List[List]([integers, strings])
>>> combined.subtype is List
True
>>> isinstance(integers, List)
True
>>> isinstance(strings, List)
True

2. List([integers, strings])

For convenience's sake, instantiating a list with the List class directly will infer the list subtype from the specified values.

>>> integers = List([1, 2, Int(3)])
>>> type(integers) is List[Int]
True
>>> integers
List[Int]([Int(1), Int(2), Int(3)])

Since List[List]([integers, strings]) works, this means that in the end, the only reason why List([integers, strings]) doesn't work is because the code that infers the subtype from the specified elements is currently a bit too strict.

3. Implications

In a previous comment, I said that nbtlib requires lists to be deeply uniform, but I didn't think about List[List], which is kind of embarrassing considering I wrote the code... 😅 Anyway, I'm going to make sure that List[List] can be used without any problem.

Converting List[List] instances to and from the binary format already works.

>>> type(combined)
<class 'nbtlib.tag.List[List]'>
>>> b = BytesIO()
>>> combined.write(b)
>>> b.seek(0)
0
>>> List.parse(b) == combined
True

Snbt serialization is already working as well.

>>> print(combined)
[[1, 2, 3], ["a", "b", "c"]]

However, parsing non-deeply uniform list tags from snbt currently raises an exception.

>>> parse_nbt('[[1, 2, 3], ["a", "b", "c"]]')
Traceback (most recent call last):
...
nbtlib.literal.parser.InvalidLiteral: Item '"a"' is not a Int tag at position 27

This is because the parser uses the List class directly to create the list tag instance, and therefore relies on inference to determine the subtype. By adjusting the inference code and making it so that List([integers, strings]) works, the problem will resolve itself.

I'm reopening the issue. nbtlib does in fact support non-deeply uniform list tags through List[List], the problem is simply that the code for inferring the subtype of a list doesn't consider List as a possible subtype.

vberlier commented 5 years ago

Alright this should be fixed in 69f7c28c66825eede75935ba0aed8bdeac6bf5f3.

vberlier commented 5 years ago

The fix is now in release 1.4.5, closing the issue.

ch-yx commented 5 years ago

Sorry. =_=

I just found that

List([List[Int]([Int(1)]),List[List]([List[Int]([])])]) (or [[1], [[]]]) the outermost List still does not get its subtype properly..........

thus,parse_nbt('[[1], [[]]]') couldn`t works too.

ch-yx commented 5 years ago

please reeopeen this issue

vberlier commented 5 years ago

My bad, this should be fixed in 1e7c90a38623753949951b43e09ad7fd8555ebd3.

vberlier commented 5 years ago

And... it's live in v1.4.6.