mypy error on checking isinstance for status type since 2.17.0 release

[samuelallan72]

In the release of 2.17.0, this code no longer passes mypy type check with warn_unreachable turned on:

        self.unit.status = MaintenanceStatus("Installing charm software")
        if self._check_if_container():
            return

        self._check_mandatory_config() # this function may set self.unit.status to an instance of BlockedStatus
        if isinstance(self.unit.status, BlockedStatus):
            return

The error from mypy (v1.11.2) is (line 166 is the isinstance call above):

src/charm.py:166: error: Subclass of "MaintenanceStatus" and "BlockedStatus" cannot exist: would have incompatible method signatures  [unreachable]
src/charm.py:167: error: Statement is unreachable  [unreachable]

Downgrading ops to 2.16.1 fixes the issue.

Note that turning off warn_unreachable is not advised in mypy, as mypy will skip typechecking code it thinks is unreachable, whether warn_unreachable is turned on or not. So with warn_unreachable off, it can result in missing real unreachable code, or getting runtime failures due to false positives on unreachable being silenced in mypy.

See here for failing CI example

[benhoyt]

Interesting -- thanks for the report. @james-garner-canonical Can you please look into this sometime this week?

[james-garner-canonical]

This must be because pre 2.17.0 all the status classes were erroneously typed in static analysis as StatusBase due to the StatusBase.register class decorator obscuring their type (issue: #1380). So we used to have:

reveal_type(self.unit.status)  # StatusBase
self.unit.status = MaintenanceStatus("Installing charm software")
reveal_type(self.unit.status)  # StatusBase

Now we have:

reveal_type(self.unit.status)  # StatusBase
self.unit.status = MaintenanceStatus("Installing charm software")
reveal_type(self.unit.status)  # MaintenanceStatus

This seems to be problematic when the value of self.unit.status gets changed in a different scope. Here's a minimal example:

def fn(u: Unit):
    reveal_type(u.status)  # StatusBase
    u.status = MaintenanceStatus("")
    reveal_type(u.status)  # MaintenanceStatus
    block(u)
    reveal_type(u.status)  # MaintenanceStatus
    if isinstance(u.status, BlockedStatus):  
        print("incorrectly seen as unreachable code")

def block(u: Unit):
    u.status = BlockedStatus("")

Thank you for reporting this, @samuelallan72. As a user, you could resolve this warning using a typing.cast in your code:

if isinstance(typing.cast(StatusBase, u.status), BlockedStatus):
    ...

It's definitely not ideal to have to write that kind of thing, but other than intentionally obscuring the type of StatusBase subclasses again for backwards compatibility, I'm not sure how we can best fix this. What do you suggest, @benhoyt?

We type check ops with pyright rather than mypy. Pyright doesn't issue warnings on unreachable code, but it does expose this information so that editors can display unreachable code differently. I'm not sure why, but Pyright doesn't see this as unreachable code. Should we consider adding tests with mypy to our static analysis?

[benhoyt]

@james-garner-canonical to test this in super-tox on 100+ charms to see how common this is. James will also suggest an alternative approach.

[dimaqq]

Explanation of type narrowing: https://github.com/microsoft/pyright/blob/main/docs/type-concepts-advanced.md

[james-garner-canonical]

To be explicit about the type narrowing happening here, the issue is that self.unit.status = MaintenanceStatus(...) narrows its type from StatusBase to MaintenanceStatus. (This did not happen pre-2.17.0 due to an internal error in ops where StatusBase subclasses had their types obscured by a decorator.) The (potential) assignment of unit.status to BlockedStatus in a different method's scope doesn't affect the type inference in the scope where the first assignment took place, so the type of unit.status isn't (un)narrowed after calling self._check_mandatory_config(). This is essentially a limitation/feature of current python type checkers that I don't think we can do anything about at the moment.

Sorry that 2.17.0 broke your tests, @samuelallan72. Here are a couple more alternatives to having to use typing.cast, which unfortunately would require a slight refactor of _check_if_container() and the calling sites, but if you're going to be adding typing.cast anyway ...

# return the bad status
self.unit.status = MaintenanceStatus("Installing charm software")
...
bad_status = self._check_mandatory_config() # return BlockedStatus if problematic, otherwise None
if bad_status is not None:
    self.unit.status = bad_status
    return

# exception on bad status
self.unit.status = MaintenanceStatus("Installing charm software")
...
try:
    self._check_mandatory_config() # raise with status msg if problematic
except ValueError as e:
    self.unit.status = BlockedStatus(str(e))
    return

I don't see any unreachable errors when running tests on 100+ charms, @benhoyt, so I guess none of the charms we test against are running mypy with warn_unreachable, or at least aren't doing so via tox (or are currently failing early due to open issues in super-tox).

Btw thanks to @dimaqq's link, I learned that that pyright has an enableReachabilityAnalysis flag that enables marking code as unreachable due to type analysis. Without this flag, it only uses non-type information for marking code as unreachable (e.g. if False: ...). Either way, pyright's analysis here appears to be more forgiving, as it does not mark the code in thecharm-storage-connector case or my minimal example as unreachable with this flag enabled. We should consider what we want to do for testing here.

[dimaqq]

Potential cop-out: asymmetric descriptors.

Type Narrowing for Asymmetric Descriptors When pyright evaluates a write to a class variable that contains a descriptor object (including properties), it normally applies assignment-based type narrowing. However, when the descriptor is asymmetric — that is, its “getter” type is different from its “setter” type, pyright refrains from applying assignment-based type narrowing. For a full discussion of this, refer to this issue. Mypy has not yet implemented the agreed-upon behavior, so its type narrowing behavior may differ from pyright’s in this case.

In other words, if we could hypothetically change the type signature this way:

    @property
    def status(self) -> StatusBase: ...

    @status.setter
    def status(self, value: BlockedStatus|MaintenanceStatus): ...

[samuelallan72]

Thanks for all your comments here! Sounds like it's not an issue with ops; just a limitation of the type checker. On the charm where we experienced this, I proposed a simple fix involving type casting: https://github.com/canonical/charm-storage-connector/pull/33

[james-garner-canonical]

Thanks for the update, @samuelallan72, I'm glad that a typing.cast may be an acceptable solution. Happy to see in your comment on the PR that you guys may consider using the collect-status event in future.

@dimaqq, that's interesting! You know, this could be more than a cop-out, since setting UnknownStatus and ErrorStatus will lead to an Exception, so maybe we'd want to type the setter and getter differently here anyway. Removing unwanted(?) narrowing would just be a side effect. Though it sounds like it wouldn't affect mypy users anyway, at least for now.

[dimaqq]

@james-garner-canonical do test this with super-tox 🎉

I wouldn't make such a change blind, because existing charms may presently rely on the setter's declared RHS type, correctness being validated static type checker; and restricting the settable types would break these users:

    def get_workload_status(self) -> StatusBase:
        ...

    def on_something(self, event):
        self.unit.status = self.get_workload_status()

Also, semantically, our .status property is in fact round-trip safe. Unit.status is essentially side-effect free and Application.status ha a caveat of raising if current unit is not the leader.

My 2c: I think the ops types are basically correct. I'd love to see the charm code refactored, because current code is equivalent to using a global variable to pass around the "accumulated status so far", and globals are like bad, right?

Type casting is a plug that's equivalent to # type: ignore, and I see it as a band-aid. After all, we use tools like mypy to both ensure correctness and guide us to write better [structured] code, don't we?

[benhoyt]

Yeah, that sounds right, Dima -- changing the setter type would likely be a backwards-incompatible change. We may be able to improve this, but I think for now we've determined this is a mypy/charm issue, and we can close this as not planned?

[james-garner-canonical]

Yes, we can close this issue. Just noting the possibility of an asymmetric descriptor to help with warning about setting status to a bad value. But I think we would actually want narrowing to continue to work as it does on assignment, so it may not be a good fit.