disnake icon indicating copy to clipboard operation
disnake copied to clipboard

Fix app command copies

Open EQUENOS opened this issue 3 years ago • 1 comments

Summary

This PR fixes app command copies. More specifically, we now ensure that InvokableApplicationCommand.body attribute is copied. This fixes the following issue:

If you use the commands.default_member_permissions decorator after the commands.slash_command decorator in a cog, the final command won't have any member permissions set.

Steps to reproduce

from disnake.ext import commands

bot = commands.InteractionBot()

class TestCog(commands.Cog):
    @commands.default_member_permissions(manage_guild=True)
    @commands.slash_command()
    async def bugged(self, _):
        ...

    @commands.slash_command()
    @commands.default_member_permissions(manage_guild=True)
    async def workaround(self, _):
        ...

bot.add_cog(TestCog())
for cmd in bot.application_commands_iterator():
    print(cmd.name, cmd.default_member_permissions)

This code will display permissions of bugged as None and permissions of workaround as <Permissions ...>

Explanation

Cogs allow to pass "global" kwargs to all cog commands via meta options called <...>_command_attrs, see CogMeta.command_attrs. In order to catch those kwargs, disnake rebuilds all commands of a given cog in Cog.__new__. It does so by constructing new command instances, passing original kwargs merged with <...>_command_attrs. However, some attributes are not being set in <...>Command.__init__. Instead, they're updated afterwards via decorators and other tools, so we have to copy them separately (this is why all <...>Command classes have a method called _ensure_assignment_on_copy). If we apply commands.default_member_permissions after commands.slash_command, original kwargs won't have the correct default permission value, they'll have it as None instead. So we have to copy that value manually, which is what this PR does.

Checklist

  • [x] If code changes were made, then they have been tested
    • [ ] I have updated the documentation to reflect the changes
    • [x] I have formatted the code properly by running task lint
    • [x] I have type-checked the code by running task pyright
  • [x] This PR fixes an issue
  • [ ] This PR adds something new (e.g. new method or parameters)
  • [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • [ ] This PR is not a code change (e.g. documentation, README, ...)

EQUENOS avatar Jul 29 '22 21:07 EQUENOS

Since the body is now copied too, does this affect this part of the code for slash commands?

It shouldn't affect it, however, I think I will change copy(body) to copying only those attributes of body that can be changed via decorators

EQUENOS avatar Aug 03 '22 10:08 EQUENOS

This unfortunately doesn't resolve the issue of not being able to overwrite default_member_permissions with None, but that's a side effect of how x_attrs is implemented and can likely only be fixed as part of a general refactor:

Is there not a way to unset those default member permissions at all when creating the function?

onerandomusername avatar Aug 24 '22 23:08 onerandomusername

Is there not a way to unset those default member permissions at all when creating the function?

Currently not, since it can't differentiate between the parameter default None (which we want to ignore when copying) and a user-provided None (which we don't want to ignore) - the relevant TODO is this one: https://github.com/DisnakeDev/disnake/blob/4c17fdc45a88dffd215db6b824c8f3e88dfe81b3/disnake/ext/commands/base_core.py#L147-L151 This isn't an issue with prefix commands, as their implementation uses **kwargs for literally everything except the name, which in turn results in little to no type-safety.

shiftinv avatar Aug 25 '22 11:08 shiftinv

that's a side effect of how x_attrs is implemented and can likely only be fixed as part of a general refactor

I can do that in a separate PR

EQUENOS avatar Aug 25 '22 13:08 EQUENOS

I can do that in a separate PR

Sounds good. I don't think there are any concrete plans for that yet, but it would be a good idea to take care of #666 first which also refactors parts of the application command system (though mostly related to syncing).

shiftinv avatar Aug 25 '22 13:08 shiftinv