jpadilla / pyjwt

JSON Web Token implementation in Python
https://pyjwt.readthedocs.io
MIT License
5.17k stars 689 forks source link

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

Open fluggo opened 3 years ago

fluggo commented 3 years ago

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 commented 3 years ago

do you have any improvement plans?

dajiaji commented 3 years ago

@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[bot] commented 2 years ago

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

Tinche commented 2 years ago

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.

kkirsche commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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[bot] commented 2 years ago

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

kkirsche commented 2 years ago

Stale doesn't mean addressed

Delgan commented 2 weeks ago

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.

https://github.com/jpadilla/pyjwt/blob/189c2561c47030d2e34aea040cf135ae4997a2d8/jwt/api_jwt.py#L52