brigadier icon indicating copy to clipboard operation
brigadier copied to clipboard

[Bug] Duplicate argument names are not prevented

Open Marcono1234 opened this issue 7 years ago • 6 comments

Currently there can be multiple arguments with the same name. This likely indicates a mistake by the programmer since the last argument would overwrite the values of all previous arguments in the CommandContext.

dispatcher.register(
    literal("foo")
    .then(
        argument("bar", integer())
        .executes(c -> {
            System.out.println("Bar1 called");
            return 1;
        })
        .then(
            argument("bar", bool())
            .executes(c -> {
                System.out.println("Bar2 called");
                return 1;
            })
        )
    )
);

dispatcher.execute("foo 123 true", null);

Marcono1234 avatar Oct 07 '18 18:10 Marcono1234

Initially I thought this would be about arguments with the same name at the same level, which is intended to allow appends but definitely could be improved to detect mistakes. I hadn't considered collisions on different levels, that's totally true.

If anyone would like to try fixing this, please first add a test that proves this bug exists. One approach to fixing this could be to walk the children after adding to a parent.

Dinnerbone avatar Oct 07 '18 18:10 Dinnerbone

Please correct me if I'm wrong (I'm trying to learn some programming in my spare time so I'm likely making a lot of incorrect assumptions and statements), but an error like this seems like it could be the cause of some mod conflicts. The way I understood the bug, the way the code currently is, you can have overlapping commands?

ex. if 2 mods add commands to "zap" an object, the "zap" command chosen would depend on the load order?

P.S thanks for open-sourcing some of the libraries. It'll be fun for me to try to figure it out and play with it ;)

IkeKap avatar Oct 08 '18 04:10 IkeKap

@IkeKap, no this issue is about arguments in the same path of a single command, for example:

foo <bar:integer> <bar:boolean>

The problem here is that when the second bar argument is provided, the value of the first one will be overwritten in the CommandContext, which is very likely not what you want. For example if you as user enter the following command:

foo 1 true

Marcono1234 avatar Oct 09 '18 14:10 Marcono1234

what would be the correct behavior in case two or more arguments have the same name at different levels?

iBlackShadow avatar Oct 22 '18 03:10 iBlackShadow

Probably throwing an exception. Other solutions I can think of:

  • Ignoring it: Likely not what the programmer wants
  • Automatically renaming it: Likely also not wanted
  • Don't accessing arguments based on string keys but on argument objects instead, see also #26. Might make serialization a little bit more complicated, but might still work when including the position of the respective node.

Marcono1234 avatar Oct 22 '18 19:10 Marcono1234