[libgpiod,07/22] bindings: python: fix Chip union-attr type errors

Since `Chip._chip` can be `None`, it's necessary to inform type checkers
of the state of the object to silence the union-attr errors.

Type checkers may not be able to infer that an object is not `None` from
an earlier call (such as `_check_closed`).

Instead of littering the code with "# type: ignore" comments, use casts
to inform type checkers that objects are not `None`.

Using `assert` is another option, however this duplicates the logic in
`_check_closed` so is redundant at best and, at worst, is not a safe
replacement as `assert` can be elided in optimized Python environments
and these checks need to be runtime enforced.

Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
---
 bindings/python/gpiod/chip.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

On Fri, Sep 27, 2024 at 8:57 PM Vincent Fazio <vfazio@xes-inc.com> wrote:
>
> Since `Chip._chip` can be `None`, it's necessary to inform type checkers
> of the state of the object to silence the union-attr errors.
>
> Type checkers may not be able to infer that an object is not `None` from
> an earlier call (such as `_check_closed`).
>
> Instead of littering the code with "# type: ignore" comments, use casts
> to inform type checkers that objects are not `None`.
>
> Using `assert` is another option, however this duplicates the logic in
> `_check_closed` so is redundant at best and, at worst, is not a safe
> replacement as `assert` can be elided in optimized Python environments
> and these checks need to be runtime enforced.
>
> Signed-off-by: Vincent Fazio <vfazio@xes-inc.com>
> ---
>  bindings/python/gpiod/chip.py | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/bindings/python/gpiod/chip.py b/bindings/python/gpiod/chip.py
> index 4aa5677f94caf8c5d863aa6d75915a5b650de137..fe7bcfe082d6e9f6220093d3fc45ff232b5d0d17 100644
> --- a/bindings/python/gpiod/chip.py
> +++ b/bindings/python/gpiod/chip.py
> @@ -7,7 +7,7 @@ from collections import Counter
>  from datetime import timedelta
>  from errno import ENOENT
>  from types import TracebackType
> -from typing import Optional, Union
> +from typing import Optional, Union, cast
>
>  from . import _ext
>  from ._internal import poll_fd
> @@ -97,6 +97,7 @@ class Chip:
>          longer be used after this method is called.
>          """
>          self._check_closed()
> +        self._chip = cast(_ext.Chip, self._chip)
>          self._chip.close()
>          self._chip = None
>

Ok, so I don't really understand what is happening here. We're going
to re-assign _chip in every function? What happens to cast() if _chip
IS None?

Bart

On Tue, Oct 8, 2024 at 8:16 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
>
> Ok, so I don't really understand what is happening here. We're going
> to re-assign _chip in every function? What happens to cast() if _chip
> IS None?

In this scenario, self._chip cannot be None. The self._check_closed() guard
ensures this, however, type checkers (at least mypy) cannot infer that from the
current code pattern.

`cast` is informing the type checker that past this point, self._chip should
not be considered as None and it's safe to reference attributes off the object

This seemed like the cleanest alternative, though others are:

* use a local variable for the cast result. This may be less confusing but
  incurs more changed lines

    self._check_closed()
    chip = cast(_ext.Chip, self._chip)
    return chip.path

* use asserts. These aren't always enforced during runtime so we cannot replace
  _check_closed but it does inform the type checker that it can narrow the type.
  Using both is ok, just slightly redundant.

    self._check_closed()
    assert self._chip is not None
    return self._chip.path

* add a `if self._chip is None` check that duplicates _check_closed
(or replace it completely)

* other "creative" solutions like a wrapper that returns a Chip or
throws an exception if it's None.

    def _chip_or_exc(self) -> Chip:
        if self._chip is None:
            raise Exception
        return self._chip

    chip = self._chip_or_exc()
    return chip.path

>
> Bart

On Tue, Oct 8, 2024 at 4:57 PM Vincent Fazio <vfazio@gmail.com> wrote:
>
> On Tue, Oct 8, 2024 at 8:16 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> >
> > Ok, so I don't really understand what is happening here. We're going
> > to re-assign _chip in every function? What happens to cast() if _chip
> > IS None?
>
> In this scenario, self._chip cannot be None. The self._check_closed() guard
> ensures this, however, type checkers (at least mypy) cannot infer that from the
> current code pattern.
>
> `cast` is informing the type checker that past this point, self._chip should
> not be considered as None and it's safe to reference attributes off the object
>
> This seemed like the cleanest alternative, though others are:
>
> * use a local variable for the cast result. This may be less confusing but
>   incurs more changed lines
>
>     self._check_closed()
>     chip = cast(_ext.Chip, self._chip)
>     return chip.path
>

For the sake of readability, I would lean more towards this option if
I'm honest.

Or even - if you need to use the cast variable only once:

self._check_closed()
return cast(_ext.Chip, self._chip).path

?

> * use asserts. These aren't always enforced during runtime so we cannot replace
>   _check_closed but it does inform the type checker that it can narrow the type.
>   Using both is ok, just slightly redundant.
>
>     self._check_closed()
>     assert self._chip is not None
>     return self._chip.path
>

Yeah, this isn't optimal.

> * add a `if self._chip is None` check that duplicates _check_closed
> (or replace it completely)
>

Yep, no.

> * other "creative" solutions like a wrapper that returns a Chip or
> throws an exception if it's None.
>
>     def _chip_or_exc(self) -> Chip:
>         if self._chip is None:
>             raise Exception
>         return self._chip
>
>     chip = self._chip_or_exc()
>     return chip.path
>

Ugh. I see, cast() is the best solution.

Please consider the small change I suggested.

Bart