Preserve arguments after redirects

[willkroboth]

This PR resolves #137.

---

Previously, when executing a CommandContext that has a child CommandContext due to the parser encountering a redirected node, the arguments present in the original context would not be copied to the child context. In the example I presented in #137, the command structure looked like this:

literal("command"):
    literal("high") -> argument("number", Integer from 10 to 20) (first branch) -> argument("string", greedy string) -> executor
    literal("low") -> argument("number", Integer from 0 to 10) -> redirects to (first branch) node

The bug I identified in #137 means that when executing the redirected branch (eg. "command low 5 some text"), an IllegalArgumentException would be thrown since the executor expects the "number" argument to be present in the child CommandContext, which is used when executing the command. I expected that the command should run without error, with the value for the "number" argument being 5. In contrast, the 'real' branch would always work as expected, with the "number" argument present as given.

---

This PR fixes this bug by adding and using the CommandContext#copyWithArgumentsOf method:

public CommandContext<S> copyWithArgumentsOf(CommandContext<S> context) {
    Map<String, ParsedArgument<S, ?>> newArguments = new HashMap<>();
    newArguments.putAll(context.arguments);
    newArguments.putAll(this.arguments);
    return new CommandContext<>(source, input, newArguments, command, rootNode, nodes, range, child, modifier, forks);
}

In my example case, the "number" -> value: 5 argument present in the original CommandContext would be copied to the child CommandContext, so that it is present when the executor is run.

---

I added tests that directly make sure the new CommandContext#copyWithArgumentsOf method works as expected. I want to add tests to make sure copyWithArgumentsOf is being used correctly when executing commands, but I wasn't sure where to put a test like that. I wanted to make sure that I followed the code style of this repo. I'm not yet sure how the code base tests functionality like this, so I erred on the side of not adding those tests yet.

---

If you'd like to discuss whether or not the 'bug' this PR resolves is actually intended behavior, I think those comments fit best as replies to the original issue, #137. I'm honestly not sure if my issue is intended behavior or not since I couldn't find any documentation on what node redirects are supposed to do. It seems most intuitive to me that the arguments should be preserved, hence why I went through the effort to make this PR :P.

[yanisderbikov]

If you will add a summary it will be better to understand your main message :)

[willkroboth]

Sure. I think I also found a different but related problem with redirects, suggestions, and arguments, so I'll explain that here too.

---

Summary:

When suggesting arguments after a redirect, the CommandContext given can only access arguments before the redirect.

When executing a command after a redirect, the CommandContext given can only access arguments after the redirect.

These two situations handle arguments differently when redirects are present. I believe that both of these situations are bugs. The correct outcome should be that all arguments are present.

---

Reproducing the 'bugs'

To help explain this better, I created a demonstration here willkroboth@b1663e5 that looks like this:

@Test
public void testCustomStuff() throws Exception {
    System.out.println("Testing custom stuff");

    SuggestionProvider<Object> monitorSuggestions = (context, builder) -> {
        System.out.println(context.getArguments());

        builder.suggest("a").suggest("b").suggest("c");
        return builder.buildFuture();
    };

    ArgumentCommandNode<Object, String> end = argument("ending", StringArgumentType.word())
            .suggests(monitorSuggestions)
            .then(argument("lastArg", StringArgumentType.word())
                    .suggests(monitorSuggestions)
                    .executes(context -> {
                        System.out.println(context.getArguments());

                        return 1;
                    })
            )
            .build();

    ArgumentCommandNode<Object, Integer> firstBranchEnding = argument("number", IntegerArgumentType.integer(1, 10)).then(end).build();
    LiteralArgumentBuilder<Object> firstBranch = literal("low").then(firstBranchEnding);

    LiteralArgumentBuilder<Object> secondBranch = literal("high").then(
            argument("number", IntegerArgumentType.integer(11, 20))
                    .redirect(firstBranchEnding)
    );

    LiteralArgumentBuilder<Object> base = literal("base")
            .then(firstBranch)
            .then(secondBranch);

    subject.register(base);

    System.out.println(subject.getCompletionSuggestions(subject.parse("base low 5 ", source)).get());
    System.out.println();
    System.out.println(subject.getCompletionSuggestions(subject.parse("base low 5 a ", source)).get());
    System.out.println();
    System.out.println(subject.getCompletionSuggestions(subject.parse("base high 15 ", source)).get());
    System.out.println();
    System.out.println(subject.getCompletionSuggestions(subject.parse("base high 15 a ", source)).get());
    System.out.println();

    subject.execute("base low 5 a b", source);
    System.out.println();
    subject.execute("base high 15 a b", source);
    System.out.println();
}

When this test is run, this gets printed into the console:

Testing custom stuff
// First test
{number=ParsedArgument{range=StringRange{start=9, end=10}, result=5}}
Suggestions{range=StringRange{start=11, end=11}, suggestions=[Suggestion{range=StringRange{start=11, end=11}, text='a', tooltip='null'}, Suggestion{range=StringRange{start=11, end=11}, text='b', tooltip='null'}, Suggestion{range=StringRange{start=11, end=11}, text='c', tooltip='null'}]}

{number=ParsedArgument{range=StringRange{start=9, end=10}, result=5}, ending=ParsedArgument{range=StringRange{start=11, end=12}, result=a}}
Suggestions{range=StringRange{start=13, end=13}, suggestions=[Suggestion{range=StringRange{start=13, end=13}, text='a', tooltip='null'}, Suggestion{range=StringRange{start=13, end=13}, text='b', tooltip='null'}, Suggestion{range=StringRange{start=13, end=13}, text='c', tooltip='null'}]}

// Second test
{number=ParsedArgument{range=StringRange{start=10, end=12}, result=15}}
Suggestions{range=StringRange{start=13, end=13}, suggestions=[Suggestion{range=StringRange{start=13, end=13}, text='a', tooltip='null'}, Suggestion{range=StringRange{start=13, end=13}, text='b', tooltip='null'}, Suggestion{range=StringRange{start=13, end=13}, text='c', tooltip='null'}]}

{number=ParsedArgument{range=StringRange{start=10, end=12}, result=15}}
Suggestions{range=StringRange{start=15, end=15}, suggestions=[Suggestion{range=StringRange{start=15, end=15}, text='a', tooltip='null'}, Suggestion{range=StringRange{start=15, end=15}, text='b', tooltip='null'}, Suggestion{range=StringRange{start=15, end=15}, text='c', tooltip='null'}]}

// Third test
{number=ParsedArgument{range=StringRange{start=9, end=10}, result=5}, ending=ParsedArgument{range=StringRange{start=11, end=12}, result=a}, lastArg=ParsedArgument{range=StringRange{start=13, end=14}, result=b}}

// Fourth test
{ending=ParsedArgument{range=StringRange{start=13, end=14}, result=a}, lastArg=ParsedArgument{range=StringRange{start=15, end=16}, result=b}}

This output shows both 'bugs' in action. It probably needs an explanation though if you don't know what to look for.

---

Structure of the example

The structure of my test command looks like this:

literal base
-> then literal low
   -> then argument number (integer from 0 to 10)
      -> then argument ending (string that's just a single word)
         suggests 'monitorSuggestions'
         -> then argument lastArg (string that's just a single word)
            suggests 'monitorSuggestions'
            executes the only executor for this command
 -> then literal high
    -> then argument number (integer from 11 to 20)
       redirects to base->low->number

monitorSuggestions looks like this:

SuggestionProvider<Object> monitorSuggestions = (context, builder) -> {
    System.out.println(context.getArguments());

    builder.suggest("a").suggest("b").suggest("c");
    return builder.buildFuture();
};

And the Command on lastArg looks like this:

context -> {
    System.out.println(context.getArguments());

    return 1;
}

---

The problem with redirects, arguments, and suggestions

The first test checks the suggestions for "base low 5 " and "base low 5 a ". Since this path does not include a redirect, it acts as expected. The CommandContext when building suggestions includes both the number and ending arguments.

Simplified output:

// Suggesting `"base low 5 "`
Arguments: {number=5}
Suggestions{'a', 'b', 'c'}

// Suggesting `"base low 5 a"`
Arguments: {number=5, ending=a}
Suggestions{'a', 'b', 'c'}

The second test checks the suggestions for "base high 15 " and "base high 15 a ". This path includes a redirect, so the arguments get messed up.

Simplified output:

// Suggesting `"base high 15 "`
Arguments: {number=15}
Suggestions{'a', 'b', 'c'}

// Suggesting `"base high 15 a"`
Arguments: {number=15}
Suggestions{'a', 'b', 'c'}

Here, the CommandContext used when building the suggestions for "base high 15 a" is missing the value for the argument ending, even though it was given as "a". When base->high->number redirects to base->low->number->ending, only arguments before the redirect are accessible.

---

The problem with redirects, arguments, and executes

The third test checks what happens when executing "base low 5 a b". Since this path does not include a redirect, it acts as expected. The CommandContext includes the number, ending and lastArg arguments.

Simplified output:

Arguments: {number=5, ending=a, lastArg=b}

The fourth test checks what happens when executing base high 15 a b. This path includes a redirect, so the arguments get messed up.

Simplified output:

Arguments: {ending=a, lastArg=b}

Here, the CommandContext used when executing "base high 15 a b" is missing the value for the argument number, even though it was given as 15. When base->high->number redirects to base->low->number->ending, only arguments after the redirect are accessible.

---

I think that both of these behaviors are unintentional. It seems that arguments are not being handled properly when redirects (and forks) create child CommandContext objects. This PR aims to resolve these two 'bugs' by making sure that all arguments given in a command are present when redirects are involved, the same way it works with a linear path when there are no redirects.

It looks like #140 moved some functionality around, so this PR currently conflicts with the main branch. I also recently realized there was a bug with the suggestions (thanks to JorelAli/CommandAPI#310), so I haven't figured out how to fix that yet. I'll try to figure something out though and update this PR soon.

[willkroboth]

Recreated this PR as #142