Warn if a class has multiple direct parents that each define a method name besides `__init__`

[socketpair]

class A:
   def close(self):
      print('A')

class B:
   def close(self):
      print('B')

class C(A, B):
   def close(self):
      print('C')
      super().close()


C().stop()

Obviously, B.close() is not called. Typically it is a bug. I think Pylint should report such kind of things.

[DanielNoord]

@socketpair Please use the issue template to report bugs. And please don't remove the template after you have just used it, there is reason open source maintainers enforce such templates.

I wonder how many people use this pattern without thinking it is a bug. We have super-init-not-called. Should we extend that cover all methods?

[socketpair]

@DanielNoord

Should we extend that cover all methods?

For sure, we should. It would not be a bug if one of the classes (A or B) did not have the close() method.

Every class has constructor. That's why we have to call them from __init__ of children classes.

[DanielNoord]

What about:

class A:
   def close(self):
      print('A')

class C(A):
   def close(self):
      print('C')

I don't think that should raise right? So it would only cover methods that have a super() call within them that does not cover all parents? That's different for __init__ which should always raise whenever we detect that super() isn't called.

But then again, the method works without calling the parent, whereas the class will be broken if the constructor isn't called.

[socketpair]

@DanielNoord

1. I did not add constructors to all the classes because it is irrelevant to the problem. For a complete example, yes, we should add constructor to C and call both base constructors from it.
2. Regarding your case, in most real cases it IS a bug. It's definitely not a bug if:

class A:
   def close(self):
      raise NotImplementedError

class C(A):
   def close(self):
      print('C')

or

class A:
   @abstractmethod
   def close(self):
      pass

class C(A):
   def close(self):
      print('C')

For the cases when one really want to redefine logic (as in case you provided), yes, he really should add something like # pylint: disable=base-method-not-called.

So, the message should be raised only if the method defined in the __mro__ more than in one class, except the case when all of them are abstract or raise NotImplemented.

[DanielNoord]

Well, then I'm going to be -1 on this proposal (although a smaller proposal I could perhaps agree with).

Overwriting methods of parent functions is perfectly valid and shouldn't raise a linter message.

class Food:
   def type(self):
      print('I'm food')

class Vegetable(Food):
   def type(self):
      print('I'm a vegetable')

class Carrot(Vegetable):
   def type(self):
      print('I'm a carrot')

Overwriting like this can help narrow down the return value in child classes.

[socketpair]

No, I don't agree. The case you provided is correct in general, but in real life, in most cases as I think, not calling base method is an error, again, in cases when the base method is normal.

[jacobtylerwalls]

in real life, in most cases as I think, not calling base method is an error, again, in cases when the base method is normal.

Thanks for the proposal, but I find this likely to be viewed as extremely opinionated. -1

[socketpair]

Okay, if you think so, can we make this check in a way that does not break standard CI systems (low severity or so)? I will set this check as important in our CI.

Also, I strongly believe that redefining method without calling original (where original has some code) is rare. Possibly you have some way to check a new version of Pylint on a large codebase before merging ?

P.S.
This case was really annoying bug that hard to find. That's why I want the check to be in Pylint.

[socketpair]

Actually, there is a mix of problems:

1. Redefining a method without calling super(). This is a case you concern on.
2. Redefining a method with calling super() in cases where both parents have this method. Actually my case.

The second case is VERY rare. And also, having same method in both parents is a bug as I think, unless both of them called explicitly. So the third case appear:

3. If a class has two same-named methods inherited from base classes hierarchy, and it does not redefine the method somehow, then it's a bug. Technically is not (because result depends on order of base classes in class definition), but really it's a bug (that's why linters exist).

[jacobtylerwalls]

@socketpair what do you think of the way I posed a new title for this issue, does it capture your idea? This proposal seems like something we could add to the MisdesignChecker with a Refactoring warning.

[DanielNoord]

@jacobtylerwalls I like this title and actually think it fits as refactoring.
One question do: should this be issued on the class definition line (i.e. the parentage is incorrect and should be fixed) or do we only want to raise this in cases like in the original example? Where the double method name is referenced in a third reimplementation?

[jacobtylerwalls]

on the class definition line (i.e. the parentage is incorrect and should be fixed)

I think it's hard to predict what should be "fixed", that's the nature of refactoring, but the advantage of putting it on the class line is to point users to exactly why pylint complained -- it's that the resulting behavior when using the class is fragile against the specific order you happened to use, and you should opt-in by silencing a warning if you want to accept the consequences.

[socketpair]

@jacobtylerwalls

Warn if a class has multiple direct parents that each define a method name besides init

Yes, but not precise. Warning should be issued only if the class does not reimplement the method. Also, using super().method in reimplementation should not be OK in this case (another warning). So finally we need only two checks:

1. If parent classes have the same-named methods, reimplementation is required. "have" means not only own, but also in class hierarchy. Also, different member types (like method and field) are not allowed to mix.
2. it's not allowed to use super().same_method in reimplementation in case number 1.

If these two checks existed, I would fix the bug much earlier in my code.

class A:
   def close(self):
        print('A')

class B:
   def close(self):
        print('B')

class C(A,B):
   def close(self): # This is OK
        A.close(self)
        B.close(self)
        print('C')

class D(C): # This class should be OK.
    pass # This class does not have `close()` defined in it, and it SHOULD NOT trigger point 1. 

class E(A,B):
    pass # Thi is point 1. The line number in warning should be the line where `class` keyword used.

class F(A,B):
   def close(self): # This is OK only to cover case 1.
        super().close()  # this is case 2. (should not be allowed)