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

BUG: Fix broken multiple inheritance in ScriptedLoadableModule classes #6243

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

allemangD
Copy link
Contributor

Missing calls to super().__init__() in the ScriptedLoadableModule classes break multiple inheritance. The init method is skipped in any derived classes that come after the ScriptedLoadableModule class in the MRO.

Adding super calls to each of the ScriptedLoadableModule classes fixes this issue and correctly supports multiple inheritance.

  • ScriptedLoadableModule.__init__
  • ScriptedLoadableModuleWidget.__init__
  • ScriptedLoadableModuleLogic.__init__

For example, this class fails to initialize VTKObservationMixin:

class MyModuleLogic(ScriptedLoadableModuleLogic, VTKObservationMixin):
  def __init__(self):
    super().__init__()
    
    self.addObserver(...)  # AttributeError on self.Observations

In this case, swapping the base class order avoids the error as VTKObservationMixin does include a super call, however that solution is not perfect. MyModuleLogic.__base__ is not ScriptedLoadableModuleLogic as it should be, and it is impossible to pass the optional parent parameter to ScriptedLoadableModuleLogic.__init__.


This change should not have any backwards compatibility issues, as currently the only way to support mixins is to explicitly call the mixin initializer, in which case the behavior of super() is ignored.


ScriptedLoadableModuleTest cannot be fixed because its base class, unittest.TestCase, does not have a super call. For such cases it would still be necessary to explicitly call the mixin initializer.

Missing calls to `super().__init__()` in the `ScriptedLoadableModule` classes break multiple inheritance. The init method is skipped in any derived classes that come after the ScriptedLoadableModule class in the MRO.

Adding `super().__init__()` calls to each of the ScriptedLoadableModule classes fixes this issue and correctly supports multiple inheritance.

- ScriptedLoadableModule.__init__
- ScriptedLoadableModuleWidget.__init__
- ScriptedLoadableModuleLogic.__init__
@jcfr
Copy link
Member

jcfr commented Mar 7, 2022

Thanks for the detailed explanation 🙏

Following b2ba482 (ENH: Add support for building Python 3.6) removing support for python 2.7, only new style python classes are supported and using super() makes sense.

For references:

@jcfr jcfr merged commit 917c02e into Slicer:master Mar 7, 2022
@allemangD
Copy link
Contributor Author

allemangD commented Mar 8, 2022

ScriptedLoadableModuleTest cannot be fixed because its base class, unittest.TestCase, does not have a super call. For such cases it would still be necessary to explicitly call the mixin initializer.

https://docs.python.org/3/library/functions.html#super

Reading through that documentation more carefully, it seems it is possible to fix the behavior for ScriptedLoadableModuleTest as well. @jcfr thanks for posting the link! It helps to read things even if you already know the bulk of it, I learned something new!

The object-or-type determines the method resolution order to be searched. The search starts from the class right after the type.

This indicates that super(UnitTest, self) would take the next base type of self after UnitTest. Below is a minimal example of the class hierarchy:

class Mixin:
    def __init__(self):
        super().__init__()
        
        print('Mixin')


class UnitTest:
    def __init__(self):
        # super call cannot be added.
        
        print('UnitTest')


class ModuleTest(UnitTest):
    def __init__(self):
        # next class in MRO after UnitTest (Mixin.__init__)
        super(UnitTest, self).__init__() 
        
        # immediate next class in MRO (UnitTest.__init__) 
        super(ModuleTest, self).__init__()

        print('ModuleTest')


class MyModuleTest(ModuleTest, Mixin):
    def __init__(self):
        super().__init__()

        print('MyModuleTest')

_ = MyModuleTest()  # Mixin / UnitTest / ModuleTest / MyModuleTest

There is a risk in this implementation: if a super call is ever added to the UnitTest class, then Mixin.__init__ (and object.__init__) would be invoked twice.

I will add commits to this branch introducing the above change, along with a test to check for regression that ensures Mixin.__init__ is only ever called once.

allemangD added a commit to allemangD/Slicer that referenced this pull request Mar 8, 2022
See Slicer#6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
allemangD added a commit to allemangD/Slicer that referenced this pull request Mar 8, 2022
…asses.

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
allemangD added a commit to allemangD/Slicer that referenced this pull request Mar 8, 2022
See Slicer#6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
allemangD added a commit to allemangD/Slicer that referenced this pull request Mar 8, 2022
…asses.

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
allemangD added a commit to allemangD/Slicer that referenced this pull request Jun 7, 2022
See Slicer#6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
allemangD added a commit to allemangD/Slicer that referenced this pull request Jun 7, 2022
…asses.

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
allemangD added a commit to allemangD/Slicer that referenced this pull request Jun 7, 2022
…asses.

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleWidgetMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly once in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
jcfr pushed a commit that referenced this pull request Jun 7, 2022
See #6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
jcfr pushed a commit that referenced this pull request Jun 7, 2022
…asses.

See #6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleWidgetMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly once in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
jcfr pushed a commit to jcfr/Slicer that referenced this pull request Jun 30, 2022
See Slicer#6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
jcfr pushed a commit to jcfr/Slicer that referenced this pull request Jun 30, 2022
…asses.

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleWidgetMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly once in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
jcfr pushed a commit to jcfr/Slicer that referenced this pull request Jul 7, 2022
(cherry picked from commit 7cfa0fc)

See Slicer#6243 (comment) for more information.

`unittest.TestCase` does not include a `super().__init__()` call, so we must invoke both `TestCase.__init__` along with the `__init__` for any mixins of derived classes.

`super(unittest.TestCase, self).__init__()` explicitly calls the `__init__` of the next class in the MRO, _after `unittest.TestCase`_; so adding this second `__init__` call solves the issue.

Thus, this is now supported:

```python
class MyModuleTest(ScriptedLoadableModuleTest, VTKObservationMixin):
   def runTest(self):
     self.addObserver(...)
```
jcfr pushed a commit to jcfr/Slicer that referenced this pull request Jul 7, 2022
…asses.

(cherry picked from commit c59fd74)

See Slicer#6243 (comment) for more information.

New tests in SlicerUtilVTKObservationMixinTests:

Ensure VTKObservationMixin functions in combination with the ScriptedLoadableModule classes:

- test_moduleWidgetMixin
- test_moduleLogicMixin
- test_moduleTestMixin

Ensure VTKObservationMixin.__init__ called exactly once in combination with ScriptedLoadableModuleTest:

- test_moduleTestInitCount
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants