JorelAli / CommandAPI

A Bukkit/Spigot API for the command UI introduced in Minecraft 1.13
https://commandapi.jorel.dev
MIT License
504 stars 60 forks source link

Implement "safe-casting" #540

Closed DerEchtePilz closed 2 months ago

DerEchtePilz commented 3 months ago

This PR is supposed to implement certain methods as suggested by Hawk here and by #535.

Currently, there is a bit more testing I want to do, additionally I am not sure if this would actually be something we want. If so, there is a bit more work required:

DerEchtePilz commented 3 months ago

Someone please review the latest changes, not sure if we want that behaviour.

willkroboth commented 2 months ago

Just a note that some arguments use a raw type as their T generic parameter. For example, MapArgument<K, V> extends Argument<LinkedHashMap> rather than Argument<LinkedHashMap<K, V>>

https://github.com/JorelAli/CommandAPI/blob/a7d2717450c017002647fe611a3d39deda6874d2/commandapi-platforms/commandapi-bukkit/commandapi-bukkit-core/src/main/java/dev/jorel/commandapi/arguments/MapArgument.java#L26

If MapArgument did extend Argument<LinkedHashMap<K, V>>, then the getPrimitiveType method would have to return a Class<LinkedHashMap<K, V>> object, which Java doesn't like very much: https://stackoverflow.com/questions/1079279/class-object-of-generic-class-java.

That just means this code has a little warning:

MapArgument<String, Integer> argument = new MapArgumentBuilder<String, Integer>("map")
    .withKeyMapper(s -> s)
    .withValueMapper(s -> Integer.parseInt(s))
    .withoutKeyList()
    .withoutValueList()
    .build();

new CommandAPICommand("command")
    .withArguments(argument)
    .executes(info -> {
        // Type safety: The expression of type LinkedHashMap needs unchecked conversion to conform to Map<String,Integer>
        Map<String, Integer> map = info.args().getByArgument(argument);
        info.sender().sendMessage(map.toString());
    })
    .register();

This note might also be relevant for https://github.com/JorelAli/CommandAPI/pull/545. It's not a big problem, but the two systems don't currently work perfectly for these arguments:

DerEchtePilz commented 2 months ago

True, but if I am not mistaken, this is the best we can do here. Since it additionally doesn't work for the CustomArgument anyway, I guess in terms of safety we can ignore the generic types.