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

Enable mypy checking for unreachable code and fix instances. #8432

Merged
merged 1 commit into from
Oct 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/8432.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Check for unreachable code with mypy.
1 change: 1 addition & 0 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ check_untyped_defs = True
show_error_codes = True
show_traceback = True
mypy_path = stubs
warn_unreachable = True
files =
synapse/api,
synapse/appservice,
Expand Down
18 changes: 9 additions & 9 deletions synapse/config/tls.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import warnings
from datetime import datetime
from hashlib import sha256
from typing import List
from typing import List, Optional

from unpaddedbase64 import encode_base64

Expand Down Expand Up @@ -177,8 +177,8 @@ def read_config(self, config: dict, config_dir_path: str, **kwargs):
"use_insecure_ssl_client_just_for_testing_do_not_use"
)

self.tls_certificate = None
self.tls_private_key = None
self.tls_certificate = None # type: Optional[crypto.X509]
self.tls_private_key = None # type: Optional[crypto.PKey]

def is_disk_cert_valid(self, allow_self_signed=True):
"""
Expand Down Expand Up @@ -226,12 +226,12 @@ def is_disk_cert_valid(self, allow_self_signed=True):
days_remaining = (expires_on - now).days
return days_remaining

def read_certificate_from_disk(self, require_cert_and_key):
def read_certificate_from_disk(self, require_cert_and_key: bool):
"""
Read the certificates and private key from disk.

Args:
require_cert_and_key (bool): set to True to throw an error if the certificate
require_cert_and_key: set to True to throw an error if the certificate
and key file are not given
"""
if require_cert_and_key:
Expand Down Expand Up @@ -480,13 +480,13 @@ def generate_config_section(
}
)

def read_tls_certificate(self):
def read_tls_certificate(self) -> crypto.X509:
"""Reads the TLS certificate from the configured file, and returns it

Also checks if it is self-signed, and warns if so

Returns:
OpenSSL.crypto.X509: the certificate
The certificate
"""
cert_path = self.tls_certificate_file
logger.info("Loading TLS certificate from %s", cert_path)
Expand All @@ -505,11 +505,11 @@ def read_tls_certificate(self):

return cert

def read_tls_private_key(self):
def read_tls_private_key(self) -> crypto.PKey:
"""Reads the TLS private key from the configured file, and returns it

Returns:
OpenSSL.crypto.PKey: the private key
The private key
"""
private_key_path = self.tls_private_key_file
logger.info("Loading TLS key from %s", private_key_path)
Expand Down
5 changes: 2 additions & 3 deletions synapse/federation/federation_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
Callable,
Dict,
List,
Match,
Optional,
Tuple,
Union,
Expand Down Expand Up @@ -803,14 +802,14 @@ def server_matches_acl_event(server_name: str, acl_event: EventBase) -> bool:
return False


def _acl_entry_matches(server_name: str, acl_entry: str) -> Match:
def _acl_entry_matches(server_name: str, acl_entry: Any) -> bool:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callers only ever checked for whether it is truth-y or not.

if not isinstance(acl_entry, str):
logger.warning(
"Ignoring non-str ACL entry '%s' (is %s)", acl_entry, type(acl_entry)
)
return False
regex = glob_to_regex(acl_entry)
return regex.match(server_name)
return bool(regex.match(server_name))


class FederationHandlerRegistry:
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ async def _user_can_delete_alias(self, alias: RoomAlias, user_id: str):
"""
creator = await self.store.get_room_alias_creator(alias.to_string())

if creator is not None and creator == user_id:
if creator == user_id:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_room_alias_creator has to return a str, I think.

return True

# Resolve the alias to the corresponding room.
Expand Down
2 changes: 0 additions & 2 deletions synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -962,8 +962,6 @@ async def _generate_room_id(
try:
random_string = stringutils.random_string(18)
gen_room_id = RoomID(random_string, self.hs.hostname).to_string()
if isinstance(gen_room_id, bytes):
gen_room_id = gen_room_id.decode("utf-8")
await self.store.store_room(
room_id=gen_room_id,
room_creator_user_id=creator_id,
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ async def copy_user_state_on_room_upgrade(

async def send_membership_event(
self,
requester: Requester,
requester: Optional[Requester],
event: EventBase,
context: EventContext,
ratelimit: bool = True,
Expand Down
2 changes: 1 addition & 1 deletion synapse/handlers/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class SyncConfig:
class TimelineBatch:
prev_batch = attr.ib(type=StreamToken)
events = attr.ib(type=List[EventBase])
limited = attr.ib(bool)
limited = attr.ib(type=bool)

def __bool__(self) -> bool:
"""Make the result appear empty if there are no updates. This is used
Expand Down
4 changes: 2 additions & 2 deletions synapse/http/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ async def _async_render(self, request: Request):
if isinstance(raw_callback_return, (defer.Deferred, types.CoroutineType)):
callback_return = await raw_callback_return
else:
callback_return = raw_callback_return
callback_return = raw_callback_return # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not figure out how to coerce mypy into realize that this might be an awaitable or might not be. 🤷


return callback_return

Expand Down Expand Up @@ -406,7 +406,7 @@ async def _async_render(self, request):
if isinstance(raw_callback_return, (defer.Deferred, types.CoroutineType)):
callback_return = await raw_callback_return
else:
callback_return = raw_callback_return
callback_return = raw_callback_return # type: ignore

return callback_return

Expand Down
10 changes: 1 addition & 9 deletions synapse/logging/_structured.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import logging
import os.path
import sys
Expand Down Expand Up @@ -89,14 +88,7 @@ def __call__(self, event: dict) -> None:
context = current_context()

# Copy the context information to the log event.
if context is not None:
context.copy_to_twisted_log_entry(event)
else:
# If there's no logging context, not even the root one, we might be
# starting up or it might be from non-Synapse code. Log it as if it
# came from the root logger.
event["request"] = None
event["scope"] = None
context.copy_to_twisted_log_entry(event)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current_context no longer returns None, but _SentinelContext (#7120), which implements copy_to_twisted_log_entry with the removed logic.


self.observer(event)

Expand Down
4 changes: 2 additions & 2 deletions synapse/push/push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import logging
import re
from typing import Any, Dict, List, Pattern, Union
from typing import Any, Dict, List, Optional, Pattern, Union

from synapse.events import EventBase
from synapse.types import UserID
Expand Down Expand Up @@ -181,7 +181,7 @@ def _contains_display_name(self, display_name: str) -> bool:

return r.search(body)

def _get_value(self, dotted_key: str) -> str:
def _get_value(self, dotted_key: str) -> Optional[str]:
return self._value_cache.get(dotted_key, None)


Expand Down
10 changes: 6 additions & 4 deletions synapse/replication/tcp/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,11 @@
import logging
import struct
from inspect import isawaitable
from typing import TYPE_CHECKING, List
from typing import TYPE_CHECKING, List, Optional

from prometheus_client import Counter

from twisted.internet import task
from twisted.protocols.basic import LineOnlyReceiver
from twisted.python.failure import Failure

Expand Down Expand Up @@ -152,9 +153,10 @@ def __init__(self, clock: Clock, handler: "ReplicationCommandHandler"):

self.last_received_command = self.clock.time_msec()
self.last_sent_command = 0
self.time_we_closed = None # When we requested the connection be closed
# When we requested the connection be closed
self.time_we_closed = None # type: Optional[int]

self.received_ping = False # Have we reecived a ping from the other side
self.received_ping = False # Have we received a ping from the other side

self.state = ConnectionStates.CONNECTING

Expand All @@ -165,7 +167,7 @@ def __init__(self, clock: Clock, handler: "ReplicationCommandHandler"):
self.pending_commands = [] # type: List[Command]

# The LoopingCall for sending pings.
self._send_ping_loop = None
self._send_ping_loop = None # type: Optional[task.LoopingCall]

# a logcontext which we use for processing incoming commands. We declare it as a
# background process so that the CPU stats get reported to prometheus.
Expand Down
2 changes: 1 addition & 1 deletion synapse/state/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ def _make_state_cache_entry(

# failing that, look for the closest match.
prev_group = None
delta_ids = None
delta_ids = None # type: Optional[StateMap[str]]

for old_group, old_state in state_groups_ids.items():
n_delta_ids = {k: v for k, v in new_state.items() if old_state.get(k) != v}
Expand Down
6 changes: 3 additions & 3 deletions synapse/storage/databases/main/censor_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
from synapse.storage._base import SQLBaseStore
from synapse.storage.database import DatabasePool
from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore
from synapse.storage.databases.main.events import encode_json
from synapse.storage.databases.main.events_worker import EventsWorkerStore
from synapse.util.frozenutils import frozendict_json_encoder

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand Down Expand Up @@ -105,7 +105,7 @@ async def _censor_redactions(self):
and original_event.internal_metadata.is_redacted()
):
# Redaction was allowed
pruned_json = encode_json(
pruned_json = frozendict_json_encoder.encode(
prune_event_dict(
original_event.room_version, original_event.get_dict()
)
Expand Down Expand Up @@ -171,7 +171,7 @@ def delete_expired_event_txn(txn):
return

# Prune the event's dict then convert it to JSON.
pruned_json = encode_json(
pruned_json = frozendict_json_encoder.encode(
prune_event_dict(event.room_version, event.get_dict())
)

Expand Down
18 changes: 5 additions & 13 deletions synapse/storage/databases/main/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,6 @@
)


def encode_json(json_object):
"""
Encode a Python object as JSON and return it in a Unicode string.
"""
out = frozendict_json_encoder.encode(json_object)
if isinstance(out, bytes):
out = out.decode("utf8")
Comment on lines -60 to -61
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was dealing with py2 encoder logic, which made this function fairly useless so I figured it made more sense to inline it.

return out


_EventCacheEntry = namedtuple("_EventCacheEntry", ("event", "redacted_event"))


Expand Down Expand Up @@ -743,7 +733,9 @@ def _update_outliers_txn(self, txn, events_and_contexts):
logger.exception("")
raise

metadata_json = encode_json(event.internal_metadata.get_dict())
metadata_json = frozendict_json_encoder.encode(
event.internal_metadata.get_dict()
)

sql = "UPDATE event_json SET internal_metadata = ? WHERE event_id = ?"
txn.execute(sql, (metadata_json, event.event_id))
Expand Down Expand Up @@ -797,10 +789,10 @@ def event_dict(event):
{
"event_id": event.event_id,
"room_id": event.room_id,
"internal_metadata": encode_json(
"internal_metadata": frozendict_json_encoder.encode(
event.internal_metadata.get_dict()
),
"json": encode_json(event_dict(event)),
"json": frozendict_json_encoder.encode(event_dict(event)),
"format_version": event.format_version,
}
for event, _ in events_and_contexts
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/databases/main/stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,7 @@ async def get_recent_event_ids_for_room(

async def get_room_event_before_stream_ordering(
self, room_id: str, stream_ordering: int
) -> Tuple[int, int, str]:
) -> Optional[Tuple[int, int, str]]:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can return None if no event is found.

"""Gets details of the first event in a room at or before a stream ordering

Args:
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/util/id_generators.py
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ def _mark_id_as_finished(self, next_id: int):
self._unfinished_ids.discard(next_id)
self._finished_ids.add(next_id)

new_cur = None
new_cur = None # type: Optional[int]

if self._unfinished_ids:
# If there are unfinished IDs then the new position will be the
Expand Down