Skip to content

Avoid triggering @property methods on plugins when looking for hookimpls during registration #536

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
39 changes: 37 additions & 2 deletions src/pluggy/_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,17 @@
)


def _attr_is_property(obj: object, name: str) -> bool:
"""Check if a given attr is a @property on a module, class, or object"""
if inspect.ismodule(obj):
return False # modules can never have @property methods

base_class = obj if inspect.isclass(obj) else type(obj)
if isinstance(getattr(base_class, name, None), property):
return True
return False


Check warning on line 59 in src/pluggy/_manager.py

View check run for this annotation

Codecov / codecov/patch

src/pluggy/_manager.py#L59

Added line #L59 was not covered by tests
class PluginValidationError(Exception):
"""Plugin failed validation.

Expand Down Expand Up @@ -181,7 +192,20 @@
customize how hook implementation are picked up. By default, returns the
options for items decorated with :class:`HookimplMarker`.
"""
method: object = getattr(plugin, name)

Check warning on line 195 in src/pluggy/_manager.py

View check run for this annotation

Codecov / codecov/patch

src/pluggy/_manager.py#L195

Added line #L195 was not covered by tests
if _attr_is_property(plugin, name):
# @property methods can have side effects, and are never hookimpls
return None

method: object
try:
method = getattr(plugin, name)
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

If you're able to cook a test for this case that would be nice.

Copy link
Author

Choose a reason for hiding this comment

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

see comment here: #536 (comment)

# AttributeError: '__signature__' attribute of 'plugin' is class-only
# can be raised when trying to access some descriptor/proxied fields
Comment on lines +204 to +205
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# AttributeError: '__signature__' attribute of 'plugin' is class-only
# can be raised when trying to access some descriptor/proxied fields
# May be raised when trying to access some descriptor/proxied fields

Copy link
Author

Choose a reason for hiding this comment

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

did you mean to remove the line with the exception in the suggestion? imo that's crucial to be able to grep through the codebase to find the error / understand why all this logic is here

Copy link
Member

Choose a reason for hiding this comment

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

The reason I suggest to remove it is that the logic handles any AttributeError which may happen for other reasons. The __signature__ thing is one case. It's not very clear what it means. I think if you can make this error happen in a test it will be better.

Copy link
Author

@pirate pirate Feb 5, 2025

Choose a reason for hiding this comment

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

I did here but it was removed from the PR for being too pydantic-specific: https://gist.github.com/pirate/01ca7a6b41595af9a480

Afaik this case is not triggered in any typical code unless you try to use a few specific weird objects w/ descriptors (like Django/Pydantic models) as plugin namespaces. I don't necessarily want to pull in those big libraries to test this escape hatch that just avoids crashing on them, but interaction with those libraries is really the only case where this code path would come up.

# https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032
return None

if not inspect.isroutine(method):
return None
try:
Expand Down Expand Up @@ -286,7 +310,18 @@
customize how hook specifications are picked up. By default, returns the
options for items decorated with :class:`HookspecMarker`.
"""
method = getattr(module_or_class, name)
if _attr_is_property(module_or_class, name):
# @property methods can have side effects, and are never hookspecs

Check warning on line 314 in src/pluggy/_manager.py

View check run for this annotation

Codecov / codecov/patch

src/pluggy/_manager.py#L314

Added line #L314 was not covered by tests
return None

Check warning on line 316 in src/pluggy/_manager.py

View check run for this annotation

Codecov / codecov/patch

src/pluggy/_manager.py#L316

Added line #L316 was not covered by tests
method: object
try:
method = getattr(module_or_class, name)
except AttributeError:

Check warning on line 320 in src/pluggy/_manager.py

View check run for this annotation

Codecov / codecov/patch

src/pluggy/_manager.py#L320

Added line #L320 was not covered by tests
# AttributeError: '__signature__' attribute of <m_or_c> is class-only
# can be raised when trying to access some descriptor/proxied fields
Comment on lines +321 to +322
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# AttributeError: '__signature__' attribute of <m_or_c> is class-only
# can be raised when trying to access some descriptor/proxied fields
# May be raised when trying to access some descriptor/proxied fields

# https://github.com/pytest-dev/pluggy/pull/536#discussion_r1786431032
return None
opts: HookspecOpts | None = getattr(method, self.project_name + "_spec", None)
return opts

Expand Down
17 changes: 17 additions & 0 deletions testing/test_pluginmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,23 @@ class A:
assert pm.register(A(), "somename")


def test_register_ignores_properties(he_pm: PluginManager) -> None:
class ClassWithProperties:
property_was_executed: bool = False

@property
def some_func(self):
self.property_was_executed = True # pragma: no cover
return None # pragma: no cover

# test registering it as a class
he_pm.register(ClassWithProperties)
# test registering it as an instance
test_plugin = ClassWithProperties()
he_pm.register(test_plugin)
assert not test_plugin.property_was_executed


def test_register_mismatch_method(he_pm: PluginManager) -> None:
class hello:
@hookimpl
Expand Down
Loading