Skip to content

Property aliases don't work right with super #18462

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Sep 12, 2020

PR Summary

While working on #18189, I had to convert some self.set_bar(...) to super().set_bar(...), but it turned out that bar was an alias for foo, and it kept calling Object3D.set_foo instead of Object.set_foo. This was easy enough to convert to the original non-alias property setters, but maybe this should work?

I haven't quite figured out how to actually fix it though, so consider this a bug report in the form of a broken test (test_define_aliases_subclass). cc @anntzer I think wrote this originally.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • [n/a] New features are documented, with examples if plot related.
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

@anntzer
Copy link
Contributor

anntzer commented Sep 12, 2020

At first glance, this is not really a problem with the aliasing machinery, right? Even if we wrote everything manually

class Base:
    def set_prop(self, val): ...  # base class implementation
    def set_alias(self, val): return self.set_prop(val)

class Derived:
    def set_prop(self, val): ...  # derived class implementation

calling Base.set_alias(Derived()) would result in a call to Derived.set_prop, not a call to Base.set_prop. On the manual implementation side fixing this would basically require writing

class Base:
    def set_prop(self, val): ...  # base class implementation
    def set_alias(self, val): return Base.set_prop(self, val)  # explicitly use *this* class

class Derived:
    def set_prop(self, val): ...  # derived class implementation
    def set_alias(self, val): return Derived.set_prop(self, val)  # explicitly redeclare alias

When using the automatic machinery I could imagine making aliases not functions but custom descriptors with a __get__ that lets them know not only what instance they're called on but also what class, and then resolved the alias based on that, along the lines of (untested)

class _Alias:
    def __init__(self, real_name):
        self._real_name = real_name

    def __get__(self, instance, owner):
        return getattr(owner, self._real_name).__get__(instance, owner)

(plus whatever is needed to fake __doc__ and __name__ and whatnot) but do we really want to go there?

(Or did I completely miss something?)

@QuLogic
Copy link
Member Author

QuLogic commented Sep 17, 2020

At first glance, this is not really a problem with the aliasing machinery, right? Even if we wrote everything manually

The difference is that aliases are supposed to be (at least to me) transparently equivalent. They should work exactly the same as the main version. The user has no idea they're calling an alias, but if the alias is called for the middle of the class hierarchy, it should not magically call the real version at the end of the hierarchy.

@anntzer
Copy link
Contributor

anntzer commented Sep 17, 2020

I don't have a really strong opinion there. If we do want to fix this, do you agree that the custom descriptor is the way to go, or can you think of something simpler? With the descriptor approach, the following patch fixes the tests AFAICT, feel free to adopt it:

diff --git i/lib/matplotlib/artist.py w/lib/matplotlib/artist.py
index 274a35ea0..6d375c73d 100644
--- i/lib/matplotlib/artist.py
+++ w/lib/matplotlib/artist.py
@@ -1305,7 +1305,7 @@ class ArtistInspector:
                     and callable(getattr(self.o, name))]
         aliases = {}
         for name in names:
-            func = getattr(self.o, name)
+            func = inspect.getattr_static(self.o, name)
             if not self.is_alias(func):
                 continue
             propname = re.search("`({}.*)`".format(name[:4]),  # get_.*/set_.*
diff --git i/lib/matplotlib/cbook/__init__.py w/lib/matplotlib/cbook/__init__.py
index 2b52dd414..04e6b0661 100644
--- i/lib/matplotlib/cbook/__init__.py
+++ w/lib/matplotlib/cbook/__init__.py
@@ -1859,6 +1859,15 @@ def _str_lower_equal(obj, s):
     return isinstance(obj, str) and obj.lower() == s
 
 
+class _Alias:
+    def __init__(self, real_name):
+        self.__name__ = real_name
+        self.__doc__ = f"Alias for `{real_name}`."
+
+    def __get__(self, instance, owner):
+        return getattr(owner, self.__name__).__get__(instance, owner)
+
+
 def _define_aliases(alias_d, cls=None):
     """
     Class decorator for defining property aliases.
@@ -1880,22 +1889,13 @@ def _define_aliases(alias_d, cls=None):
     if cls is None:  # Return the actual class decorator.
         return functools.partial(_define_aliases, alias_d)
 
-    def make_alias(name):  # Enforce a closure over *name*.
-        @functools.wraps(getattr(cls, name))
-        def method(self, *args, **kwargs):
-            return getattr(self, name)(*args, **kwargs)
-        return method
-
     for prop, aliases in alias_d.items():
         exists = False
         for prefix in ["get_", "set_"]:
             if prefix + prop in vars(cls):
                 exists = True
                 for alias in aliases:
-                    method = make_alias(prefix + prop)
-                    method.__name__ = prefix + alias
-                    method.__doc__ = "Alias for `{}`.".format(prefix + prop)
-                    setattr(cls, prefix + alias, method)
+                    setattr(cls, prefix + alias, _Alias(prefix + prop))
         if not exists:
             raise ValueError(
                 "Neither getter nor setter exists for {!r}".format(prop))

@tacaswell
Copy link
Member

My understanding is that the core problem here is that when you do d.set_alias() you want to do late binding to get the derived class version of set_prop, but when you do Base.set_alias you want to have done "early" binding and get the Base class version of set_prop (I tried the "simple" fix of closing over the Base classes's target at class definition time and that just breaks the tests differently as then d.set_alias() falls back to the base classes version 🤦 ).

I agree that if we want to make this work the descriptor approach is the only option (as we need to know the context in which the method has been accessed).

@anntzer
Copy link
Contributor

anntzer commented Sep 23, 2020

I'll let you take over my patch, then :)

@tacaswell
Copy link
Member

Even slowly convincing my self this is a good idea as the other alternatives are a) leave in broken in this way (which is a surprising an subtle kind of broken) b) deprecating all of the aliases. Neither of which is great....

@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants