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

Published types aren't complete enough for Pyright/Pylance #660

Open
fluggo opened this issue May 20, 2021 · 10 comments
Open

Published types aren't complete enough for Pyright/Pylance #660

fluggo opened this issue May 20, 2021 · 10 comments
Labels

Comments

@fluggo
Copy link

fluggo commented May 20, 2021

It seems a bit unfair to file this as a "bug," when really what's going on is that the Python community is trying to figure out what a "typed" Python library looks like. In this case, what looks like perfectly reasonable code is, in another light, not explicit enough to allow tools to determine types.

(For clarity's sake: Pylance is Microsoft's new language-analysis extension for Python in VS Code. Pylance is closed source, but wraps the open source type engine Pyright.)

Expected Result

I have a repo at https://github.com/fluggo/pylance-test that demonstrates this issue. Pylance 2021.5.3 now knows to bypass the stub libraries when the installed library uses py.typed to declare itself typed, and so it should be able to see that this call to encode is wrong:

import jwt

# This should be an error because payload expects Dict[str, Any]
jwt.encode(payload='subsandwich')

Actual Result

Unfortunately it doesn't, because according to Pyright, the type is unknown:

image

This is because Pyright has decided to play it safe with type inferences. The problematic code is at the bottom of api_jwt.py:

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

These assignments look cut and dry—PyJWT is just creating a module-level alias for these functions. But it only looks that way to a programmer—putting our language lawyer hats on, these are variables, not constants, and they can be reassigned. Since that's the case, what are the other valid values for these variables? Pyright can't know, and can't assume they will be constrained to the type of their first assignment.

There's also the issue that, if an assignment like this requires any type inference at all, the results could be different from one analysis tool to the next. This is different from the situation in TypeScript, because in TypeScript, there's only the one implementation of type inference, whereas Python doesn't yet have a standard for inferring types.

In Pyright's analysis, these variables need their own type annotations, and they recommend so in their Typing Guidance for Python Libraries.

The discussion on what to do here is still ongoing in microsoft/pyright#1846, but as things sit now, this is something library implementors will have to wrestle with. There's also a bit more background in microsoft/pylance-release#1197.

Reproduction Steps

Grab my repo above and look at it with Pylance under VS Code. Apologies to anyone not using Poetry; I'm not well versed with the Python package managers.

System Information

$ python -m jwt.help
{
  "cryptography": {
    "version": "3.4.7"
  },
  "implementation": {
    "name": "CPython",
    "version": "3.9.4"
  },
  "platform": {
    "release": "19.6.0",
    "system": "Darwin"
  },
  "pyjwt": {
    "version": "2.1.0"
  }
}
@auvipy
Copy link
Collaborator

auvipy commented Oct 9, 2021

do you have any improvement plans?

@dajiaji
Copy link
Contributor

dajiaji commented Oct 11, 2021

@auvipy @fluggo I think the following code is reasonable to fix this issue for now. It is redundant and we have to maintain the two functions which have the same interface though.

If you are OK with this change, I can send a PR.

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

# --- decode_complete = _jwt_global_obj.decode_complete
def decode_complete(
    jwt: str,
    key: str = "",
    algorithms: List[str] = None,
    options: Dict = None,
    audience: Optional[Union[str, List[str]]] = None,
    issuer: Optional[str] = None,
    leeway: Union[float, timedelta] = 0,
) -> Dict[str, Any]:
    return _jwt_global_obj.decode_complete(jwt, key, algorithms, options, audience, issuer, leeway)

# --- decode = _jwt_global_obj.decode
def decode(
    jwt: str,
    key: str = "",
    algorithms: List[str] = None,
    options: Dict = None,
    audience: Optional[Union[str, List[str]]] = None,
    issuer: Optional[str] = None,
    leeway: Union[float, timedelta] = 0,
) -> Dict[str, Any]:
    return _jwt_global_obj.decode(jwt, key, algorithms, options, audience, issuer, leeway)

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label May 19, 2022
@Tinche
Copy link

Tinche commented May 26, 2022

Looks fine to me in the latest VS Code.

Screenshot 2022-05-26 at 15 05 30

The bigger problem is the annotations seem to be wrong, key isn't just a string.

@github-actions github-actions bot removed the stale Issues without activity for more than 60 days label May 27, 2022
@kkirsche
Copy link
Contributor

kkirsche commented Jul 8, 2022

I may be able to take a stab at expanding and cleaning up the type hints, incorporating typing extensions so that the types are usable by all Python 3.6+ versions, etc.

It sounds like the key is working backwards from the "core" functions out towards the user API to ensure its accuracy for things like keys, etc.

@tvallois
Copy link

I'm still having some issues with pyright when typeCheckingMode is strict.
image

Probably because key and value types are Unknown for pyright for headers parameter.

@kkirsche
Copy link
Contributor

Yeah, to fix the user-facing ones means going from the ground up and reworking some of the interfaces in the crypto portions to be more consistent so that the higher-level abstractions can expose correct types. It’s more involved than expected but not too horrible as long as you start from third-party modules, install proper types for those (eg types-cryptography) and then just work your way up through the whole app fixing things.

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Sep 12, 2022
@kkirsche
Copy link
Contributor

Stale doesn't mean addressed

@jpadilla jpadilla added keep and removed stale Issues without activity for more than 60 days labels Sep 12, 2022
@Delgan
Copy link

Delgan commented Oct 31, 2024

At least one reason of failure is that AllowedPrivateKeys is not defined when the cryptography package is not installed. This causes pyright to consider the signature of encode() to be "partially unknown" in strict mode.

key: AllowedPrivateKeys | PyJWK | str | bytes,

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

No branches or pull requests

8 participants