Pass context to ArgumentType.parse

[Martmists-GH]

For a few of my projects I'm considering using Brigadier, however one limitation has been stopping me from using this: ArgumentType.parse is not context-aware.

In many cases, the object to be returned requires context of where or by who the command was invoked. The simple solution would be to pass the source object to the parse method as second parameter, however this would cause breaking changes in the library.

To solve this, I propose to give the parse method two signatures: parse(StringReader reader) and parse(StringReader reader, Object context), both with a default implementation which returns null.
Sadly due to how the current arguments are implemented they do not supply the type of the context, but on a per-application basis this could be done with a manual cast in the parse() method. To handle both functions, the following could be done:

    // ArgumentCommandNode.java
    @Override
    public void parse(final StringReader reader, final CommandContextBuilder<S> contextBuilder) throws CommandSyntaxException {
        final int start = reader.getCursor();
        final T result = type.parse(reader);
        if (result == null) {
            // The context-unaware method was not implemented, call the context-aware method
            result = type.parse(reader, contextBuilder.getSource())
        }
        if (result == null) {
            // Throw an error to avoid magic NullPointerExceptions in the user's code
            throw CommandSyntaxException("No implementation for parse on argument type!")  // Simple error as example, should be improved
        }
        final ParsedArgument<S, T> parsed = new ParsedArgument<>(start, reader.getCursor(), result);

        contextBuilder.withArgument(name, parsed);
        contextBuilder.withNode(this, parsed.getRange());
    }

This way users can implement context-aware parse methods without breaking any existing code, unless someone implements an ArgumentType that returns a nullable value.

[sodiboo]

Have you considered ArgumentCommandNode.isValidInput()? It does not have a context or source object and needs to call the ArgumentType's parse method, how is that meant to work with this change?

brigadier/src/main/java/com/mojang/brigadier/tree/ArgumentCommandNode.java

Lines 97 to 106 in ff42867

	@Override
	public boolean isValidInput(final String input) {
	try {
	final StringReader reader = new StringReader(input);
	type.parse(reader);
	return !reader.canRead() || reader.peek() == ' ';
	} catch (final CommandSyntaxException ignored) {
	return false;
	}
	}

if an ArgumentType requires a source object and uses the default implementation for no source, then any input for it will be considered invalid when searching for ambiguities (since most arguments won't have leading whitespace), and as such finding ambiguities will silently fail with the wrong results

From looking at a decompile, Minecraft seems to get around this limitation by parsing the intention of every argument into some object (i.e. relative coordinates, the @s selector), and then in execution applying this "intention" to the sender. It seems to fully parse whether an argument is valid or not, what that argument actually means, but only actually finishing the parse into a proper result at execution time.

I can think of one place this model does break down though, in for example a Discord bot, you might want an argument type for a role - reasonably this should only work for roles within the guild the command was sent, and although you can easily parse a given snowflake, make sure it's the correct format, make sure it actually resolves to a role - there's no way to check that this role exists in the given guild at parse-time, the only solution would be to throw an exception at execution-time after additional parsing, so clearly this doesn't work for everyone.

Proposal

I propose this: A new step in the execution process, right after parsing and before execution where input is validated and finalized.

For most arguments (like every single one that's built into brigadier) nothing would be done here, but for some arguments that require access to the source, the input should be parsed at parse-time and made sure it's the correct format. For my discord example, this would be making sure it's a valid snowflake, that the timestamp isn't in the future or whatever, and storing it as a Snowflake or a long.

Then at validate-time additional processing can be done, which in my discord example could make sure that the snowflake corresponds to a valid role, and that the role exists in the guild, and finally present the actual command context with a Role instead of a Snowflake. This is where the argument would have access to the command source, and this step is skipped when searching for ambiguities (which is why it'd be important to actually parse the format at parse-time).

This could also allow multiple arguments that use the same or similar format, but have different valid values (i.e. a user and a role both use snowflakes, however their snowflakes won't ever match) to be unambigous before execution-time. An argument can throw at validate-time the same way it does at parse-time and to the user it should look the same, and mostly everything should be nearly unchanged.

Minecraft seems to follow this model actually, just that it embeds validate-time inside of execution-time and completely outside of brigadier, which seems a bit strange in my opinion, and could probably be improved by being a part of brigadier.

Overall this shouldn't be a big change, it could be non-breaking if a default implementation for the validate stage were to just return the parsed value.

[Martmists-GH]

I'll look into where and how this validation step will be implemented, thanks for reminding me about isInputValid.
Sadly, I don't have much time to work on this right now, but if you'd like to I can give you access to my fork so you can implement it.

[sodiboo]

I was actually looking through the issues to see any unfixed issues in brigadier to fix before releasing a port of it to Dart - i think brigadier is a nice library, but personally i don't like Java that much

I've implemented this proposal in my port by using a shared type ArgumentType<S, T>, and a SimpleArgumentType<T> extends ArgumentType<Never, T> (Never is the bottom type, meaning this can be assigned to any ArgumentType as long as the T is shared), aswell as a ComplexArgumentType<S, I, T> extends ArgumentType<S, T>. (I is the intermediate parsed value, but not the final result)

SimpleArgumentType<T> is meant to have a similar interface to ArgumentType (the one in this repo, not the base bottom type in my impl) when overriding, but of course if updating brigadier you might need to make some breaking changes or have some ugly naming to make ArgumentType work the same

The parse() function is the same, except it returns Object? (top type) in ArgumentType and overridden to return T in SimpleArgumentType<T> and returns I in ComplexArgumentType<S, I, T>.

validate() returns a FutureOr<T> (i don't know how well that translates to Java, essentially it can be an async function or sync function for anyone implementing an argument type, async might be required for certain tasks like my discord role example, although most libs would have caching, so it might not be necessary to fetch from discord). This value is then awaited and the contents of the Future (or simply the value returned) is the result that the context.getArgument() function returns.

In ArgumentType, validate() takes a covariant parameter parsed (i don't know if that works at all in java) of Object? (top type), and of course the S source. The signature is FutureOr<T> validate(covariant Object? parsed, S source);

In SimpleArgumentType<T> its implementation is simply T validate(T parsed, Object? source) => parsed; - even though S is Never in SimpleArgumentType, i can accept that aswell as other types, and essentially any sender. The rest of this simply synchronously returns parsed, which is how i skip the validate stage.

In ComplexArgumentType it returns FutureOr<T>, and takes I parsed and S source - this allows the method to be asynchronous for argument types that need it, and of course this T is what is given to the command code. The signature is FutureOr<T> validate(I parsed, S source);

In the parse stage, the parse() function is ran (duh) and its results stored in an "Unfinished Arguments" map, where i have an UnvalidatedArgument, which is just like ParsedArgumentValue except it stores an Object? intermediate value, aswell as the actual type that can validate it. This intermediate value will always be what was returned from parse() meaning it's safe to assign it to the parameter type Object? when you know the value has a subtype like String, because in SimpleArgumentType and ComplexArgumentType the types already match.

Right before the execute stage, where the command path would already be found from the parse, every argument is validated when the CommandContext is created, aswell as validated again when the CommandContext is copied to a new sender (for redirect modifiers), if a validate throws, then that error goes directly to the calling code as if it were an error from the command - when describing this, i realize i should probably make some kind of type for validation exceptions, rather than accepting any exception, that way calling code can differentiate between command syntax and argument values being invalid without actually looking at error message.

Now, what if this argument doesn't throw in parse but would throw in validate? what about isInputValid that doesn't call validate? well, it should throw in the parse then if it would always fail at validate, but for any sender-specific validation that throws most of the time except maybe one sender that can't even exist, as far as the input being valid is concerned, the command is fine, as isInputValid asks "is this input a valid command?", not "is this input a valid command from this sender?".

Although of course this doesn't map 1:1 to java, but it should give you an idea of how i've done it so you can implement it the best way for your language? (mainly the asynchrony, which from what i can tell is a breaking change unless you specifically implement it as a fourth AsyncArgumentType or something like that, but also that covariant parameter which seems to not really work, perhaps you'll have to settle with Object and force implementers to cast to their intermediate type, assuring them that's completely safe, or you cast that in the library itself and make implementers instead implement an overload specific to the ComplexArgumentType, and in SimpleArgumentType just return the value casted to T)

I think a breaking change here would be preferable to really ugly argument type organization. (and might actually be necessary if you implement asynchrony in your implementation, seeing as the whole thing seems to be unmaintained, with the latest commit nearing 2 years ago, it wouldn't be that big a deal to implement such a change on a fork of it, seeing as nobody relies on your fork yet anyways, so no code would actually be broken by it)

Also, you might find yourself wondering why i didn't mention suggestions (those could be sender specific, especially what if you want async?) when describing my argument type suggestions, and this is simply because i haven't implemented suggestions yet, which is also why i didn't release this yet when it might be seemingly complete (because it is, it can dispatch commands just fine at this point in time)

[Gamebuster19901]

I don't know if anyone is even going to find this useful, but here it goes I guess.

I needed context when parsing to check if a string matched the effective name of any user in a discord server. This is to prevent a user from specifying a name for a user they don't share a server with. It's also needed because if you don't have the context, you can't parse by the player's nickname, you can parse by their actual name.

I modified brigadier to pass in the context when parsing arguments. It seems to work for me. It is obviously not compatible with Mojang's brigadier.

Fork is here if anyone is interested: https://github.com/TheGameCommunity/brigadier

[Martmists-GH]

I guess I'll link my solution as well; I basically wrote my own implementation of a command parser, loosely based on Brigadier for use with a Kotlin DSL: https://github.com/Martmists-GH/command_parser

[haykam821]

Does #151 implement this functionality?