Attempt to improve liskov substitution principle error

[daehan-koreapool]

Background

• Subclasses of CBORSerializable tend to override primitive related methods with tighter and specific parameter and return value types.
• Mypy raises the same error for most of the CBORSerializable subclasses and network.py module is the smallest module demonstrating the problem.
• According to Liskov Principle, subclasses should have looser type requirements rather than having tighter type requirements:
  • https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
  • https://stackoverflow.com/questions/56860/what-is-an-example-of-the-liskov-substitution-principle

Goal

• Start a discussion in an attempt to improve PyCardano code quality
• I would love to hear your opinion on this, @cffls.

$ make qa
poetry run flake8 pycardano
poetry run mypy --install-types --non-interactive pycardano
pycardano/network.py:21: error: Return type "int" of "to_primitive" incompatible with return type "Primitive" in supertype "CBORSerializable"
pycardano/network.py:25: error: Argument 1 of "from_primitive" is incompatible with supertype "CBORSerializable"; supertype defines the argument type as "Primitive"
pycardano/network.py:25: note: This violates the Liskov substitution principle
pycardano/network.py:25: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
Found 2 errors in 1 file (checked 10 source files)
make: *** [Makefile:65: qa] Error 1

[cffls]

Thanks @daehan-koreapool for initiating this convo! Not sure why it is complaining line 21. The return type (int) is more restrictive than the super class (Primitive), even the example in the guide demonstrated that more restrictive return type is allowed.

Regarding line 25, it makes sense to change the type of value to Primitive, and add a check at line 26 to make sure the value is an integer, otherwise raise an exception. This will fail early when the wrong input is passed in, and prevent obscure bugs.

[daehan-koreapool]

I tried making bunch of changes on serialization.Primitive type since this is where most of the type warnings start from, and I noticed few things.

Returning TypeVar without any method parameters raises an error

pycardano/serialization.py:175: error: A function returning TypeVar should receive at least one argument containing the same Typevar
pycardano/serialization.py:175: note: Consider using the upper bound "Union[bytes, bytearray, str, int, float, Decimal, bool, None, Tuple[Any, ...], List[Any], IndefiniteList, Dict[Any, Any], defaultdict[Any, Any], OrderedDict[Any, Any], datetime, Pattern[Any], Any, Any, Set[Any], FrozenSet[Any]]" instead

For a simple method which only returns a generic Primitive type without taking any parameters, mypy cannot infer what value to return.

I also tried defining a generic type with a looser upper bound, but this raises the above error message once agin since we are still using a TypeVar.

Primitive = TypeVar(
    "Primitive",
    bound=Union[
        bytes,
        bytearray,
        str,
        int,
        float,
        Decimal,
        bool,
        None,
        tuple,
        list,
        IndefiniteList,
        dict,
        defaultdict,
        OrderedDict,
        # type(undefined),
        datetime,
        re.Pattern,
        CBORSimpleValue,
        CBORTag,
        set,
        frozenset,
    ],
)

Primitive TypeVar generic type could be a Union type.

With a TypeVar generic type, only a single primitive type can exist in a function scope because how TypeVar works in Python.
Realistically, we should allow multiple CBOR primitive types to exist within a function scope and a Union type represents this better than more strict TypeVar generic type.

Also, when we define more simple Primitive = Union[...], mypy now understands a method can return either of the primitive types.
So, the following error message is satisfied if we use an Union type:

pycardano/network.py:21: error: Return type "int" of "to_primitive" incompatible with return type "Primitive" in supertype "CBORSerializable"

Regarding line 25, it makes sense to change the type of value to Primitive, and add a check at line 26 to make sure the value is an integer, otherwise raise an exception. This will fail early when the wrong input is passed in, and prevent obscure bugs.

Lastly, I do agree and mypy seems to be happy with this approach.

I'll try making few changes and push code changes for you to review through @cffls

[cffls]

Thanks @daehan-koreapool , changing Primitive to a union type makes sense to me.

[codecov-commenter]

Codecov Report

Merging #128 (652ca84) into main (7787695) will increase coverage by 0.02%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
+ Coverage   85.92%   85.95%   +0.02%     
==========================================
  Files          25       25              
  Lines        2693     2698       +5     
  Branches      518      519       +1     
==========================================
+ Hits         2314     2319       +5     
+ Misses        286      285       -1     
- Partials       93       94       +1     

Impacted Files	Coverage Δ
pycardano/backend/blockfrost.py	33.61% <ø> (ø)
pycardano/serialization.py	86.89% <83.33%> (-0.77%)	⬇️
pycardano/network.py	100.00% <100.00%> (+15.38%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[cffls]

Great work as always. Thank you @daehan-koreapool !