Querz / NBT

A java implementation of the NBT protocol, including a way to implement custom tags.
MIT License
183 stars 48 forks source link

Casting of empty ListTag is error-prone #14

Closed Marcono1234 closed 5 years ago

Marcono1234 commented 5 years ago

You can change the type of an empty ListTag by adding an element of a different type. This appears to be "intended", see ListTagTest.testCasting(), but in my opinion this is pretty error-prone.

It makes it pretty easy to change the list type by accident and when you then use the list at a completely unrelated place you suddenly get an exception.

I would suggest adding a Class<? extends Tag> to the constructor or using some new class to store both class and typeID and then validate every added element against them.

Querz commented 5 years ago

and when you then use the list at a completely unrelated place you suddenly get an exception.

This would not change by passing the type class to the constructor. The point of having generics is that the type is already ensured at compile time, therefore the actual solution would be to remove all convenience methods that add values directly to a ListTag like e.g. ListTag#addString(). I made a compromise by having generic methods on the one hand that use the Tag classes for type safety and on the other hand having convenience methods that are consistent with the other structure tags (CompoundTag and StructTag).

Marcono1234 commented 5 years ago

When the type is passed to the constructor it would already be initialized when you try to add an element of the incorrect type.

Then the special case in checkValue would have to be removed https://github.com/Querz/NBT/blob/947c750bbe0a4cdce79b3bf51ec941aaf657f4ff/src/main/java/net/querz/nbt/ListTag.java#L335

And the modification of the tag type in the add method https://github.com/Querz/NBT/blob/947c750bbe0a4cdce79b3bf51ec941aaf657f4ff/src/main/java/net/querz/nbt/ListTag.java#L88-L89

Additionally it would probably be good to have a protected or private method addUnchecked(Tag<?>) to prevent all the casts and SuppressWarnings annotations for the type specific methods.

Querz commented 5 years ago

I'll have too look into how deserialisation works with that, because the type of the list is read after it's been instantiated.

Querz commented 5 years ago

An empty ListTag can now only be untyped if created using the protected static method ListTag.createUnchecked(). ListTag#getTypeClass() will return EndTag.class if the type is not specified and the ListTag is empty. From this untyped state, the LisTag can only ever change it's type once using any of the set(), add...() or as...List() methods. A ListTag of type EndTag can not be created anymore. If attempting to do so it will throw an IllegalArgumentException.

#clone(), #equals() and #hashCode() have been adjusted to include the type class in their actions.

The ListTag constructor now needs a mandatory type class additionally to the ListTag generic type specification. The type class passed to the constructor must be of the exact same type as the generic type.

ListTag<StringTag> s = new ListTag<>(StringTag.class);
ListTag<ListTag<?>> l = new ListTag<>(ListTag.class);