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

Fix app command copies #678

Merged
merged 10 commits into from
Aug 25, 2022
Merged

Fix app command copies #678

merged 10 commits into from
Aug 25, 2022

Conversation

EQUENOS
Copy link
Member

@EQUENOS EQUENOS commented Jul 29, 2022

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

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • 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, ...)

@onerandomusername onerandomusername added this to the disnake v2.6 milestone Jul 29, 2022
@onerandomusername onerandomusername added p: medium Medium priority and removed p: low Low priority labels Jul 30, 2022
Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if self.description != other.description and "description" not in other.__original_kwargs__:
# Allows overriding the default description cog-wide.
other.body.description = self.description
if self.options != other.options:
other.body.options = self.options

@EQUENOS
Copy link
Member Author

EQUENOS commented Aug 3, 2022

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

disnake/ext/commands/base_core.py Outdated Show resolved Hide resolved
@shiftinv shiftinv self-requested a review August 24, 2022 19:16
@shiftinv shiftinv force-pushed the fix/app-command-copies-in-cogs branch from c0c1a69 to 5386c20 Compare August 24, 2022 22:50
Copy link
Member

@shiftinv shiftinv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

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:

class Cog(commands.Cog, slash_command_attrs={"default_member_permissions": 64}):
    @commands.slash_command(default_member_permissions=None)
    async def cmd(self, _):
        ...

print(Cog().cmd.default_member_permissions)  # -> 64

Regardless, this does fix the core issue, anything beyond that should go in a future PR c:

@onerandomusername
Copy link
Member

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?

@shiftinv
Copy link
Member

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:

def __new__(cls, *args: Any, **kwargs: Any) -> Self:
self = super().__new__(cls)
# todo: refactor to not require None and change this to be based on the presence of a kwarg
self.__original_kwargs__ = {k: v for k, v in kwargs.items() if v is not None}
return self

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.

@EQUENOS
Copy link
Member Author

EQUENOS commented Aug 25, 2022

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

@shiftinv
Copy link
Member

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 shiftinv enabled auto-merge (squash) August 25, 2022 14:10
@shiftinv shiftinv merged commit 2d9dea5 into master Aug 25, 2022
@shiftinv shiftinv deleted the fix/app-command-copies-in-cogs branch August 25, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: medium Medium priority t: bugfix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants