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

Casting of empty ListTag is error-prone #14

Closed
Marcono1234 opened this issue Dec 9, 2018 · 4 comments
Closed

Casting of empty ListTag is error-prone #14

Marcono1234 opened this issue Dec 9, 2018 · 4 comments

Comments

@Marcono1234
Copy link
Contributor

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

Querz commented Dec 10, 2018

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

Marcono1234 commented Dec 11, 2018

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

if (typeID != 0 && t.getID() != typeID) {

And the modification of the tag type in the add method

typeID = t.getID();
typeClass = t.getClass();

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

Querz commented Dec 12, 2018

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 added a commit that referenced this issue Dec 12, 2018
This was referenced Dec 12, 2018
@Querz
Copy link
Owner

Querz commented Dec 20, 2018

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);

@Querz Querz closed this as completed Dec 20, 2018
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

2 participants