Allowed to enforce invariants on attribute setting

[mristin]

Originally, we had enforced invariants only at calls to "normal" methods, and excluded __setattr__ since it is usually too expensive to verify invariants whenever setting an attribute.

However, there are use cases where the users prefer to incur to computational overhead for correctness. To that end, we introduced the feature in this patch to steer when the invariants are enforced (at method calls, on setting attributes, or in both situations).

Fixes #291.

[mristin]

@GithubCamouflaged could you please review this pull request? In particular, could you please double-check that I covered all the obviously relevant test cases?

In addition, could you please check how this change plays with your larger code base? Does it work as intended?

There are currently some minor CI issues, which I'll try to fix in the following days.

[mristin]

@GithubCamouflaged I added a small optimization as well so that the checkers run in linear time without making copies of the invariants.

The CI also passes.

[coveralls]

coverage: 92.924% (+0.1%) from 92.784%
when pulling 9212d87 on mristin/Evaluate-invariants-on-attribute-operations
into 8edda0d on master.

[GithubCamouflaged]

@GithubCamouflaged could you please review this pull request? In particular, could you please double-check that I covered all the obviously relevant test cases?

In addition, could you please check how this change plays with your larger code base? Does it work as intended?

As far as I can discern, through the tests below, this does indeed work as expected!

# -*- coding: utf-8 -*-
import dataclasses
import typing
import icontract

check_on_default=icontract.InvariantCheckEvent.SETATTR | icontract.InvariantCheckEvent.CALL

T = typing.TypeVar('T')
class DataDescriptor(typing.Generic[T]):
    def __set_name__(self, owner:object, name:str):
        self.private_name:str = '_' + name
        self.type_ = self.__orig_class__.__args__[0]

    def __get__(self, obj:object, objtype=None):
        return getattr(obj, self.private_name) if hasattr(obj, self.private_name) else self.type_()

    def __set__(self, obj:object, value) -> None:
        setattr(obj, self.private_name, value)

@icontract.invariant(lambda self: self.parent_dataclass_field > 10, check_on=check_on_default)
@dataclasses.dataclass
class ParentDataClass():
    parent_dataclass_field:int

@icontract.invariant(lambda self: len(self.child_dataclass_field) <= 2)
@dataclasses.dataclass
class ChildDataClass(ParentDataClass):
    child_dataclass_field:str

    def process_child_dataclass_field(self):
        self.child_dataclass_field

@icontract.invariant(
    lambda self: self.parent_attribute in range(0,10),
    check_on=icontract.InvariantCheckEvent.SETATTR | icontract.InvariantCheckEvent.CALL
)
@icontract.invariant(
    lambda self: self.parent_property in ['foo','bar'],
    check_on=icontract.InvariantCheckEvent.CALL
)
class ParentClass():
    parent_attribute:list

    @property
    def parent_property(self) -> str:
        return self._parent_property

    @parent_property.setter
    def parent_property(self, value:str):
        self._parent_property:str = value

    def __init__(self, parent_attribute:int, parent_property:list):
        self.parent_attribute:int = parent_attribute
        self.parent_property:str = parent_property
    
    def process_parent_property(self):
        self.parent_property

@icontract.invariant(lambda self: all(item < 0 for item in self.child_descriptor), check_on=check_on_default)
class ChildClass(ParentClass):
    child_data:ParentDataClass
    child_descriptor:list[int] = DataDescriptor[list[int]]()

    def __init__(self, parent_attribute:int, parent_property:list, child_data:ParentDataClass, child_descriptor:list):
        super().__init__(parent_attribute=parent_attribute, parent_property=parent_property)
        self.child_data:ParentDataClass = child_data
        self.child_descriptor:list = child_descriptor

obj = ChildClass(
    parent_attribute=1,
    parent_property='foo',
    child_descriptor=[-1],
    child_data=ChildDataClass(
        parent_dataclass_field=30,
        child_dataclass_field='AB'
    )
)

#obj.parent_attribute = 10                           # Expectation met: contract violation, because 'icontract.InvariantCheckEvent.SETATTR is (also) set.
#obj.parent_property = 'foobar'                      # Expectation met: no contract violation, because (only) 'icontract.InvariantCheckEvent.CALL' is set.
#obj.process_parent_property()                       # - Expectation met: contract violation for 'obj.parent_property', because (only) 'icontract.InvariantCheckEvent.CALL' is set.
#obj.child_descriptor = [-1,2]                       # Expectation met: contract violation, because 'InvariantCheckEvent.SETATTR' is (also) set.
#obj.child_data.parent_dataclass_field = 3           # Expectation met: contract violation, because 'InvariantCheckEvent.SETATTR' is (also) set.
#obj.child_data.child_dataclass_field = 'ABC'        # Expectation met: no contract violation, because (only) 'icontract.InvariantCheckEvent.CALL' is set (by default).
#obj.child_data.process_child_dataclass_field()      # - Expectation met: contract violation for 'obj.child_data.child_dataclass_field', because (only) 'icontract.InvariantCheckEvent.CALL' is set (by default).

I did find a corner case where (unintentional) bypassing __setattr__ doesn't evaluate contracts, which may lead to some confusion for users. However, my guess is alleviating that confusion would require intimate knowledge of value types for method hooking:

# -*- coding: utf-8 -*-
import dataclasses
import typing
import icontract

@icontract.invariant(
    lambda self: any(item < 0 for item in self.attribute),
    check_on=icontract.InvariantCheckEvent.CALL
)
class Class():
    def __init__(self, attribute:list):
        self.attribute:list = attribute
    
    def process_attribute(self):
        self.attribute

# Case one: going through 'obj.__setattr__':
#obj = Class(attribute=[-1,])    # Expectation met: no contract violation during initialization.
#obj.attribute = [1]             # Expectation met: contract violation, if 'icontract.InvariantCheckEvent.SETATTR' is set.
#obj.process_attribute()         # Expectation met: Contract violation, if 'icontract.InvariantCheckEvent.CALL' is set.

# Case two: bypassing 'obj.__setattr__' (unintentionally):
obj = Class(attribute=[-1,])    # Expectation met: no contract violation during initialization.
obj.attribute.append(1)         # Expectation fuzzy: no contract violation when 'icontract.InvariantCheckEvent.SETATTR' is set.
obj.process_attribute()         # Expectation not met: no contract violation, even though 'icontract.InvariantCheckEvent.CALL' is set.

Finally, I am also curious why you chose to implement CALL and SETATTR , but not GETATTR and ALL?

[mristin]

Hi @GithubCamouflaged ,
Please apologize the delay -- I'm trying hard to come back to this pull request, but it's been quite turbulent these days for me and my family (we are moving between continents). This should be all sorted out till mid-August, hopefully before.

[GithubCamouflaged]

Hi @GithubCamouflaged , Please apologize the delay -- I'm trying hard to come back to this pull request, but it's been quite turbulent these days for me and my family (we are moving between continents). This should be all sorted out till mid-August, hopefully before.

Appreciate the status update :)

Good luck with the move!

[mristin]

Hi @GithubCamouflaged ,
I'm finally having some time on my fingers again to work on this. I hope you are still there :-).

I think you have a bug in your code:

@icontract.invariant(
    lambda self: any(item < 0 for item in self.attribute),
    check_on=icontract.InvariantCheckEvent.CALL
)
class Class():
    def __init__(self, attribute:list):
        self.attribute:list = attribute
    
    def process_attribute(self):
        self.attribute

...

# Case two: bypassing 'obj.__setattr__' (unintentionally):
obj = Class(attribute=[-1,])    # Expectation met: no contract violation during initialization.
obj.attribute.append(1)         # Expectation fuzzy: no contract violation when 'icontract.InvariantCheckEvent.SETATTR' is set.
obj.process_attribute()         # Expectation not met: no contract violation, even though 'icontract.InvariantCheckEvent.CALL' is set.

The invariant is any(item < 0 for item in self.attribute), and that is satisfied (note the any!) when you call:

obj.attribute.append(1)

I added a unit test for this particular case to make sure I didn't get it wrong (see the unit test test_that_invariant_checked_on_call_even_though_violated_through_leaking in test_invariants.py).

I wrote some more documentation to address the fuzzy expectation. Indeed, a good catch!

You wrote:

Finally, I am also curious why you chose to implement CALL and SETATTR , but not GETATTR and ALL?

GETATTR resulted in endless loops all over the place as I attach __invariants__ under the hub.

I omitted ALL by mistake; fixed it now.

Can you please have another review? I made a separate commit (68d9d30) so that it is easier for you to review. I'll squash & merge them all in the end.

[coveralls]

coverage: 92.929% (+0.1%) from 92.784%
when pulling cd43b14 on mristin/Evaluate-invariants-on-attribute-operations
into 8edda0d on master.

[mristin]

@GithubCamouflaged just a kind reminder in case this pull request got lost in your mailbox :-)