Skip to content

type hints: (trivial) add missing "Optional" qualifiers - #10903

Open
SomberNight wants to merge 4 commits into
spesmilo:masterfrom
SomberNight:202608_fix_type_hints_optional
Open

type hints: (trivial) add missing "Optional" qualifiers#10903
SomberNight wants to merge 4 commits into
spesmilo:masterfrom
SomberNight:202608_fix_type_hints_optional

Conversation

@SomberNight

Copy link
Copy Markdown
Member

In many cases with our type hinting, we do not explicitly specify that variables are allowed to be None when they have a default value of None.
This PR now adds the missing explicit "Optional" markers.


Consider:

class Foo(NamedTuple):
    field: str = None

This type-hinting is technically incorrect, we should type hint it as either one of these options:

  • field: Optional[str] = None
  • field: str | None = None

Thomas said he prefers the second, so str | None.
However I am not going to touch lines with existing Optionals, I am only fixing lines that are type-hinted incorrectly.

PyCharm has had soft-warnings about these lines for quite a while,
however when doing type-inference, it used to internally "fix" the type by allowing None.
Recently however it seems it is not doing the internal inference-fixing anymore, but assumes that path has type str (as we specified) and then make "incorrect" inferences:

class Foo(NamedTuple):
    field: str = None

def func(foo: Foo):
    assert foo.field is None
    print("function body")  # <<< PyCharm now says this code is unreachable

I have whole function bodies grayed out and marked as dead code due to this.

Example:

assert htlc_set.parent_set_key is None, f"Must not settle child {htlc_set=}"
, where htlc_set.parent_set_key is type-hinted as str, so the assert "must" always fail

In many cases with our type hinting, we do not explicitly specify that variables are allowed to be None when they have a default value of None.
This PR now adds the missing explicit "Optional" markers.

-----

Consider:
```
class Foo(NamedTuple):
    field: str = None
```
This type-hinting is technically incorrect, we should type hint it as either one of these options:
- `field: Optional[str] = None`
- `field: str | None = None`

PyCharm has had soft-warnings about these lines for quite a while,
however when doing type-inference, it used to internally "fix" the type by allowing None.
Recently however it seems it is not doing the internal inference-fixing anymore, but assume that `path` has type `str` (as we specified) and then make "incorrect" inferences:

```
class Foo(NamedTuple):
    field: str = None

def func(foo: Foo):
    assert foo.field is None
    print("function body")  # <<< PyCharm now says this code is unreachable
```
I have whole function bodies grayed out and marked as dead code due to this.

Example: https://github.com/spesmilo/electrum/blob/c82a0d12ef1e03a96d6180839f215c05efe9efe3/electrum/lnpeer.py#L2320, where htlc_set.parent_set_key is type-hinted as str, so the assert "must" always fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant