Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidation of abstracting away from Type reliance #19

Open
VelvetToroyashi opened this issue Nov 25, 2022 · 0 comments · May be fixed by #20
Open

Consolidation of abstracting away from Type reliance #19

VelvetToroyashi opened this issue Nov 25, 2022 · 0 comments · May be fixed by #20

Comments

@VelvetToroyashi
Copy link
Contributor

VelvetToroyashi commented Nov 25, 2022

This issue is an addendum to #2, though focuses more broadly on the entire library.

Note
Abstract (tl;dr for skim-readers)
The premise of this idea to increase the flexibility of commands in Remora.Commands by abstracting away the implementation detail currently present in CommandNode which includes a Type and MethodInfo and replacing it with a larger object that contains all relevant data about a command, such as its attributes, conditions, and parameters. In addition, a set of builders would be added to introduce a fluent API to construct these commands, which may or not be rooted to a CommandGroup (the 'normal' way of creating a command today).

Discord.NET, as insane of a library as it is, does occasionally spark good ideas.

Something that even DSharpPlus has achieved is the ability to dynamically define commands at runtime, without the need to be bound to a specific Type, which in Remora.Commands is used to pull an instance from the container and invoke its MethodInfo.

I want to preface this issue by saying this will be a MASSIVE undertaking, relatively speaking, as it requires uprooting a majority of the library's code, and even rewriting parts of the glue-code present in Remora.Discord.Commands

For obvious reasons, this is not something that can simply be done in a day, nor do I expect it to be, if ever. There are solid benefits to be had with this, though, which will be laid out below.


The What:

Remora.Commands is a great library, truly. Lightweight, performant, and quite enjoyable to use, and easy-to-use.

The issues arise from how the library functions, however. A simple parent-child tree system, which actually works quite well, but has short-comings. This system is very rigid, first and foremost. This is OK for a majority of use-cases; its primary function is served well by the current architecture, but when it comes to, say, generating delegate-bound commands, this just crumbles.


The Why:

There are probably an infinite number of examples that could be given for this, but as mentioned above, delegate-bound commands are probably the most tangible; be it for testing, or dynamic creation of commands, there's currently no way for Remora to handle this; the library expects a CommandNode with a given Type and MethodInfo that it can invoke (which is quite slow; I'll touch on that further down).

There are also runtime benefits to ditching the current tree system. As it stands, I wager we can bite the bullet for some extra, one-time allocations (Trees are non-ephemeral by nature). If instead, we treat CommandNode as a Func<IServiceCollection, object[], Task>, the implementation details become irrelevant to Remora.Commands; it simply invokes the delegate (which, under normal circumstances could be a compiled expression to replicate the current behavior of pulling from the container).

This in and of itself may not appear to be much of a motivator given the amount of work required to achieve this, but there's more to it. CommandNode (and by extension GroupNode) would be expanded to capture (or contain in the case of non-class-bound commands) all the metadata about a command, instead of performing reflection on every invocation.

This would allow for grabbing:

  • Conditions
  • Permissions
  • Parameters
  • Meta attributes (e.g. [ExcludeFromSlashCommands], [CommandType], and [SupressInteractionResponse])

Once, and referencing them via a list/property on some kind of metadata object (or CommandNode itself).
This in and of itself is a valuable improvement, in my opinion. As explained further down, this behavior also allows for further customizability by end-users by extending the library's functionality without the need for forking.


The How:

The biggest issue will definitely be rewriting glue-code, and handling searching/executing commands.

Actually rewriting GroupNode and ChildNode should prove rather trivial.

I imagine following in the footsteps of Discord.NET and DSharpPlus and having a builder system (much like there's already TreeBuilder, which holds an array of Type, which is then used to determine how to build the tree).

The pseudo-API would look something along the lines of this:

// CommandNode.cs
public class CommandNode : IChildNode
{
    /// <summary>
    /// The name of the command.
    /// </summary>
    public string Name { get; }

    /// <summary>
    /// The description of the command.
    /// </summary>
    public string Description { get; }

    /// <summary>
    /// The aliases of the command.
    /// </summary>
    public IReadOnlyList<string> Aliases { get; }

    /// <summary>
    /// The attributes of the command.
    /// </summary>
    public IReadOnlyList<Attribute> Attributes { get; }

    /// <summary>
    /// The parameters of the command, including metadata about the parameter itself.
    /// </summary>
    // NOTE: CommandShape already exists here, but this is just a name for the sake of
    // example.
    public IReadOnlyList<CommandParameter> Parameters { get; }

    /// <summary>
    /// The parent of the node.
    /// </summary>
    // NOTE: This is nullable because of any runtime tom-foolery that may spawn this node
    public IParentNode? Parent { get; }

    /// <summary>
    /// The delegate to invoke the command. This is responsible for any
    /// required instantiation, such as resolving the command from the IoC
    /// container, and invoking the command method itself.
    /// </summary>
    public Func<IServiceCollection, object[], Task<IResult> InvocationDelegate { get; }
}
// CommandParameter.cs
public class CommandParameter
{
    /// <summary>
    /// The name of this parameter, if any.
    /// </summary>
    public string? Name { get; }
    
    /// <summary>
    /// The description of this parameter, if any.
    /// </summary>
    public string? Description { get; }

    /// <summary>
    /// Whether this parameter is optional
    /// </summary>
    public bool IsOptional { get; }

    /// <summary>
    /// Whether this parameter is greedy.
    /// </summary>
    public bool IsGreedy { get; }

    /// <summary>
    /// The default value of this parameter, if any.
    /// </summary>
    public virtual object? DefaultValue { get; }

    /// <summary>
    /// The type of the parameter.
    /// </summary>
    // No need for an entire `ParameterInfo` when we only use two properties* from it.
    public Type ParameterType { get; }

    /// <summary>
    /// The conditions applied to this parameter, if any.
    /// </summary>
    public IReadOnlyList<ConditionAttribute> Conditions { get; }

    /// <summary>
    /// The attributes applied to this parameter
    /// </summary>
    public IReadOnlyList<Attribute> Attributes { get; }
}
// CommandBuilder.cs
public class CommandBuilder
{
    private string _name;
    private string? _description;

    private readonly GroupBuilder? _builder;

    private readonly  List<string> _aliases;
    private readonly  List<Attribute> _attributes;
    internal readonly List<ParameterBuilder> _parameters;
    private readonly  List<ConditionAttribute> _conditions;

    private Func<IServiceCollection, object[], Task> _invocation;

    /// <summary>
    /// Sets the name of the command.
    /// </summary>
    public CommandBuilder WithName(string name) {}

    /// <summary>
    /// Sets the description of the command.
    /// </summary>
    public CommandBuilder WithDescription(string description) {}

    /// <summary>
    /// Adds an alias to the command.
    /// </summary>
    public CommandBuilder AddAlias(string alias) {}

    /// <summary>
    /// Adds multiple aliases to the command.
    /// </summary>
    public CommandBuilder AddAliases(IEnumerable<string> aliases) {}

    /// <summary>
    /// Adds an attribute to the command. Conditions must be added via <see cref="AddCondition{T}"/>.
    /// </summary>
    public CommandBuilder AddAttribute<T>(T attribute) where T : Attribute {}

    /// <summary>
    /// Adds a condition to the command.
    /// </summary>
    public CommandBuilder AddCondition<T>(T condition) where T : ConditionAttribute {}

    /// <summary>
    /// Applies a function in which the command is invoked by.
    /// </summary>
    public CommandBuilder WithInvocation(Func<IServiceProvider, object?[], Task> invocation) {}

    /// <summary>
    /// Adds a parameter to the command.
    /// </summary>
    public ParameterBuilder<T> AddParameter<T>() {}

    /// <summary>
    /// Builds the current <see cref="CommandBuilder"/> into a <see cref="CommandNode"/>.
    /// </summary>
    public CommandNode Build() {}

    /// <summary>
    /// Finishes building the command, and returns the group builder if applicable.
    /// This method should only be called if the instance was generated from <see cref="GroupBuilder.AddCommand"/>.
    /// </summary>
    /// <exception cref="InvalidOperationException">Thrown if the command builder was not associated with a group.</exception>
    public GroupBuilder Finish() {}
}
// CommandGroupBuilder.cs
public class GroupBuilder 
{
    private string _name;
    private string? _description;

    private readonly List<string>             _groupAliases;
    private readonly List<Attribute>          _groupAttributes;
    private readonly List<ConditionAttribute> _groupConditions;
    
    private readonly List<OneOf<CommandBuilder, GroupBuilder>> _children;

    /// <summary>
    /// Sets the name of the group.
    /// </summary>
    public GroupBuilder WithName(string name) {}

    /// <summary>
    /// Sets the description of the group.
    /// </summary>
    public GroupBuilder WithDescription(string description) {}

    /// <summary>
    /// Adds an alias to the group.
    /// </summary>
    public GroupBuilder AddAlias(string alias) {}

    /// <summary>
    /// Adds multiple aliases to the group.
    /// </summary>
    public GroupBuilder AddAliases(IEnumerable<string> aliases) {}

    /// <summary>
    /// Adds an attribute to the group. Conditions should be added via <see cref="AddCondition"/>.
    /// </summary>
    public GroupBuilder AddAttriubte(Attribute attribute) {}

    /// <summary>
    /// Adds a condition to the group.
    /// </summary>
    public GroupBuilder AddCondition(ConditionAttribute attribute) {}

    /// <summary>
    ///  Adds a command to the group.
    /// </summary>
    /// <returns>
    ///  A <see cref="CommandBuilder"> to build the command with. 
    ///  Call <see cref="CommandBuilder.Finish"> to retrieve the group builder.
    /// </returns>
    public CommandBuilder AddCommand() {}

    public GroupBuilder AddGroup()
    {
        var builder = new GroupBuilder(this);
        _children.Add(OneOf<CommandBuilder, GroupBuilder>.FromT1(builder)); // new() would also work.

        return builder;
    }

    /// <summary>
    /// Builds the current <see cref="GroupBuilder"/> into a <see cref="GroupNode"/>.
    /// </summary>
    public GroupNode Build() {}
}
//ParameterBuilder.cs

public /* abstract? */ class ParameterBuilder
{
    private protected string? _name;
    private protected bool    _isGreedy;
    private protected bool    _isOptional; // Implicitly set when `WithDefaultValue` is called.
    private protected string? _description;
    private protected object? _defaultValue;

    private Type _parameterType;
    
    private protected readonly List<Attribute>          _attributes;
    private protected readonly List<ConditionAttribute> _conditions;

    private protected readonly CommandBuilder _builder;

    /// <summary>
    /// Sets the name of the parameter.
    /// </summary>
    public ParameterBuilder WithName(string name) {}

    /// <summary>
    /// Sets the description of the parameter.
    /// </summary>
    public ParameterBuilder WithDescription(string description) {}

    /// <summary>
    /// Sets the default value of the parameter. This must match the parameter's type.
    /// </summary>
    /// <remarks>
    /// Invoking this method will mark the parameter as optional regardless of the value
    /// passed.
    /// </remarks>
    public ParameterBuilder WithDefaultValue(object? value) {}

    /// <summary>
    /// Sets the parameter to be greedy.
    /// </summary>
    public ParameterBuilder IsGreedy() {}

    // public ParameterBuilder.IsSwitch() (?)

    /// <summary>
    /// Adds an attribute to the parameter. Conditions must be added via <see cref="AddCondition{T}"/>.
    /// </summary>
    public ParameterBuilder AddAttribute<T>(T attribute) where T : Attribute {}

    /// <summary>
    /// Adds a condition to the parameter.
    /// </summary>
    public ParameterBuilder AddCondition<T>(T condition) where T : ConditionAttribute {}

    public CommandBuilder Finish() 
    {
        // Perhaps the builder should add it manually, and ParameterBuilder just holds a ref to return?
        // Akin to CommandTreeBuilder with it's service collection ref.
        _builder._parameters.Add(this);
        return _builder;
    }

    /// <summary>
    /// Builds the current builder into a <see cref="CommandParameter"/>.
    /// </summary>
    public CommandParameter Build() {}
}

public class ParameterBuilder<T> : ParameterBuilder
{
    public ParameterBuilder<T>(CommandBuilder builder) : base(builder)
    {
        // Of questionable viability, but something ot think of nonetheless
        base.ParameterType = typeof(T);
    }

    /// <inheritdoc/>
    public ParameterBuilder WithName(string name) {}

    /// <inheritdoc/>
    public ParameterBuilder WithDescription(string description) {}

    /// <summary>
    /// Sets a default value of type <typeparam name="T"/> for the parameter. 
    /// This can be set to null (the default) for no default value. 
    /// </summary>
    /// <inheritdoc/>
    public ParameterBuilder<T> WithDefaultValue<T>(T? value) {}

    /// <inheritdoc/>
    public ParameterBuilder<T> IsGreedy() {}

    /// <inheritdoc/>
    public ParameterBuilder AddAttribute<TAttribute>(TAttribute attribute) where TAttribute : Attribute {}

    /// <inheritdoc/>
    public ParameterBuilder AddCondition<TAttribute>(TAttribute condition) where TAttribute : ConditionAttribute {}

    /// <inheritdoc/>
    public CommandParameter Build() {}
}

This is a slightly cut down version of what I actually had in mind becuase it was 4:20 at the time of writing, but there'd also probably be a reference to IServiceCollection in there for good measure? .CreateCommand[Group] would be an additional extension method on IServiceCollection, with the .Finish/.Build calls actually registering themselves into the tree builder.

Alternatively, however (and what I had in mind?) is that registration code has to change very little (perhaps still writing those extension methods for those that want them, but mweh). Either way, Remora would, for default commands, generate the function necessary for invoking the command, and it's mostly similar to what Remora already does anyways:

private IEnumerable<IChildNode> ToChildNodes(IEnumerable<Type> moduleTypes, IParentNode parent)
{
IEnumerable<IGrouping<string, Type>> groups = moduleTypes.GroupBy
(
mt => mt.TryGetGroupName(out var name) ? name : string.Empty
);
foreach (var group in groups)
{
if (group.Key == string.Empty)
{
// Nest these commands and groups directly under the parent
foreach (var groupType in group)
{
var subgroups = groupType.GetNestedTypes().Where(t => typeof(CommandGroup).IsAssignableFrom(t));
// Extract submodules and commands
foreach (var child in GetModuleCommands(groupType, parent))
{
yield return child;
}
foreach (var child in ToChildNodes(subgroups, parent))
{
yield return child;
}
}
}
else
{
// Nest these commands and groups under a subgroup
var groupChildren = new List<IChildNode>();
var groupAliases = new List<string>();
// Pick out the first custom description
var description = group
.Select(t => t.GetDescriptionOrDefault("MARKER"))
.Distinct()
.FirstOrDefault(d => d != "MARKER");
description ??= Constants.DefaultDescription;
var groupNode = new GroupNode(group.ToArray(), groupChildren, parent, group.Key, groupAliases, description);
foreach (var groupType in group)
{
// Union the aliases of the groups under this key
var groupAttribute = groupType.GetCustomAttribute<GroupAttribute>();
if (groupAttribute is null)
{
throw new InvalidOperationException();
}
foreach (var alias in groupAttribute.Aliases)
{
if (groupAliases.Contains(alias))
{
continue;
}
groupAliases.Add(alias);
}
var subgroups = groupType.GetNestedTypes().Where(t => typeof(CommandGroup).IsAssignableFrom(t));
// Extract submodules and commands
groupChildren.AddRange(GetModuleCommands(groupType, groupNode));
groupChildren.AddRange(ToChildNodes(subgroups, groupNode));
}
yield return groupNode;
}
}
}

I'll certainly go more in depth into this tomorrow (maybe). But for now I just wanted to get this idea off my mind and into a proper issue.

Also thanks to @uwx for the intial idea of compiled delegates. Unfortunately (ish), I can't imagine you would've thought that it would've spiraled into this enormous thing, but I think this is a better solution than "just" adding compiled delegates (we'll still use them, probably).

@VelvetToroyashi VelvetToroyashi linked a pull request Nov 25, 2022 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant