Relax Representation Invariant Checking Logic for Recursive Classes

[ckatsube]

Motivation & Proposal

The Representation Invariant checker timing is currently too strict. In many data structures, it's common for instances to mutate other instances of the same type (i.e. graphs/trees or recursive data structures). The current logic enforces RI checks in two places: 1) when an attribute is set EXCEPT for instance methods 2) after the instance method is called. The problem is that Path 1 is executed inside instance methods that set the attributes of different instances of the same type (motivating example below).

The proposed change is to additionally exempt instances of the same type as self during the Path 1 check and push checking these RIs until after the instance method has returned, alongside the Path 2 check.

More practical motivating example with RecursiveList

Note: this specific example can technically be avoided by having a better constructor for RecursiveList but the bug still stands regardless.

from __future__ import annotations
from typing import Any
from python_ta.contracts import check_contracts


@check_contracts
class RecursiveList:
    """
    Representation Invariants:
    - (self._value is None and self._rest is None) or \
        (self._value is not None and self._rest is not None)
    """
    _value: Any | None
    _rest: RecursiveList | None

    def __init__(self, items: list) -> None:
        if items == []:
            self._value = None
            self._rest = None
        else:
            self._value = items[0]
            self._rest = RecursiveList(items[1:])

    def insert(self, value: Any):
        """Insert value at the current position (push current value to next position)"""
        new_rest = RecursiveList([])
        # new_rest._value and ._rest are originally None by this construction logic.
        new_rest._value = self._value  # RI violation occurs here despite RI being satisfied in the next line.
        new_rest._rest = self._rest
        self._value = value
        self._rest = new_rest


if __name__ == '__main__':
    node_a = RecursiveList([10, 20, 30])
    node_a.insert(5)

Changes

• Modified the Path 1 check to also exempt RI checking if the type of the parent frame's self matches the type of self

• Accumulated all of the instances that were exempt in the Path 1 check to later check at Path 2

• Modified the Path 2 logic to also check RIs of the accumulated instance attributes

• Added unit tests to test_class_contracts for both the attribute mutation and function arg mutation cases.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)	X
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📚 Documentation update (change that only updates documentation)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

• I have performed a self-review of my changes.
  • Check that all changed files included in this pull request are intentional changes.
  • Check that all changes are relevant to the purpose of this pull request, as described above.
• I have added tests for my changes, if applicable.
  • This is required for all bug fixes and new features.
• I have updated the project documentation, if applicable.
  • This is required for new features.
• I have updated the project Changelog (this is required for all changes).
• If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

• I have verified that the pre-commit.ci checks have passed.
• I have verified that the CI tests have passed.
• I have reviewed the test coverage changes reported by Coveralls.
• I have requested a review from a project maintainer.

Questions and Comments

I made a minor logical nuance where subclasses should be able to temporarily violate parent class instances' Representation Invariants in its instance methods (Child.modify(parent)). If you think that Child instances should not be allowed to temporarily violate Parent RIs and this check should be strictly equal class types (only Child==Child and Parent == Parent but no Child modifies Parent), let me know! It would effectively just be changing not isinstance(caller_self, type(self)) to not type(caller_self) == type(self)

[ckatsube]

Converted to draft because the failing test test_call_super_creates_temp_invalid where the subclass Teacher's RI is throwing an error after calling a subclass method. Need to figure out why subclass RIs are being evaluated after a parent method is ran (when it shouldn't be).

[coveralls]

Pull Request Test Coverage Report for Build 13983965613

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 23 of 23 (100.0%) changed or added relevant lines in 1 file are covered.
• 27 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+0.1%) to 93.089%

---

Files with Coverage Reduction	New Missed Lines	%
python_ta/init.py	9	94.65%
python_ta/contracts/init.py	18	92.39%

Totals
Change from base Build 13749526863:	0.1%
Covered Lines:	3300
Relevant Lines:	3545

---

💛 - Coveralls

[ckatsube]

Not a current collaborator so can't directly request a reviewer.

@david-yz-liu
I'd like to request a review.

PR size: small

[david-yz-liu]

Thank you @ckatsube! It's great to see you active on here. :) I will review soon.

BTW, if you want to join the new SDS slack, you're welcome to join by following the instructions under "Slack" at https://www.cs.toronto.edu/~david/sds/welcome-to-sds.html.

[david-yz-liu]

Thank you, @ckatsube! I left some comments. Also it would be good to add a test case to illustrate the interaction with inheritance.

[david-yz-liu]

Use hasattr and getattr instead of accessing __dict__

[ckatsube]

Sounds good!

[david-yz-liu]

The variable should probably be called mutated_instances

[ckatsube]

Thanks for the consistency check 👍

[david-yz-liu]

I think this else is a bit too broad: if caller_self is self it doesn't need to be added to __mutated_instances__, as the RIs should be checked on self at the end of the calling method.

Also, it's worth adding a comment to explain what's going on (parallel to the comment in the if branch).

[ckatsube]

Sounds good! Thanks for the keen eye

[david-yz-liu]

The check for isinstance(caller_self, type(self)) is good (it's fine for an instance of a child class to violate an RI on another instance of the a parent class).

In the comment, the wording should be reversed (caller_self is an instance of self's type); the part including and after the -- is not necessary and can be removed.

[ckatsube]

Thanks for following up on the question!

[david-yz-liu]

The restoring is generally good; however, we should have different cases for whether __mutated_instances__ was an attribute or not originally. If it wasn't, we should use delattr to remove the instance attribute altogether, so it remains a temporary attribute.

[ckatsube]

Sounds like a plan--added a branch for deleting the temporary attribute

[david-yz-liu]

"Excepts" -> "Expects"

"thrown" -> "raised"

Also, please add a period to the end of the sentence on the previous line.

[ckatsube]

Done!

[ckatsube]

Resolved the review comments and added the requested tests.

Additionally modified the logic around invariant checking the mutated instances to use the klass/klass_mod of the mutated instance to prevent child class RIs from being enforced on mutated parent class instances (this was caught by the new tests).

[ckatsube]

Since the checks passed, this is ready for re-review!

[david-yz-liu]

Thanks, @ckatsube!