Inventory API query return Type

[Faithcaio]

I see this kind of code very often:

GridInventory gridInv= inventory.query(INVENTORY_TYPE.of(GridInventory.class));

Which is fine if you are sure you will get the specific type of inventory back.
But can also result in ClassCastException because the result can also be:

• an empty inventory
• a ResultInventoryAdapter with multiple inventories of that type

One idea to reduce confusion about this would be to return Inventory everywhere instead of <T extends Inventory> T

Opinions? other Ideas?

[Mumfrey]

The general idea of the pseudo-duck-typing afforded by the bounded generic was that people are likely going to query and immediately cast to the desired type, the generic just elides the explicit cast. Neither is ideal but I felt when I designed it that the duck typing was just cleaner in situations that the query is known to succeed.

• The benefits of removing the bound are that people will be encouraged to assign to Inventory and check if empty before casting
• The counterpoint is that they won't, in fact, do that. Developers are more likely to just put in an explicit cast.

From an "encouraging people to do the right thing" point of view, I think the suggestion makes sense. From a totally misanthropic perspective I don't think it will actually do that.

[SimonFlash]

This is something that I've wanted ever since our conversation a couple weeks ago. The fact that this behavior isn't well documented is made worse by the improper use of generics. It's a pitfall for many people who are just starting to work with Sponge or the InventoryAPI.

My recommendation is to have an inventory containing the result that respects generics, much like the ResultInventoryAdapter<T> (though if it's only used for queries, QueryResult... is more specific. This should contain all inventories that matched the query in such a way where the size of the collection is visible (aka, number of results of the query) and if there are no results the collection is empty. Additonally, generics shoud only be applicable to INVENTORY_TYPE queries; everything else should just return a basic Inventory.

The result should also not comtain an empty inventory. Instead, methods like .first() would return something like(results.size() > 0 ? iterator.next() : empty. Generally speaking, a priority should be put on wrapping the returned collection more than anything else - by emphasizing the ability to have one, zero, or multiple results, it'll help point developers in the right direction for how to properly use the query system.

[Faithcaio]

@Mumfrey
Plugin developers are often beginners with java so while I think there will still be some that just try to cast without knowing or checking anyways it will be clearer to them that the issue in their code because they are explicitly casting. (and getting a ClassCastException)

@SimonFlash
While I certainly could always wrap the result I feel the API gets clunky when you know what you are getting.
Also inventories are already themself always a collection of sub-inventories.
Having a separate query for InventoryTypes could make sense.

[SimonFlash]

The result is already being wrapped - the inventory you receive isn't an inventory per se, it's a view of the inventories returned by the query. The class returned should make it clear that A: it is not the same thing that was queried for, and B: it contains an unknown (non-negative) number of inventories that is what was queried for.

[Faithcaio]

Not always. Currently if you get a single inventory matching the query you get the inventory itself.

[JBYoshi]

How about making ResultInventoryAdapter a final class with generic typing? Then the compiler won't even let you do a cast (without casting to Object first, which most people don't know about).

public interface Inventory {
    <E extends Inventory> ResultInventoryAdapter<E> query(Class<E> type);
    // other methods here
}

public interface GridInventory extends Inventory {}

// Because this is final, the compiler already knows the full hierarchy (excluding any mixin antics).
// So it can generate errors like the one below.
public final class ResultInventoryAdapter<E extends Inventory> implements Inventory {
    public <E2 extends Inventory> ResultInventoryAdapter<E2> query(Class<E2> type) {
        return new ResultInventoryAdapter<>();
    }
    // other methods here
}

public void test(Inventory anyInventoryObject) {
    // Compiler errors here: "Inconvertible types; cannot cast ResultInventoryAdapter to GridInventory"
    GridInventory inv = (GridInventory) anyInventoryObject.query(GridInventory.class);
}

[Faithcaio]

see #1848