Repository: Parquery/icontract

Issue:
Invariant on subclass of generic Mapping produces ValueError
An `invariant` decorator on a subclass of a generic `typing.Mapping` class produces a `ValueError`.  Using Python 3.8.5 and icontract 2.3.7.

Example file `foo.py`:
``` python
from icontract import invariant
from typing import Dict, Iterator, Mapping

@invariant(lambda self: True)
class Foo(Mapping[str, int]):
    def __init__(self, table: Dict[str, int]) -> None:
        self._table = table

    def __getitem__(self, key: str) -> int:
        return self._table[key]

    def __iter__(self) -> Iterator[str]:
        return iter(self._table)

    def __len__(self) -> int:
        return len(self._table)

    def __str__(self) -> str:
        return f'{self.__class__.__name__}({self._table})'

print(Foo({'a': 1}))
```

```
$ python foo.py 
Traceback (most recent call last):
  File "foo.py", line 21, in <module>
    print(Foo({'a': 1}))
  File "/tmp/foo/test-env/lib/python3.8/site-packages/icontract/_checkers.py", line 487, in wrapper
    instance = _find_self(param_names=param_names, args=args, kwargs=kwargs)
  File "/tmp/foo/test-env/lib/python3.8/site-packages/icontract/_checkers.py", line 432, in _find_self
    instance_i = param_names.index("self")
ValueError: 'self' is not in list
```
Disabling asssertions removes the error:
```
$ python -O foo.py 
Foo({'a': 1})
```

Discussion:
coveralls: ## Pull Request Test Coverage Report for [Build 475](https://coveralls.io/builds/34999403)

* **10** of **17**   **(58.82%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-0.5%**) to **92.11%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [icontract/_checkers.py](https://coveralls.io/builds/34999403/source?filename=icontract%2F_checkers.py#L435) | 10 | 17 | 58.82%
<!-- | **Total:** | **10** | **17** | **58.82%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/34999403/badge)](https://coveralls.io/builds/34999403) |
| :-- | --: |
| Change from base [Build 472](https://coveralls.io/builds/34944531): |  -0.5% |
| Covered Lines: | 1004 |
| Relevant Lines: | 1090 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
diff --git a/README.rst b/README.rst
index 80615a0..927040a 100644
--- a/README.rst
+++ b/README.rst
@@ -164,7 +164,8 @@ We consider the following methods to be "public":
 * All methods not prefixed with ``_``
 * All magic methods (prefix ``__`` and suffix ``__``)
 
-Class methods can not observe the invariant since they are not associated with an instance of the class.
+Class methods (marked with ``@classmethod`` or special dunders such as ``__new__``) can not observe the invariant
+since they are not associated with an instance of the class.
 
 We exempt ``__getattribute__``, ``__setattr__`` and ``__delattr__`` methods from observing the invariant since
 these functions alter the state of the instance and thus can not be considered "public".
diff --git a/icontract/_checkers.py b/icontract/_checkers.py
index 7882875..83fa17b 100644
--- a/icontract/_checkers.py
+++ b/icontract/_checkers.py
@@ -429,13 +429,16 @@ def decorate_with_checker(func: CallableT) -> CallableT:
 
 def _find_self(param_names: List[str], args: Tuple[Any, ...], kwargs: Dict[str, Any]) -> Any:
     """Find the instance of ``self`` in the arguments."""
-    instance_i = param_names.index("self")
-    if instance_i < len(args):
-        instance = args[instance_i]
-    else:
-        instance = kwargs["self"]
+    instance_i = None
+    try:
+        instance_i = param_names.index("self")
+    except ValueError:
+        pass
 
-    return instance
+    if instance_i is not None:
+        return args[instance_i]
+
+    return kwargs["self"]
 
 
 def _decorate_with_invariants(func: CallableT, is_init: bool) -> CallableT:
@@ -458,8 +461,12 @@ def _decorate_with_invariants(func: CallableT, is_init: bool) -> CallableT:
 
         def wrapper(*args, **kwargs):  # type: ignore
             """Wrap __init__ method of a class by checking the invariants *after* the invocation."""
-            instance = _find_self(param_names=param_names, args=args, kwargs=kwargs)
-            assert instance is not None, "Expected to find `self` in the parameters, but found none."
+            try:
+                instance = _find_self(param_names=param_names, args=args, kwargs=kwargs)
+            except KeyError as err:
+                raise KeyError(("The parameter 'self' could not be found in the call to function {!r}: "
+                                "the param names were {!r}, the args were {!r} and kwargs were {!r}").format(
+                                    func, param_names, args, kwargs)) from err
 
             # We need to disable the invariants check during the constructor.
             id_instance = str(id(instance))
@@ -481,12 +488,15 @@ def _decorate_with_invariants(func: CallableT, is_init: bool) -> CallableT:
 
         def wrapper(*args, **kwargs):  # type: ignore
             """Wrap a function of a class by checking the invariants *before* and *after* the invocation."""
-            #
+            try:
+                instance = _find_self(param_names=param_names, args=args, kwargs=kwargs)
+            except KeyError as err:
+                raise KeyError(("The parameter 'self' could not be found in the call to function {!r}: "
+                                "the param names were {!r}, the args were {!r} and kwargs were {!r}").format(
+                                    func, param_names, args, kwargs)) from err
+
             # The following dunder indicates whether another invariant is currently being checked. If so,
             # we need to suspend any further invariant check to avoid endless recursion.
-            instance = _find_self(param_names=param_names, args=args, kwargs=kwargs)
-            assert instance is not None, "Expected to find `self` in the parameters, but found none."
-
             id_instance = str(id(instance))
             if not hasattr(_IN_PROGRESS, id_instance):
                 setattr(_IN_PROGRESS, id_instance, True)
@@ -543,10 +553,11 @@ def add_invariant_checks(cls: type) -> None:
 
     # Filter out entries in the directory which are certainly not candidates for decoration.
     for name, value in [(name, getattr(cls, name)) for name in dir(cls)]:
+        # __new__ is a special class method (though not marked properly with @classmethod!).
         # We need to ignore __repr__ to prevent endless loops when generating error messages.
         # __getattribute__, __setattr__ and __delattr__ are too invasive and alter the state of the instance.
         # Hence we don't consider them "public".
-        if name in ["__repr__", "__getattribute__", "__setattr__", "__delattr__"]:
+        if name in ["__new__", "__repr__", "__getattribute__", "__setattr__", "__delattr__"]:
             continue
 
         if name == "__init__":


Test patch:
diff --git a/tests/test_invariant.py b/tests/test_invariant.py
index d45172c..e8e7a67 100644
--- a/tests/test_invariant.py
+++ b/tests/test_invariant.py
@@ -5,7 +5,7 @@
 
 import time
 import unittest
-from typing import Optional  # pylint: disable=unused-import
+from typing import Dict, Iterator, Mapping, Optional, Any  # pylint: disable=unused-import
 
 import icontract
 import tests.error
@@ -157,6 +157,63 @@ class TestOK(unittest.TestCase):
 
         _ = A()
 
+    def test_new_exempted(self) -> None:
+        # This test is related to the issue #167.
+        new_call_counter = 0
+        init_call_counter = 0
+
+        @icontract.invariant(lambda self: True)
+        class Foo:
+            def __new__(cls, *args, **kwargs) -> 'Foo':  # type: ignore
+                nonlocal new_call_counter
+                new_call_counter += 1
+                return super(Foo, cls).__new__(cls)  # type: ignore
+
+            def __init__(self) -> None:
+                nonlocal init_call_counter
+                init_call_counter += 1
+
+        _ = Foo()
+        self.assertEqual(1, new_call_counter)
+        self.assertEqual(1, init_call_counter)
+
+    def test_subclass_of_generic_mapping(self) -> None:
+        # This test is related to the issue #167.
+        counter = 0
+
+        def increase_counter(self: Any) -> bool:
+            nonlocal counter
+            counter += 1
+            return True
+
+        @icontract.invariant(increase_counter)
+        class Foo(Mapping[str, int]):
+            def __init__(self, table: Dict[str, int]) -> None:
+                self._table = table
+
+            def __getitem__(self, key: str) -> int:
+                return self._table[key]
+
+            def __iter__(self) -> Iterator[str]:
+                return iter(self._table)
+
+            def __len__(self) -> int:
+                return len(self._table)
+
+            def __str__(self) -> str:
+                return '{}({})'.format(self.__class__.__name__, self._table)
+
+        f = Foo({'a': 1})  # test the constructor
+        _ = f['a']  # test __getitem__
+        _ = iter(f)  # test __iter__
+        _ = len(f)  # test __len__
+        _ = str(f)  # test __str__
+
+        # 1 invariant check after the constructor +
+        # 4 checks before the methods +
+        # 4 checks after the methods.
+        self.assertEqual(9, counter)
+
 
 class TestViolation(unittest.TestCase):
     def test_init(self) -> None: