Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pylance failing with psycopg #2267

Closed
eykamp opened this issue Jan 20, 2022 · 5 comments
Closed

Pylance failing with psycopg #2267

eykamp opened this issue Jan 20, 2022 · 5 comments

Comments

@eykamp
Copy link

eykamp commented Jan 20, 2022

VS Code/pylance are flagging lots of errors with psycopg (v3, which has type hints baked in to everything, not v2 which doesn't). Using the sample code flags lots of things as having missing types, but looking at the source shows everything seems tightly typed. I haven't tried, but the psycopg devs swear the code looks clean with mypy (see conversation here).

I'll note that the code runs (at least until the login fails) so the libraries are properly installed.

Here is a sample:

image

Environment data

  • Language Server version: 2022.1.1
  • OS and version: win32 x64
  • Python version Python 3.7.10, Anaconda:
  • python.analysis.indexing: undefined
  • python.analysis.typeCheckingMode: strict
@erictraut
Copy link
Contributor

This library is marked as "py.typed", which means that it is claiming to be fully typed. While it does contain some type information, some of the symbols are lacking type annotations.

Pyright (the type checker that pylance is built upon) has a command-line option "--verifytypes" that you can run on a "py.typed" library to determine its "type completeness". When I run pyright --verifytypes psycopg on the latest published version of the library, I get the following report:

Symbols exported by "psycopg": 1473
  With known type: 1121
  With partially unknown type: 352
  Functions without docstring: 456
  Functions without default param: 82
  Classes without docstring: 201

Other symbols referenced but not exported by "psycopg": 1038
  With known type: 919
  With partially unknown type: 119

Type completeness score: 76.1%

For "py.typed" libraries, pyright (and pylance) rely on type annotations. Symbols that have no type annotations are assumed to be "Any" / "Unknown". Guidance for library maintainers can be found here. Pyright fully implements this guidance, whereas mypy has not yet been updated to comply with these rules. That explains why you are seeing the difference between the two type checkers.

If this library is important to you, please reach out to the maintainer and recommend that they add the missing type annotations. And if they have any questions, we're happy to assist.

In the meantime, consumers of psycopg have a couple of options:

  1. Disable strict mode type checking in pyright / pylance so you don't receive error messages for unknown types.
  2. Manually delete the "py.typed" file from the installed library so pyright / pylance falls back on type inference for symbols that are missing type annotations.

@eykamp
Copy link
Author

eykamp commented Jan 20, 2022

Ok, thank you!!!

@dvarrazzo
Copy link

Hello,

For "py.typed" libraries, pyright (and pylance) rely on type annotations. Symbols that have no type annotations are assumed to be "Any" / "Unknown". Guidance for library maintainers can be found here. Pyright fully implements this guidance, whereas mypy has not yet been updated to comply with these rules. That explains why you are seeing the difference between the two type checkers.

I have been reported that pylance reports this attribute as untyped:

class BaseCursor(Generic[ConnectionType, Row]):
    ...
    def __init__(self, connection: ConnectionType):                             
        self._conn = connection
        self.format = Format.TEXT  # Missing type annotation for public attibute here

format type is well specified as being Format. It cannot be otherwise, or mypy would complain. Requiring to add a : Format specification here is redundant.

I have read the reference document and

# Class with partially unknown type
class BaseClass:
    # Missing type annotation
    height = 2.0

It is overly bureaucratic to require height to be specified as float: it is a float literal, it can't be better specified. I have read the "explicit is better than implicit" card played but this doesn't seem the case.

I would like this detail of the spec to be discussed instead of being implemented by a tool, called a standard, and declaring other tools not updated.

@judej judej added the reference label Feb 7, 2022
@github-actions github-actions bot removed the triage label Feb 7, 2022
@erictraut
Copy link
Contributor

Hi @dvarrazzo, thanks for reaching out to us. I see that you’re the maintainer of the psycopg library.

This might not be the best forum for this discussion since this is a closed issue in pylance. It might be better to create a new discussion in the python/typing project. That is where the active members of the Python typing community tend to hang out, and it would give it more visibility and allow others to participate.

I can understand how it may appear that a type annotation for a class variable “height” serves no use here, but there are some factors that you may not be aware of.

First, a value assignment and a type declaration have very different meanings to a static type analyzer. A value assignment (such as height = 2.0) simply means that there is a class variable named height that is initialized with a value of 2.0. It says nothing about what value types can be assigned to that variable. Does the class still work if you assign a complex value to this variable? What if you assign None to it? A type declaration (height: float or height: float = 2.0) has a well-defined meaning in the Python typing standards (PEP 484, etc.). It tells all type checkers and language servers that the variable accepts only values of that type or compatible subtypes. The Python typing standards say little or nothing about variable assignments and what type rules should be enforced in this case. Most static type checkers and language servers will perform some form of type inference to try to determine the intent of the code, but type inference rules are not specified, and they differ across type checkers.

Second, while we’re open to the idea of standardizing certain special cases where value assignments imply the same thing as a type declaration, specifying those special cases has proven to be complicated. If we allow height = 2.0, would we also allow height = 2.0 + 2.0? What about height = 2.0 + some_variable? Or height = 2.0 if conditional else None? Or height = some_function()? If some_function does not have a return type annotation, should its return type must be inferred from the return statements? What if height = 2.0 appears within the class body but self.height = None appears within the __init__ method? What about cases like x = [1, 2] where the inferred type is ambiguous (List[int]? List[Any]? Sequence[float]? Iterable[int]?). These are all complications that static type checkers need to deal with. It’s easy to point to a specific example and say “why can’t this particular case be exempted”, but if you want consistent behavior across all static type checkers and language servers, the rules need to be clearly spelled out along with the exceptions. This reasoning has led authors of the major Python type checkers to agree that the best solution is to say that all symbols that comprise a library’s public interface contract (including public classes, functions, methods, and instance and class variables) should have explicit type annotations. Note that this doesn’t apply to local variables, internal classes, etc. It applies only to the public interface contract — the interface that you document and support for your library. You can think of these type annotations as a special form of documentation that is understandable and enforceable by all Python tooling in a consistent manner. So while it may seem like a burden to add type annotations to your public interface, it’s a one-time investment that has a lot of benefits for all of the consumers of your library.

Like I said, it’s probably best to move this discussion to a more public forum so others can participate.

@dvarrazzo
Copy link

Most of the example you point at are well understood by the type inference rules that mypy produces and I see no reason why class attributes must follow a different, stricter rule compared to local variable.

If height = 2.0 but I want the possibility to set it to None, I will set it Optional[float]. The default, the strictest type, is meaningful. height = 2.0 if conditional else None is understood as Optional[float] by mypy, as well by humans. If the class is designed to have it assume a different type then it should be annotated. x = [1, 2] is a list of integers; if the class can accept a sequence of floats too it should be annotated as so, but the default, which you are discarding, which is consistently inferred in a local variable, is an especially meaningful one.

I will follow your advice to write to python typing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants