Fix Mypy linting with latest Twisted

[H-Shay]

This PR sets a constraint for the Twisted version to avoid upstream version changes breaking CI, and fixes a few type errors introduced by an upstream Twisted version update.

[clokep]

It might be worth trying to fix up the type hints like we did for Synapse: matrix-org/synapse#10490, or does newer Twisted versions actually not pass other tests for some reason?

[H-Shay]

Twisted adding type hints, which caused the mypy CI check to fail is what precipitated was this PR, as you mentioned. Oliver had just suggested that pinning/constraining the version in general was probably the proper thing to do to avoid similar problems in the future. So pinning the version wasn't necessarily to avoid fixing the type hints, it was just to prevent CI from breaking unexpectedly in future.

[richvdh]

pinning a library to a particular version is oookayyy but, to my mind, brings with it a commitment to regularly review the latest changes and upgrade, to avoid getting stuck on an old version. (In general, it's much easier to keep on top of library changes with regular small updates than by doing one big update after months or years.) I don't think we're in a position to make that commitment here.

In the particular case of Twisted, it's worth noting that they have a very strong compatibility policy in which they essentially promise not to break runtime compatibility - but they also don't provide any security support for anything but the latest version.

It's unfortunate that the changes in Twisted 21.7 means that we require extra type hints, but that's very much an unusual case - and even then, it only breaks mypy rather than anything at runtime.

All that is a long-winded way of saying: I think pinning the version is okay as a temporary measure to make CI green again, but I don't think we should pin Twisted in the long term.

[reivilibre]

I took away the version constraint -- my mistake for suggesting it (I must have dreamt that we did it).
Rich is very correct in that it gives us the extra task of having to review updates from time to time; for a small project like Sygnal this probably means it would rarely get done. (I was also not aware of Twisted's compatibility policy, so I've learnt something there).

There are just 3 mypy issues -- just applying the small changes you had in the other PR should make everything happy again (and I suppose change the newsfile and remove the 237 newsfile), then this PR should be a good'un :)

[callahad]

brings with it a commitment to regularly review the latest changes and upgrade, to avoid getting stuck on an old version.

Sounds like we should let the target version in setup.py float, but use something like Poetry, Pipenv, or pip-tools to pin the specific versions we rely on so that our CI and releases are more deterministic. We can turn on Dependabot to ensure that everything is kept up to date and tested with more recent versions, without needing as much of a commitment on our end.

...but this is a tangent and I'll politely return to figuring out where to best propose this :)

[callahad]

We've dropped Buildkite and switched up GHA to run on all pull requests in #240, so I've taken the liberty of merging main, removing the extraneous changelog file, and pushing to Shay's branch.

[reivilibre]

SGTM

[callahad]

...and in the interest of getting CI green, I went ahead and fixed the errors 😆

[clokep]

I think that this will now break with old versions of Twisted, which is why we quoted all of the types in the Synapse PR.

[callahad]

@clokep Tests appear to pass locally with 19.7.0, and we do exercise the twisted_sleep function.

I could throw in a from __future__ import annotations if you'd like? (Sygnal requires Py3.7+, so that is available). What failure mode are you worried about?

[clokep]

@clokep Tests appear to pass locally with 19.7.0, and we do exercise the twisted_sleep function.

What failure more are you worried about?

@richvdh and I had a conversation about it in #synapse-dev:matrix.org: https://matrix.to/#/!XaqDhxuTIlvldquJaV:matrix.org/$mpaHhwF-RQLR4TscGgrUHwX9W3lAGcBQMysydMSrOUw?via=matrix.org&via=vector.modular.im&via=envs.net

clokep: I forget -- what was the deal with ensuring it doesn't break old Twisted? Did we end up needing to do anything special?
richvdh: I kinda decided we didn't, but I think I might have been wrong

• richvdh investigates
  clokep: I think just quoting them might be enough btw, so that they're not evaluated except by mypy.
  richvdh: yup, looks like

Unfortunately it seems like the past CI run is gone, so I can't see exactly what failed. I can't say anything looks dramatically different between the Synapse and Sygnal PRs with the way we're using Deferreds.

I could throw in a from __future__ import annotations if you'd like? (Sygnal requires Py3.7+, so that is available).

Well I'd prefer we keep things consistent and solve it the same way for our different packages.

[richvdh]

For the record, if I run https://github.com/matrix-org/synapse/tree/rav/broken_deferred_annotations in a virtualenv with Twisted 21.2 (and python 3.8) I get:

Traceback (most recent call last):
  File "/usr/lib/python3.8/runpy.py", line 194, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/lib/python3.8/runpy.py", line 87, in _run_code
    exec(code, run_globals)
  File "/home/rav/work/synapse/synapse/app/homeserver.py", line 38, in <module>
    from synapse.app import _base
  File "/home/rav/work/synapse/synapse/app/_base.py", line 37, in <module>
    from synapse.app.phone_stats_home import start_phone_stats_home
  File "/home/rav/work/synapse/synapse/app/phone_stats_home.py", line 21, in <module>
    from synapse.metrics.background_process_metrics import wrap_as_background_process
  File "/home/rav/work/synapse/synapse/metrics/background_process_metrics.py", line 30, in <module>
    from synapse.util.async_helpers import maybe_awaitable
  File "/home/rav/work/synapse/synapse/util/async_helpers.py", line 476, in <module>
    deferred: defer.Deferred[_T], timeout: float, reactor: IReactorTime
TypeError: 'type' object is not subscriptable

That error is on this line: https://github.com/matrix-org/synapse/blob/rav/broken_deferred_annotations/synapse/util/async_helpers.py#L476.

[reivilibre]

I did some testing.

First of all, Twisted 19.2.1 fails because of missing things that we use in the test suite.

Upgraded to Twisted 19.7. It's fine with Deferred[None] as a variable annotation, but TypeError occurs if I put that same annotation in the signature of a method.