pyright reports using private imports and attribute access errors on example code

[davefiddes]

I have been using pyright for type checking of my project using canopen in preference top mypy as it seems to be the default in VS Code. As part of upgrading to 2.3.0 I found it reporting canopen.Network() as a private symbol.

I have been able to distill a minimal example of the problem using the example code from the readme and the following environment:

virtualenv venv
. venv/bin/activate
pip install canopen pyright

Running pyright then yields a lot of errors:

$ pyright
/tmp/pytest/canopen-example.py
  /tmp/pytest/canopen-example.py:4:19 - error: "Network" is not exported from module "canopen" (reportPrivateImportUsage)
  /tmp/pytest/canopen-example.py:7:16 - error: "RemoteNode" is not exported from module "canopen" (reportPrivateImportUsage)
  /tmp/pytest/canopen-example.py:22:52 - error: Cannot access attribute "raw" for class "SdoArray"
    Attribute "raw" is unknown (reportAttributeAccessIssue)
  /tmp/pytest/canopen-example.py:22:52 - error: Cannot access attribute "raw" for class "SdoRecord"
    Attribute "raw" is unknown (reportAttributeAccessIssue)
  /tmp/pytest/canopen-example.py:23:13 - error: "__getitem__" method not defined on type "SdoVariable" (reportIndexIssue)
  /tmp/pytest/canopen-example.py:26:37 - error: Cannot assign to attribute "raw" for class "SdoArray"
    Attribute "raw" is unknown (reportAttributeAccessIssue)
  /tmp/pytest/canopen-example.py:26:37 - error: Cannot assign to attribute "raw" for class "SdoRecord"
    Attribute "raw" is unknown (reportAttributeAccessIssue)
  /tmp/pytest/canopen-example.py:51:45 - error: Cannot access attribute "raw" for class "PdoMap"
    Attribute "raw" is unknown (reportAttributeAccessIssue)
8 errors, 0 warnings, 0 informations 

It seems that https://github.com/microsoft/pyright/blob/main/docs/typed-libraries.md#library-interface describes the rules for module imports.

[acolomb]

The first two are false positives. The names are definitely imported in canopen/__init__.py .

The other ones look like wrong usage of the API. Only the variable classes have a .raw attribute, while only records and arrays have item getters.

[sveinse]

Not precisely false positive the two first, unfortunately. pyright and other linters complain if imports are not explicitly exported by __all__. So canopen/__init__.py needs:

from canopen.network import Network, NodeScanner
__all__ = [
    "Network",
    "NodeScanner",
]

I'm using VS Code which use pylance (=pyright under the hood). These imports show up as "unused" until they are exported via __all__. mypy also have options for this https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport which gets enabled under strict.

[sveinse]

Since we don't have the code which generates these errors, allow me to speculate. The SdoBase.__getitem__() methods returns a value of type Union[SdoVariable, SdoArray, SdoRecord]. It is a common pattern to do devicetype = remote.sdo[0x1000].raw. We know that this object is an SdoVariable so we do .raw access on it. But the linter doesn't know that. It tests all possible types for the attribute getter, including SdoArray and SdoRecord. I strongly suspect this is the error that's being reported.

The fix is to add an explicit type test, a cast or use a proxy method that only return the right type (which is a cast of sorts).

[acolomb]

Possible, but that's kinda the way Python works and which allows us to provide a lean an easy API. Dynamic typing is a feature in this context, and I don't think we should bend over backwards just to make some linter not complain about stuff it simply cannot know, but which is nonetheless absolutely correct if you know the context. If absolute type safety is required, then a statically typed language might be preferable, which mostly results in longer code because cases like this need handling through extra conditionals.

I'm totally okay with the __all__ attribute thing, go ahead and make a PR for that one if you have time.

With all other workarounds, you basically add some code (e.g. an assert) just to shut up the linter. But it doesn't change the fact that the breakage will happen at runtime in case the OD had declared the object of wrong type. The only thing it possibly does is change the error message.

[sveinse]

Well, the linter is not wrong pointing out that the returned type might be of several types and that .raw only exists for one of them. The only way to avoid run-time exception, which the linter is trying to help identify, is for the user to check the returned type. Tedious? Yes. But accurate.

The problem for the user, is that this in turn quickly becomes a merry chase of silencing the linter or adding code for the linter. For the architecture of canopen, I don't really think multi-object interfaces like this can be avoided.

As for PR, I have #360 which I been working on. Albeit slowly - I need to update it for the latest updates from master.

[davefiddes]

Since we don't have the code which generates these errors, allow me to speculate.

I'm sorry. I should have been clearer. I took the example code from the Quick Start section of the readme and copy-pasted it into a file called canopen-example.py. I made no changes to the code.

From my reading it seems that adding the public symbols to __all__ would solve the problems I see in my own project. I raised the issue because of the wider problems I saw when turning on type checking on the example code.

[friederschueler]

We are in the middle of improving the type hinting step by step. there are some PRs pending which will improve type checking/correctness.

I'm totally okay with the all attribute thing, go ahead and make a PR for that one if you have time.

Added a minimalistic PR.

@davefiddes if you are happy with the answers, you can close this. The Quick Start is to help people get started with the API fast. More useless code that makes linters happy would just be distraction.

[davefiddes]

Thanks. That change seems to fix things back to where they were with 2.2.0.