[Bug] Merging of sibling nodes with same name is faulty

[Marcono1234]

It appears it is intended that sibling nodes with the same name exist (see this comment). However the way they are treated is in my opinion rather unintuitive.

The method CommandNode.addChild(CommandNode<S>) replaces the command of the existing node and adds the children of the new node to the existing one. This is, at least from my point of view, not the wanted behavior since

• The old command is lost
• Other attributes of the new node like the requirement are lost

It might be best to either remove this merging and allow siblings with the same name, or to prevent them when building.

Example code

CommandDispatcher<Object> dispatcher = new CommandDispatcher<>();
    
dispatcher.register(
    literal("foo")
    .then(
        argument("bar", word())
        .executes(c -> {
            System.out.println("string");
            return 1;
        })
    )
    .then(
        argument("bar", bool())
        .executes(c -> {
            System.out.println("bool");
            return 1;
        })
    )
);

dispatcher.execute("foo not_a_bool", null);

[Dinnerbone]

Names must be unique, so we can't allow multiple with the same name. It's used in a variety of places as a key, such as pathing in the tree, serialization, or simply getting values out of the context.

Merging is supported as a design principle (which I really must write down somewhere!). It must be supported to "enhance" a command after it has already been registered, to allow mods to easily extend commands in Minecraft (and I'm sure other applications/games will find that useful too).

That said, merging must be made smarter - I totally agree with this. We should probably do some sanity checking when a merge happens, and throw an exception if it's nonsense - if anything other than the children changed. For example in your code, the second .then(argument("bar"), bool()) should throw an exception as the very type of the argument has changed, and that should not be allowed.

[Marcono1234]

What do you think about having separate methods for this modification instead? For example ArgumentCommand could have the methods

• addArgument(ArgumentType<T>) Adds the given argument type, which has to have the same type as the argument node. Argument types would then be stored in a list and the respective methods would go through the other arguments in this list if one fails.
• setArguments(List<ArgumentType<T>>) Replaces all currently set argument types.

Methods like these might be less suprising than runtime exceptions which are thrown when the merge fails and they already define restrictions during compile time. Additionally this implementation might be less error prone since there are a lot of cases you have to consider when determining if a merge is valid.

[JesFot]

A solution I can think of would be :

• Allow merging literal arguments with the same name
• Allow merging required arguments with the same name and same type
• Prevent merging of required arguments of different types

Exemple :

// Case 1 :
dispatcher.register(
    literal("foo")
    .then(
        literal("bar")
        .executes(c -> {
            System.out.println("string");
            return 1;
        })
    )
    .then(
        literal("bar") // Same name, literal -> Merge
        .then(literal("baz"))
    )
);
// Case 2 :
dispatcher.register(
    literal("foo")
    .then(
        argument("bar", word())
        .executes(c -> {
            System.out.println("string");
            return 1;
        })
    )
    .then(
        argument("bar", word()) // Same name, same type -> Merge
        .then(literal("baz"))
    )
);
// Case 3 :
dispatcher.register(
    literal("foo")
    .then(
        argument("bar", word())
        .executes(c -> {
            System.out.println("string");
            return 1;
        })
    )
    .then(
        argument("bar", bool()) // Same name, different type -> Exception
        .executes(c -> {
            System.out.println("bool");
            return 1;
        })
    )
);