Pyright in core #1939

grdddj · 2021-11-24T09:37:32Z

Creating a PR for the pyright-transition branch on top of mypy-core-testing to be able to track and discuss the changes and to have CI checking all the device tests etc.

core/src/apps/ethereum/tokens.py

core/src/apps/ethereum/tokens.py.mako

grdddj · 2021-11-24T16:06:11Z

After last commit, pyright is showing around 100 issues (however a lot of them are duplicates). Below they are grouped and commented (some errors could be missing here):

/home/jmusil/trezor-firmware/core/src/usb.py:106:13 - error: "iface_debug" is possibly unbound (reportUnboundVariable)
/home/jmusil/trezor-firmware/core/src/usb.py:108:13 - error: "iface_webauthn" is possibly unbound (reportUnboundVariable)
/home/jmusil/trezor-firmware/core/src/usb.py:110:13 - error: "iface_vcp" is possibly unbound (reportUnboundVariable)
- could just merge the corresponding IFs together (even though the high-level visibility could suffer a little bit)

 /home/jmusil/trezor-firmware/core/src/apps/bitcoin/get_address.py:92:40 - error: "multisig_xpub_magic" is possibly unbound (reportUnboundVariable)
- could be a problem when NOT if coin.segwit and not msg.ignore_xpub_magic:

/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:573:60 - error: "txi_sign" is possibly unbound (reportUnboundVariable)
/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/bitcoin.py:580:27 - error: "txi_sign" is possibly unbound (reportUnboundVariable)
- could be an issue when index will be higher than the length of tx_info.tx.inputs_count

/home/jmusil/trezor-firmware/core/src/apps/webauthn/credential.py:224:31 - error: Argument of type "str" cannot be assigned to parameter "data" of type "bytes | None" in function "__init__"
- str.encode() if it is really a string

/home/jmusil/trezor-firmware/core/src/apps/webauthn/fido2.py:606:39 - error: Function with declared type of "bool | State" must return value
- could raise UnimplementedError

/home/jmusil/trezor-firmware/core/src/trezor/ui/layouts/tt/__init__.py:135:31 - error: Argument of type "str | bytes | None" cannot be assigned to parameter "confirm" of type "str" in function "__init__"
- strange that it coould even be bytes

/home/jmusil/trezor-firmware/core/src/trezor/ui/layouts/tt/__init__.py:135:44 - error: Argument of type "str | bytes | None" cannot be assigned to parameter "cancel" of type "bool" in function "__init__"
- could do bool(xxx)

/home/jmusil/trezor-firmware/core/src/trezor/ui/__init__.py:46:5 - error: Declaration "refresh" is obscured by a declaration of the same name (reportGeneralTypeIssues)
/home/jmusil/trezor-firmware/core/src/trezor/sdcard.py:68:5 - error: Declaration "is_present" is obscured by a declaration of the same name (reportGeneralTypeIssues)
/home/jmusil/trezor-firmware/core/src/trezor/sdcard.py:69:5 - error: Declaration "capacity" is obscured by a declaration of the same name (reportGeneralTypeIssues)
- I see no problem there

/home/jmusil/trezor-firmware/core/src/trezor/utils.py:143:19 - error: Expression of type "str" cannot be assigned to yield type "Chunkable@chunks_intersperse"
- str is defined in Chunkable types

/home/jmusil/trezor-firmware/core/src/trezor/sdcard.py:68:18 - error: "sdcard" is possibly unbound (reportUnboundVariable)
/home/jmusil/trezor-firmware/core/src/trezor/sdcard.py:69:16 - error: "sdcard" is possibly unbound (reportUnboundVariable)
- not a problem

/home/jmusil/trezor-firmware/core/src/apps/common/request_pin.py:110:21 - error: "request_pin_on_device" is possibly unbound (reportUnboundVariable)
- is alright, it must be defined

/home/jmusil/trezor-firmware/core/src/storage/sd_salt.py:84:15 - error: "fatfs" is not a known member of module (reportGeneralTypeIssues)
/home/jmusil/trezor-firmware/core/src/apps/debug/__init__.py:194:16 - error: "sdcard" is not a known member of module (reportGeneralTypeIssues)
/home/jmusil/trezor-firmware/core/src/apps/base.py:43:27 - error: "device" is not a known member of module (reportGeneralTypeIssues)
- strange stuff

/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/tx_info.py:24:16 - error: Expression of type "ellipsis" cannot be assigned to declared type "CoinInfo"
- we may just assign something to it

/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/helpers.py:255:6 - error: Return type of generator function must be "Generator" or "Iterable" (reportGeneralTypeIssues)
- we may need to change the return type

-------------------------------
MORE COMPLEX ISSUES
-------------------------------

/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/helpers.py:184:14 - error: Expression of type "(l: object, r: object) -> bool" cannot be assigned to declared type "(self: Self@object, __o: object) -> bool"
    Type "(l: object, r: object) -> bool" cannot be assigned to type "(self: Self@object, __o: object) -> bool"
      Parameter name mismatch: "self" versus "l" (reportGeneralTypeIssues)
	  
/home/jmusil/trezor-firmware/core/src/trezor/ui/components/tt/word_select.py:55:20 - error: Expression of type "Tuple[Task | Awaitable[Any], ...]" cannot be assigned to return type "tuple[Task, ...]"
    Tuple entry 1 is incorrect type
      Type "Task | Awaitable[Any]" cannot be assigned to type "Task"
        "Awaitable[Any]" is incompatible with "Task" (reportGeneralTypeIssues)
		
/home/jmusil/trezor-firmware/core/src/trezor/ui/components/tt/scroll.py:149:20 - error: Expression of type "tuple[Task, Task, Coroutine[Any, Any, None], Awaitable[Any]]" cannot be assigned to return type "tuple[Task, ...]"
    Tuple entry 4 is incorrect type
      "Awaitable[Any]" is incompatible with "Task" (reportGeneralTypeIssues)
	  
/home/jmusil/trezor-firmware/core/src/apps/bitcoin/sign_tx/__init__.py:70:28 - error: Expression of type "Type[Zcashlike]" cannot be assigned to declared type "Type[SignerClass]"
    "Zcashlike" is incompatible with protocol "SignerClass"
    Type "Type[Zcashlike]" cannot be assigned to type "Type[SignerClass]"
      "__init__" is an incompatible type
        Type "(tx: SignTx, keychain: Keychain, coin: CoinInfo, approver: Approver) -> None" cannot be assigned to type "(tx: SignTx, keychain: Keychain, coin: CoinInfo, approver: Approver | None) -> None"
          Parameter 4: type "Approver | None" cannot be assigned to type "Approver"
            Type "Approver | None" cannot be assigned to type "Approver" (reportGeneralTypeIssues)

/home/jmusil/trezor-firmware/core/src/apps/bitcoin/keychain.py:233:12 - error: Expression of type "(ctx: Context, msg: MsgIn@wrapper, auth_msg: MessageType | None = None) -> Coroutine[Any, Any, MsgOut@with_keychain]" cannot be assigned to return type "Handler[Unknown, MsgOut@with_keychain]"
    Type "(ctx: Context, msg: MsgIn@wrapper, auth_msg: MessageType | None = None) -> Coroutine[Any, Any, MsgOut@with_keychain]" cannot be assigned to type "Handler[Unknown, MsgOut@with_keychain]"
      Parameter 2: type "MsgIn@Handler" cannot be assigned to type "MsgIn@wrapper" (reportGeneralTypeIssues

grdddj · 2021-11-25T09:23:10Z

Force-pushed rebase on master, done in combination with rebasing the "base" grdddj/mypy_whole_src branch - however, after this it is not correctly showing the difference between these two branches, which is unfortunate.

I still have both original "pre-rebased" branches locally, can I do it somehow better so we will maintain this difference in this PR? (I did the rebases separately for both branches, maybe first rebasing the mypy_whole_src and then this one on top of it?)

matejcik · 2022-01-05T11:56:22Z

zero issues as of now with pyright 1.1.202 (the newest 1.1.203 brings a bug)
let's see what CI has to say

grdddj

Some smaller comments, otherwise good to go.

General themes:

from typing import ... on top-level when it is being used in the code (not only in if TYPE_CHECKING: branch)
- quite a lot of places in core/src/trezor
using xxx | None instead of Optional[xxx]
- at least two occurrences
ctx: Context vs ctx: wire.Context - whether to do from trezor.wire import Context or from trezor import wire
- but not really important
specifying the error being ignored in # type: ignore [error substring]
- in the whole core folder
will we use the pyright-tool?
- connected with the previous one, and to identify unused ignores

core/pyrightconfig.json

core/src/apps/binance/layout.py

core/src/apps/bitcoin/sign_tx/helpers.py

core/src/apps/cardano/helpers/bech32.py

core/src/apps/eos/actions/__init__.py

core/src/trezor/wire/__init__.py

core/src/trezor/wire/codec_v1.py

core/src/trezor/workflow.py

core/tests/test_trezor.protobuf.py

grdddj · 2022-01-07T11:24:11Z

In this branch, core contains 94 # type: ignore comments, but when I disabled them by "enableTypeIgnoreComments": false in pyrightconfig.json, only 51 errors were encountered.

That means we could get rid of a lot of these # type: ignores. We could also take the opportunity to put the error message in the ignore code in errors which persist. core/src/apps/bitcoin/sign_tx/helpers.py also contains around 20 of the exact same issues, so it is a good candidate for a pyright-tool's FILE_SPECIFIC_IGNORES, not to duplicate the ignore so many times

Probably as a first step, we should try to resolve at least some of those 51 remaining issues

docs/core/misc/codestyle.md

matejcik · 2022-01-07T14:40:21Z

TODO: at rebase time, add "[no changelog]" to 5ca6b81

grdddj · 2022-01-07T14:46:13Z

Seems good to me! Looking forward to rebasing the pyright-tool PR on these things in master

Changes many fields to required -- as far as we were able to figure out, signing would fail if these fields aren't provided anyway, so this should not pose a compatibility problem. Co-authored-by: matejcik <ja@matejcik.cz>

This way AST parsers won't get stack exceeded (because an elif counts as an else { if {}} to an AST parser apparently?) and so automated tools will like tokens.py and friends.

[no changelog]

prusnak · 2022-01-07T21:12:07Z

Love this PR! Python 3.10 type annotations are great! 👍

jimpo · 2022-01-09T05:59:34Z

This seems to have broken the docker image because the image does not COPY in the new pyright directory

follow-up to #1939

prusnak · 2022-01-09T09:10:56Z

@jimpo thanks for the notice! fix in #2054

follow-up to #1939

grdddj requested a review from matejcik November 24, 2021 09:37

prusnak requested changes Nov 24, 2021

View reviewed changes

core/src/apps/ethereum/tokens.py Outdated Show resolved Hide resolved

core/src/apps/ethereum/tokens.py.mako Outdated Show resolved Hide resolved

grdddj force-pushed the grdddj/mypy_whole_src branch from f44c03a to 2d984f6 Compare November 25, 2021 09:13

grdddj force-pushed the grdddj/pyright_in_core branch from f370f52 to 882bdf9 Compare November 25, 2021 09:13

matejcik mentioned this pull request Dec 1, 2021

Mypy tests for the whole src folder #1876

Closed

matejcik changed the base branch from grdddj/mypy_whole_src to master December 1, 2021 12:08

matejcik force-pushed the grdddj/pyright_in_core branch from 882bdf9 to e68e814 Compare January 5, 2022 11:55

matejcik force-pushed the grdddj/pyright_in_core branch from e68e814 to fa23ac1 Compare January 6, 2022 12:56

ci(nix): update to pyright-1.1.204

f54a65f

matejcik force-pushed the grdddj/pyright_in_core branch from fa23ac1 to a1b020b Compare January 6, 2022 15:11

matejcik marked this pull request as ready for review January 6, 2022 15:11

matejcik requested review from andrewkozlik and onvej-sl as code owners January 6, 2022 15:11

matejcik removed request for matejcik, onvej-sl and andrewkozlik January 6, 2022 15:11

matejcik mentioned this pull request Jan 6, 2022

Update to pyright-1.1.202 #2051

Closed

grdddj commented Jan 6, 2022

View reviewed changes

matejcik force-pushed the grdddj/pyright_in_core branch from a1b020b to 2dad748 Compare January 7, 2022 14:16

grdddj commented Jan 7, 2022

View reviewed changes

docs/core/misc/codestyle.md Outdated Show resolved Hide resolved

matejcik added 3 commits January 7, 2022 16:50

build(core): exclude all "from typing" imports from build

b2f988a

build(core): rewrite build_mocks with pathlib, add symlinks to ../mocks

9e553eb

chore(core): add fake typing module (not included in build)

fe5b382

matejcik force-pushed the grdddj/pyright_in_core branch from 9068b49 to 05373e0 Compare January 7, 2022 15:51

grdddj and others added 14 commits January 7, 2022 16:54

style(core): full pyright-based type-checking

f67f7e6

Changes many fields to required -- as far as we were able to figure out, signing would fail if these fields aren't provided anyway, so this should not pose a compatibility problem. Co-authored-by: matejcik <ja@matejcik.cz>

refactor(core): improve type signature of bech32.convertbits

263963e

refactor(core): convert if/elif chains to if-chains

af6760a

This way AST parsers won't get stack exceeded (because an elif counts as an else { if {}} to an AST parser apparently?) and so automated tools will like tokens.py and friends.

style(core): support AnyStr in all hash function arguments

70ffcab

build: fully replace mypy with pyright

c287f01

[no changelog]

style(core): improve types in rlp.py

3d58797

style(python): fix typecheck issues

9ef53a0

[no changelog]

fix(legacy/nem): remove checks for newly required protobuf attributes

8b4f685

[no changelog]

docs(core): add changelog for type-checking

bf99237

style(core): define obj_eq and obj_repr so that they pass type-check

7a85ced

style(core): remove unused type:ignore comments

23d9cad

style(core): remove old-style annotations (Set, Tuple, Union)

4ae685e

docs(core): document coding style

585bde6

docs(core): add entries to index

d1e6154

matejcik force-pushed the grdddj/pyright_in_core branch from 05373e0 to d1e6154 Compare January 7, 2022 15:55

matejcik merged commit 4ab33f0 into master Jan 7, 2022

matejcik deleted the grdddj/pyright_in_core branch January 7, 2022 20:41

prusnak added a commit that referenced this pull request Jan 9, 2022

fix(ci): copy pyright directory in Dockerfile

33eede3

follow-up to #1939

prusnak mentioned this pull request Jan 9, 2022

fix(ci): copy pyright directory in Dockerfile #2054

Merged

matejcik mentioned this pull request Jan 10, 2022

Expand type checking to the whole src folder #642

Open

prusnak added a commit that referenced this pull request Jan 12, 2022

fix(ci): copy pyright directory in Dockerfile

52a09c3

follow-up to #1939

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pyright in core #1939

Pyright in core #1939

grdddj commented Nov 24, 2021

grdddj commented Nov 24, 2021

grdddj commented Nov 25, 2021

matejcik commented Jan 5, 2022

grdddj left a comment

grdddj commented Jan 7, 2022

matejcik commented Jan 7, 2022

grdddj commented Jan 7, 2022

prusnak commented Jan 7, 2022

jimpo commented Jan 9, 2022

prusnak commented Jan 9, 2022

Pyright in core #1939

Pyright in core #1939

Conversation

grdddj commented Nov 24, 2021

grdddj commented Nov 24, 2021

grdddj commented Nov 25, 2021

matejcik commented Jan 5, 2022

grdddj left a comment

Choose a reason for hiding this comment

grdddj commented Jan 7, 2022

matejcik commented Jan 7, 2022

grdddj commented Jan 7, 2022

prusnak commented Jan 7, 2022

jimpo commented Jan 9, 2022

prusnak commented Jan 9, 2022