sopel-irc / sopel

:robot::speech_balloon: An easy-to-use and highly extensible IRC Bot framework. Formerly Willie.
https://sopel.chat
Other
950 stars 405 forks source link

Importing callables from other plugins silently duplicates bot commands #2488

Open SnoopJ opened 1 year ago

SnoopJ commented 1 year ago

Description

The loader machinery treats Sopel callables in a plugin as belonging to that plugin even if the callable is imported from another plugin, which can result in duplication of a command when one plugin is importing from another.

Reproduction steps

I discovered this while writing a plugin that aliases the command w to wp

# wikipedia_alias.py
from sopel import plugin
from sopel.modules.wikipedia import wikipedia as original_wikipedia_cmd

@plugin.command("w")
@plugin.output_prefix("[wikipedia ]")
def wiki_alias(bot, trigger)
    return original_wikipedia_cmd(bot, trigger)

This "unboxing" of the callable from the original plugin can lead to duplication of the wp command.

Expected behavior

The loader should be able to distinguish between callables that are indigenous to a plugin and callables that came from somewhere else, by comparing the __module__ attribute of the callable against the plugin module under consideration.

It is probably too disruptive to skip callables that came from somewhere else (users may be relying on this behavior), but the core could issue a warning about this to make the failure marginally less mysterious.

Relevant logs

No response

Notes

No response

Sopel version

6af4f23

Installation method

pip install

Python version

3.9.16

Operating system

Ubuntu 20.04

IRCd

No response

Relevant plugins

No response

dgw commented 1 year ago

To make the workaround explicit:

Avoiding this is as simple as doing e.g. from sopel.modules import pluginname and then using pluginname.the_callable(), instead of directly doing from sopel.modules.pluginname import the_callable.

SnoopJ commented 1 year ago

@Exirel points out that we can't look at __module__ because plugins may have substructure that the core should respect. It may be possible to introspect if a callable was defined "under" the relevant plugin that is being processed, but the check described above would be too aggressive.

dgw commented 1 year ago

It's like what I said earlier on IRC about "or a submodule"—but with more nuance, I guess? Plugins that import different commands from submodules of themselves to build the "real" plugin file that Sopel loads were what I had in mind. 😁

Exirel commented 1 year ago

To report and to extend on what I said on IRC: I'm not sure to see an issue to resolve here, and I don't think solving said issue would be a good idea, especially given the complexity of the problem.

Sopel makes no promise about plugin's interdependence, but it promises that a plugin callable exposed by a plugin will be loaded for that plugin (note that I say "exposed" here, and not "defined"). Where the plugin callable is defined is never considered. If you think that way, then you understand that Sopel does exactly what is asked: you expose a plugin callable? It'll be loaded for that plugin.

Here is the catch: we can't ensure loading order. Because Sopel makes no promise on interdependence, Sopel also makes no promise on loading order! So we really can't know if a plugin is the "legitimate owner" of a callable, unless we are willing to control a bit too much on Python's internal loading machinery.

That's why I'm really not into this. I'd rather help people to craft better reusable code for their plugin, and be a bit more conscious about what they are doing, than trying to solve the problem for them by throwing more code at it. I think #2489 is a good first step, another step would be to write that "create your own plugin" tutorials I'm thinking about (since forever, I should have done it already).