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

Importing callables from other plugins silently duplicates bot commands #2488

Open
SnoopJ opened this issue Jul 11, 2023 · 4 comments
Open
Labels
Bug Things to squish; generally used for issues Core/Plugin Handling
Milestone

Comments

@SnoopJ
Copy link
Contributor

SnoopJ commented Jul 11, 2023

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

@SnoopJ SnoopJ added Bug Things to squish; generally used for issues Needs Triage Issues that need to be reviewed and categorized labels Jul 11, 2023
@dgw
Copy link
Member

dgw commented Jul 11, 2023

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 SnoopJ removed the Needs Triage Issues that need to be reviewed and categorized label Jul 11, 2023
@dgw dgw added this to the 8.1.0 milestone Jul 11, 2023
@SnoopJ
Copy link
Contributor Author

SnoopJ commented Jul 11, 2023

@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
Copy link
Member

dgw commented Jul 11, 2023

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
Copy link
Contributor

Exirel commented Jul 13, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Things to squish; generally used for issues Core/Plugin Handling
Projects
None yet
Development

No branches or pull requests

3 participants