PR: use single inheritance

[edreamleo]

This PR is based on PR #664, but it retains all tests of the form isinstance(obj, AbstractX).

Differences from PR 664

• Remove is_abstract_* predicates.
  664 shows that these predicates do work, but for this PR I wanted to minimize the changed files.
• Retain the three Abstract classes as empty classes.
  Alas, circular imports prevented using tuples as proxies for the Abstract classes.
• Define _BuiltinElement as a subclass of po.PyObject.
• Add pyobjects.PyObject as an explicit base class wherever an Abstract class appeared.
• As in 664, replace a @staticmethod with a function.
• Simplify various annotations.
• Add docstrings for all Abstract classes.

Discussion

This PR shows that:

• Using single inheritance for the classes in builtins.py, pyobjects.py and pyobjectsdef is straightforward.
  • PyDefinedObject is now a subclass of po.PyObject. This change simplifies annotations.
  • The type hierarchy is natural: everything is a po.PyObject!
• The legacy Abstract classes were faux helpers:
  • They obscured the true class hierarchy and complicated the ctors of various classes.
    The revised ctors all use super().__init__. As a result, the ctors are easier to understand.
  • They defined default methods that were more naturally defined elsewhere.
• The new (empty!) Abstract classes have only one function: to tag various classes.
  The Abstract classes now have docstrings explaining why they exist.

[edreamleo]

These remarks are a reply to a discussion started in PR #675. They apply equally to this PR.

@lieryan said:
QQQ
This change does not look correct to me.

AbstractFunction classes defines the interface for what it means to be a function, it just provides a default implementation so that subclasses don't have to implement anything if it doesn't need to implement something, ditto with the others.

Defining empty interface immediately screams that something is wrong with the new version of Abstract classes.
QQQ

The three Abstract classes act as markers:

• These classes provide the basis for tests such as isinstance(obj, AbstractFunction).
  These tests form a crucial part of Rope's inference machinery.
• The Abstract classes aren't needed: a separate PR could replace the isinstance test above with:

getattr(obj, "type_", None) == "Function"

Summary

• Rope's unit tests prove there is nothing wrong with this PR :-)
• I would be happy to eliminate the Abstract classes entirely!
• The first comment of this PR explains its benefits. I won't repeat that list here.
• If you reject this PR, the Theory of Operation must explain why Rope uses a confusing maze of mixin classes when a simpler scheme works just as well.

[edreamleo]

@lieryan A word about workflow: I prefer to use "good enough" PRs. This speeds development. I would much rather tweak code in later PRs than wait for perfection.

[edreamleo]

@lieryan Afaik super().__init__() does not work with plugin classes. Here is a test:

# Super Works with known inheritance.
class A:
    def __init__(self, a):
        print('A1', a)
        super().__init__()
        print('A2')

class B(A):
    def __init__(self, a, b):
        print('B1', b)
        super().__init__(a)
        print('B2')

class C(B):
    def __init__(self):
        print('C1')
        super().__init__('a', 'b')
        print('C2')

C()

print('-----')
        
# super() does not work with mixins.
class E:
    def __init__(self, e):
        print('E', e)

class F:
    def __init__(self, f):
        print('F', f)

class G(E, F):
    def __init__(self):
        print('G')
        E.__init__(self, 'e')
        F.__init__(self, 'f')

G()

The output is:

C1
B1 b
A1 a
A2
B2
C2
-----
G
E e
F f

I know of no way to use super().__init__ in G.__init__. Do you?

[lieryan]

super() works fine with mixins, it will just follows the MRO of the actual runtime type rather than the lexical type, that is how super() is designed to work.

When a class uses super() for multiple inheritance, the classes need to be designed to work cooperatively. The rule of using super() with multiple inheritances are:

1. When a method uses super() to do supercalls, all occurences of that method in the class hierarchy must also use super(). You can't mix the E.__init__() style of calling superclass with super().
2. Methods that calls super() usually would need to pass along *args and **kwargs
3. When overriding methods that aren't in object, it is usually necessary to have a terminal implementation that would eat the last method call by not calling super()
4. Order of inheritance is important, class G(E, F) and class G(F, E) are not the same. When multiple classes are designed to be inherited together often, you usually would want to use a consistent ordering whenever they appear together.

Your second example violates a number of those rules, so that's why it's not working correctly.

Combined together, the correct usage of super() here would be:

class E:
    def __init__(self, e, *args, **kwargs):
        print('E', e)
        super().__init__(*args, **kwargs)

class F:
    def __init__(self, f, *args, **kwargs):
        print('F', f)
        super().__init__(*args, **kwargs)

class G(E, F):
    def __init__(self, *args, **kwargs):
        print('G')
        super().__init__('e', 'f', *args, **kwargs)

# output:
# G
# E e
# F f

Raymond Hettinger's "Python's super() considered super()!" is an old article, but it is still one of the best article describing the practical use of super() in various multiple inheritance scenarios.

[edreamleo]

@lieryan Thanks for these comments about super.

When a class uses super() for multiple inheritance, the classes need to be designed to work cooperatively.

Single inheritance sidesteps the need for "cooperation". Everything just works.

[edreamleo]

@lieryan I wonder whether (in this branch) the annotation Union[PyObject, PyDefinedObject] is any different from plain PyObject. The Union is more explicit, but is there any circumstance in which simpler annotation would act differently? I'll create a test file and see what mypy says.

[edreamleo]

@lieryan pyobjects.PyModule is also an empty class. Rope uses po.PyModule as a target of isinstance tests, just like the Abstract classes.

The only difference is that the Abstract classes are mixins while po.PyModule is explicitly part of the class hierarchy (it is a subclass of po._PyModule).

In other words, Rope already uses an empty target class. I'll add a docstring to po.PyModule.

Update: Done at rev b8ec122.

[edreamleo]

@lieryan The following test file passes mypy:

"""Is there any advantage to using Union[A, B] if B is a subclass of A?"""

from typing import Union

class A:
    def __init__(self, a):
        self.a: int = a
    
class B(A):
    def __init__(self, a, b):
        super().__init__(a)
        self.b: float = b
    
a1 = A(1)
a2: A = A(2)
a3: Union[A, B] = A(3)  # Dubious annotation.

b1 = B(4, 1.0)
b2: B = B(5, 5.0)
b3: Union[A, B] = B(6, 6.0)
b4: A = B(7, 7.0)  # Dubious annotation.

My (tentative) takeaway is that mypy does not force us to be a specific as possible:

• mypy won't complain if we annotate everything as a po.PyObject.
• If we expect a PyDefinedObject we had better say so explicitly.