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

Type-annotate all function arguments and internal variables #8351

Closed
7 of 11 tasks
ShadowJonathan opened this issue Sep 18, 2020 · 7 comments
Closed
7 of 11 tasks

Type-annotate all function arguments and internal variables #8351

ShadowJonathan opened this issue Sep 18, 2020 · 7 comments
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.

Comments

@ShadowJonathan
Copy link
Contributor

ShadowJonathan commented Sep 18, 2020

Description:

Related to #8347, (partially) blocked till 3.5 is deprecated

The goal is to;

  • Convert existing name = ... # type: comments to name: type = .... (see "com2ann progress" below)
  • Put type annotations on every function's parameters and return type.
  • Put name: type annotations on;
    • Every class variable.
    • Every instance variable (declared in __init__() functions), unless;
      • They're constructing another instance (self.thing = Thing()) (dubious, see Use inline type hints in handlers/ and rest/ #10382 (comment))
      • They're getting assigned a non-container literal (1, b"", "", etc. but not None)
      • The assignment comes from a function/method call (self.thing = hs.get_another_thing()), given that the function signature is known and inferrable.

com2ann progress

(com2ann changes comment type annotations to inline type annotations, name = ... # type: to name: type = ..., this is possible since python 3.6, and standard across the python ecosystem.)

@clokep
Copy link
Contributor

clokep commented Sep 18, 2020

This could probably finally allow a mypy linter to run with CI, to catch future inconsistencies.

mypy runs in CI on a subset of the code-base, see https://github.com/matrix-org/synapse/blob/develop/mypy.ini#L9

@clokep clokep added enhancement Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label) labels Sep 18, 2020
@clokep
Copy link
Contributor

clokep commented Sep 18, 2020

blocked till 3.5 is deprecated

This isn't blocked? Python 3.5 supports type annotations.

@ShadowJonathan
Copy link
Contributor Author

blocked till 3.5 is deprecated

This isn't blocked? Python 3.5 supports type annotations.

not variable type annotations, but you're right about it not being completely blocked

@ShadowJonathan
Copy link
Contributor Author

A large portion of this can be fixed with com2ann, see #9131 (comment)

@clokep clokep added T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. and removed z-enhancement Z-Help-Wanted We know exactly how to fix this issue, and would be grateful for any contribution z-p3 (Deprecated Label) labels Apr 5, 2021
@ShadowJonathan
Copy link
Contributor Author

Updated description, the previous one was vague and also implied setting type annotations for every variable inside non-init functions, which i think is overdoing it a little bit, type-annotating all other places noted should already correctly resolve those spots.

@ShadowJonathan
Copy link
Contributor Author

I found some interesting mypy optional errors that could be enabled to enforce type annotations;

https://mypy.readthedocs.io/en/stable/error_code_list2.html

  • Check that type arguments exist [type-arg]
    (Errors on List, requires List[<type>])

  • Check that every function has an annotation [no-untyped-def]

  • Check that comparisons are overlapping [comparison-overlap]

    def is_magic(x: bytes) -> bool:
      # Error: Non-overlapping equality check 
      #        (left operand type: "bytes", right operand type: "str")  [comparison-overlap]
      return x == 'magic'
  • (maybe) Check that no untyped functions are called [no-untyped-call]

  • Check that function does not return Any value [no-any-return]

@clokep
Copy link
Contributor

clokep commented Aug 15, 2022

I think this is essentially a duplicate of #11271.

@clokep clokep closed this as completed Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

No branches or pull requests

2 participants