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 uses its own type stubs over actually installed libraries #1197

Closed
fluggo opened this issue Apr 27, 2021 · 17 comments
Closed

Pylance uses its own type stubs over actually installed libraries #1197

fluggo opened this issue Apr 27, 2021 · 17 comments
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@fluggo
Copy link

fluggo commented Apr 27, 2021

Environment data

  • Language Server version: v2021.4.2
  • OS and version: macOS Catalina 10.15.7
  • Python version (& distribution if applicable, e.g. Anaconda): Python 3.9.4 via Homebrew, installed in a Poetry virtual environment in workspace/.venv

Expected behaviour

Pylance should search the venv libraries to see if they support types before searching its own bundled type stubs.

Actual behaviour

Pylance, when operating on PyJWT 2.0.1, tries to flag that the return type of jwt.encode is bytes, not str:

image

...which would be helpful, except that by all rights, that definition does not exist. PyJWT's own code says:

def encode(
        self,
        payload: Dict[str, Any],
        key: str,
        algorithm: str = "HS256",
        headers: Optional[Dict] = None,
        json_encoder: Optional[Type[json.JSONEncoder]] = None,
    ) -> str

Pylance seems to be pulling in a different definition for the types than the ones defined in the installed library.

Logs

Pylance sets its search paths like so:

Search paths for /Users/fluggo/software/python-test/src
  /Users/fluggo/.vscode/extensions/ms-python.vscode-pylance-2021.4.2/dist/typeshed-fallback/stdlib
  /Users/fluggo/software/python-test/src
  /Users/fluggo/software/python-test/src
  /Users/fluggo/software/python-test/typings
  /Users/fluggo/.vscode/extensions/ms-python.vscode-pylance-2021.4.2/dist/typeshed-fallback/stubs/...
  /Users/fluggo/.vscode/extensions/ms-python.vscode-pylance-2021.4.2/dist/bundled/stubs
  /Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9
  /Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/lib-dynload
  /Users/fluggo/software/python-test/.venv/lib/python3.9/site-packages

Pylance parses its own type stub:

[BG(1)]   binding: /Users/fluggo/.vscode/extensions/ms-python.vscode-pylance-2021.4.2/dist/typeshed-fallback/stubs/jwt/jwt/__init__.pyi (1ms)

and the installed one:

[BG(1)]   parsing: /Users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/api_jwt.py (17ms)
[BG(1)]   binding: /Users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/api_jwt.py (4ms)

...but seems to prefer its own.

@jakebailey
Copy link
Member

jakebailey commented Apr 27, 2021

I recently added those logs and it does look like we are sticking typeshed before site-packages; it's likely we need to reexamine this ordering to ensure that it's something like:

  • extraPaths/PYTHONPATH
  • typings
  • workspace root
  • bundled partial
  • interpreter paths (like site-packages)
  • typeshed and bundled

But, it's a risky change to change this ordering, lest some stubs that work stop working.

@jakebailey
Copy link
Member

I will note that if pyjwt is stubbed and has py.typed, it might be good to ask typeshed to remove them.

@fluggo
Copy link
Author

fluggo commented Apr 27, 2021

PyJWT does have py.typed, or at least, this version does. It would make sense to keep the stubs for prior untyped versions of PyJWT, but check whether the site-packages version has types before using the stubs.

@jakebailey
Copy link
Member

Yes, I only suggest that as a workaround and courtesy to the typeshed project; it's a problem that we are using typeshed or our own stubs for projects that have their own.

@judej judej added the bug Something isn't working label Apr 28, 2021
@github-actions github-actions bot removed the triage label Apr 28, 2021
@jakebailey jakebailey added the needs investigation Could be an issue - needs investigation label Apr 28, 2021
@erictraut
Copy link
Contributor

Incidentally, I think this is the same issue as microsoft/pyright#1764.

@jakebailey
Copy link
Member

Yes, almost certainly.

@jakebailey jakebailey added fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed needs investigation Could be an issue - needs investigation labels May 14, 2021
@jakebailey
Copy link
Member

This issue has been fixed in version 2021.5.3, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202153-19-may-2021

@fluggo
Copy link
Author

fluggo commented May 20, 2021

Yes and no... while Pylance 2021.5.3 is certainly looking in the right place:

[BG(1)]   parsing: /users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/__init__.py [fs read 0ms] (1ms)
[BG(1)]   binding: /users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/__init__.py (1ms)
[BG(1)]   parsing: /users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/api_jwt.py [fs read 1ms] (8ms)
[BG(1)]   binding: /users/fluggo/software/python-test/.venv/lib/python3.9/site-packages/jwt/api_jwt.py (4ms)

...it's not getting the right type:

image

Minimal demo repo: https://github.com/fluggo/pylance-test

fluggo pushed a commit to fluggo/pylance-test that referenced this issue May 20, 2021
@erictraut
Copy link
Contributor

This highlights the problem with overriding typeshed stubs with "py.typed" type information. We find that sometimes library authors mark their library as "py.typed" when they do not provide complete type information in their library code. That's what you're seeing here with PyJwt.

For details about how pylance handles "py.typed" libraries — and what it expects in terms of type information, refer to this documentation.

Pyright (the type checker upon which pylance is built) contains a command-line option called --verifytypes that can be run on a "py.typed" library to determine its "type completeness score" — the percentage of symbols in its public interface that have complete type information. When I run this on the latest version of PyJwt, here's the result:

Symbols exported by "jwt": 253
  With known type: 93
  With partially unknown type: 160

Type completeness score: 36.8%

That's a pretty low type completeness score. The library should probably not be marked as "py.typed" until its type information is more complete.

Any symbol that has an "unknown type" will be treated as an Any type, and will potentially result in false negative errors like the one you're seeing here. I recommend filing bugs or PRs for PyJwt to address the missing type information.

We're also discussing the possibility of adding an option to pyright that allows you to ignore the "py.typed" for a library, falling back on type inference to try to infer the missing type information. Refer to this issue if you're interested in the details or would like to join in on the discussion.

@fluggo
Copy link
Author

fluggo commented May 20, 2021

If I'm understanding the documentation right, Pyright loses the plot here, at the end of api_jwt.py, when encode is aliased to a module variable?

_jwt_global_obj = PyJWT()
encode = _jwt_global_obj.encode
decode_complete = _jwt_global_obj.decode_complete
decode = _jwt_global_obj.decode

If so, that is... really unfortunate, as the implementor has to repeat a lot of type information just to make that simple statement work.

ETA: Reading further on the rationale, I see why Pyright decided to do this. It would be different if these variables could be declared const like in TypeScript; then you could safely infer their types. Still troubling, there ought to be an easier way...

@erictraut
Copy link
Contributor

Even if they were declared const, it probably wouldn't be a good idea to rely on type inference for the symbols that comprise a library's public interface contract. Type inference rules vary by type checker, so you can get different results if you rely on type inference rules.

To address this, library authors may need to add a few additional (otherwise unnecessary) type annotations. In most cases, this involves very little extra work, and it provides real benefits to consumers of that library. So my hope is that library authors won't see it as too onerous.

TypeScript has things a bit easier because there are not multiple type checkers for TypeScript; everything is handled by the TypeScript compiler itself, and there's only one of those. In the Python world there are many different implementations of type checkers, linters and language servers. This diversity is good, but it can also present some challenges.

@fluggo
Copy link
Author

fluggo commented May 20, 2021

Thank you for the background. Seems like Python still has a way to go in the typing journey, but I'd like to encourage it. I feel static checking like this is an essential tool, and Python is too cool of a language to miss out.

@jerryc05
Copy link

I also encountered this issue with pyjwt 2.1.0 using Pylance 2021.7.3-pre.1.

Have we reached a consensus on how to solve this problem?

@jakebailey
Copy link
Member

What problem are you referring to? We've removed the pyjwt stubs we had bundled, so they no longer conflict. If you mean the quality of the types, that is up to the library and is tracked in the cross-linked issue here: jpadilla/pyjwt#660

@jerryc05
Copy link

What problem are you referring to? We've removed the pyjwt stubs we had bundled, so they no longer conflict. If you mean the quality of the types, that is up to the library and is tracked in the cross-linked issue here: jpadilla/pyjwt#660

The encode method in pyjwt. It should return str type but the pylance said it returns bytes type even after reinstall.

@jakebailey
Copy link
Member

This will probably be fixed once python/typeshed#5767 is merged; I was wrong to say that we had removed the stubs.

@jakebailey
Copy link
Member

Though, we should still be picking pyjwt's own types, as it's py.typed (why this issue is closed), so maybe not. Hard to say without some trace logs to show what we are analyzing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

5 participants