Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Use mypy 1.0 (#15052)
Browse files Browse the repository at this point in the history
* Update mypy and mypy-zope
* Remove unused ignores

These used to suppress

```
synapse/storage/engines/__init__.py:28: error: "__new__" must return a
class instance (got "NoReturn")  [misc]
```

and

```
synapse/http/matrixfederationclient.py:1270: error: "BaseException" has no attribute "reasons"  [attr-defined]
```

(note that we check `hasattr(e, "reasons")` above)

* Avoid empty body warnings, sometimes by marking methods as abstract

E.g.

```
tests/handlers/test_register.py:58: error: Missing return statement  [empty-body]
tests/handlers/test_register.py:108: error: Missing return statement  [empty-body]
```

* Suppress false positive about `JaegerConfig`

Complaint was

```
synapse/logging/opentracing.py:450: error: Function "Type[Config]" could always be true in boolean context  [truthy-function]
```

* Fix not calling `is_state()`

Oops!

```
tests/rest/client/test_third_party_rules.py:428: error: Function "Callable[[], bool]" could always be true in boolean context  [truthy-function]
```

* Suppress false positives from ParamSpecs

````
synapse/logging/opentracing.py:971: error: Argument 2 to "_custom_sync_async_decorator" has incompatible type "Callable[[Arg(Callable[P, R], 'func'), **P], _GeneratorContextManager[None]]"; expected "Callable[[Callable[P, R], **P], _GeneratorContextManager[None]]"  [arg-type]
synapse/logging/opentracing.py:1017: error: Argument 2 to "_custom_sync_async_decorator" has incompatible type "Callable[[Arg(Callable[P, R], 'func'), **P], _GeneratorContextManager[None]]"; expected "Callable[[Callable[P, R], **P], _GeneratorContextManager[None]]"  [arg-type]
````

* Drive-by improvement to `wrapping_logic` annotation

* Workaround false "unreachable" positives

See Shoobx/mypy-zope#91

```
tests/http/test_proxyagent.py:626: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:762: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:826: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:838: error: Statement is unreachable  [unreachable]
tests/http/test_proxyagent.py:845: error: Statement is unreachable  [unreachable]
tests/http/federation/test_matrix_federation_agent.py:151: error: Statement is unreachable  [unreachable]
tests/http/federation/test_matrix_federation_agent.py:452: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:60: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:93: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:127: error: Statement is unreachable  [unreachable]
tests/logging/test_remote_handler.py:152: error: Statement is unreachable  [unreachable]
```

* Changelog

* Tweak DBAPI2 Protocol to be accepted by mypy 1.0

Some extra context in:
- matrix-org/python-canonicaljson#57
- python/mypy#6002
- https://mypy.readthedocs.io/en/latest/common_issues.html#covariant-subtyping-of-mutable-protocol-members-is-rejected

* Pull in updated canonicaljson lib

so the protocol check just works

* Improve comments in opentracing

I tried to workaround the ignores but found it too much trouble.

I think the corresponding issue is
python/mypy#12909. The mypy repo has a PR
claiming to fix this (python/mypy#14677) which
might mean this gets resolved soon?

* Better annotation for INTERACTIVE_AUTH_CHECKERS

* Drive-by AUTH_TYPE annotation, to remove an ignore
  • Loading branch information
David Robertson authored Feb 16, 2023
1 parent 979f237 commit ffc2ee5
Show file tree
Hide file tree
Showing 17 changed files with 209 additions and 104 deletions.
1 change: 1 addition & 0 deletions changelog.d/15052.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Improve type hints.
69 changes: 36 additions & 33 deletions poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion synapse/handlers/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def __init__(self, hs: "HomeServer"):
for auth_checker_class in INTERACTIVE_AUTH_CHECKERS:
inst = auth_checker_class(hs)
if inst.is_enabled():
self.checkers[inst.AUTH_TYPE] = inst # type: ignore
self.checkers[inst.AUTH_TYPE] = inst

self.bcrypt_rounds = hs.config.registration.bcrypt_rounds

Expand Down
18 changes: 14 additions & 4 deletions synapse/handlers/ui_auth/checkers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
# limitations under the License.

import logging
from typing import TYPE_CHECKING, Any
from abc import ABC, abstractmethod
from typing import TYPE_CHECKING, Any, ClassVar, Sequence, Type

from twisted.web.client import PartialDownloadError

Expand All @@ -27,19 +28,28 @@
logger = logging.getLogger(__name__)


class UserInteractiveAuthChecker:
class UserInteractiveAuthChecker(ABC):
"""Abstract base class for an interactive auth checker"""

def __init__(self, hs: "HomeServer"):
# This should really be an "abstract class property", i.e. it should
# be an error to instantiate a subclass that doesn't specify an AUTH_TYPE.
# But calling this a `ClassVar` is simpler than a decorator stack of
# @property @abstractmethod and @classmethod (if that's even the right order).
AUTH_TYPE: ClassVar[str]

def __init__(self, hs: "HomeServer"): # noqa: B027
pass

@abstractmethod
def is_enabled(self) -> bool:
"""Check if the configuration of the homeserver allows this checker to work
Returns:
True if this login type is enabled.
"""
raise NotImplementedError()

@abstractmethod
async def check_auth(self, authdict: dict, clientip: str) -> Any:
"""Given the authentication dict from the client, attempt to check this step
Expand Down Expand Up @@ -304,7 +314,7 @@ async def check_auth(self, authdict: dict, clientip: str) -> Any:
)


INTERACTIVE_AUTH_CHECKERS = [
INTERACTIVE_AUTH_CHECKERS: Sequence[Type[UserInteractiveAuthChecker]] = [
DummyAuthChecker,
TermsAuthChecker,
RecaptchaAuthChecker,
Expand Down
2 changes: 1 addition & 1 deletion synapse/http/matrixfederationclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -1267,7 +1267,7 @@ async def get_file(
def _flatten_response_never_received(e: BaseException) -> str:
if hasattr(e, "reasons"):
reasons = ", ".join(
_flatten_response_never_received(f.value) for f in e.reasons # type: ignore[attr-defined]
_flatten_response_never_received(f.value) for f in e.reasons
)

return "%s:[%s]" % (type(e).__name__, reasons)
Expand Down
24 changes: 17 additions & 7 deletions synapse/logging/opentracing.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def set_fates(clotho, lachesis, atropos, father="Zues", mother="Themis"):
)

import attr
from typing_extensions import ParamSpec
from typing_extensions import Concatenate, ParamSpec

from twisted.internet import defer
from twisted.web.http import Request
Expand Down Expand Up @@ -445,7 +445,7 @@ def init_tracer(hs: "HomeServer") -> None:
opentracing = None # type: ignore[assignment]
return

if not opentracing or not JaegerConfig:
if opentracing is None or JaegerConfig is None:
raise ConfigError(
"The server has been configured to use opentracing but opentracing is not "
"installed."
Expand Down Expand Up @@ -872,7 +872,7 @@ def extract_text_map(carrier: Dict[str, str]) -> Optional["opentracing.SpanConte

def _custom_sync_async_decorator(
func: Callable[P, R],
wrapping_logic: Callable[[Callable[P, R], Any, Any], ContextManager[None]],
wrapping_logic: Callable[Concatenate[Callable[P, R], P], ContextManager[None]],
) -> Callable[P, R]:
"""
Decorates a function that is sync or async (coroutines), or that returns a Twisted
Expand Down Expand Up @@ -902,10 +902,14 @@ def _wrapping_logic(func: Callable[P, R], *args: P.args, **kwargs: P.kwargs) ->
"""

if inspect.iscoroutinefunction(func):

# In this branch, R = Awaitable[RInner], for some other type RInner
@wraps(func)
async def _wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
async def _wrapper(
*args: P.args, **kwargs: P.kwargs
) -> Any: # Return type is RInner
with wrapping_logic(func, *args, **kwargs):
# type-ignore: func() returns R, but mypy doesn't know that R is
# Awaitable here.
return await func(*args, **kwargs) # type: ignore[misc]

else:
Expand Down Expand Up @@ -972,7 +976,11 @@ def _decorator(func: Callable[P, R]) -> Callable[P, R]:
if not opentracing:
return func

return _custom_sync_async_decorator(func, _wrapping_logic)
# type-ignore: mypy seems to be confused by the ParamSpecs here.
# I think the problem is https://github.com/python/mypy/issues/12909
return _custom_sync_async_decorator(
func, _wrapping_logic # type: ignore[arg-type]
)

return _decorator

Expand Down Expand Up @@ -1018,7 +1026,9 @@ def _wrapping_logic(
set_tag(SynapseTags.FUNC_KWARGS, str(kwargs))
yield

return _custom_sync_async_decorator(func, _wrapping_logic)
# type-ignore: mypy seems to be confused by the ParamSpecs here.
# I think the problem is https://github.com/python/mypy/issues/12909
return _custom_sync_async_decorator(func, _wrapping_logic) # type: ignore[arg-type]


@contextlib.contextmanager
Expand Down
9 changes: 6 additions & 3 deletions synapse/rest/media/v1/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import logging
import os
import urllib
from abc import ABC, abstractmethod
from types import TracebackType
from typing import Awaitable, Dict, Generator, List, Optional, Tuple, Type

Expand Down Expand Up @@ -284,13 +285,14 @@ async def respond_with_responder(
finish_request(request)


class Responder:
class Responder(ABC):
"""Represents a response that can be streamed to the requester.
Responder is a context manager which *must* be used, so that any resources
held can be cleaned up.
"""

@abstractmethod
def write_to_consumer(self, consumer: IConsumer) -> Awaitable:
"""Stream response into consumer
Expand All @@ -300,11 +302,12 @@ def write_to_consumer(self, consumer: IConsumer) -> Awaitable:
Returns:
Resolves once the response has finished being written
"""
raise NotImplementedError()

def __enter__(self) -> None:
def __enter__(self) -> None: # noqa: B027
pass

def __exit__(
def __exit__( # noqa: B027
self,
exc_type: Optional[Type[BaseException]],
exc_val: Optional[BaseException],
Expand Down
4 changes: 2 additions & 2 deletions synapse/storage/engines/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
except ImportError:

class PostgresEngine(BaseDatabaseEngine): # type: ignore[no-redef]
def __new__(cls, *args: object, **kwargs: object) -> NoReturn: # type: ignore[misc]
def __new__(cls, *args: object, **kwargs: object) -> NoReturn:
raise RuntimeError(
f"Cannot create {cls.__name__} -- psycopg2 module is not installed"
)
Expand All @@ -36,7 +36,7 @@ def __new__(cls, *args: object, **kwargs: object) -> NoReturn: # type: ignore[m
except ImportError:

class Sqlite3Engine(BaseDatabaseEngine): # type: ignore[no-redef]
def __new__(cls, *args: object, **kwargs: object) -> NoReturn: # type: ignore[misc]
def __new__(cls, *args: object, **kwargs: object) -> NoReturn:
raise RuntimeError(
f"Cannot create {cls.__name__} -- sqlite3 module is not installed"
)
Expand Down
Loading

0 comments on commit ffc2ee5

Please sign in to comment.