Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest making protocol attribute read-only if invariance causes conflict #6002

Open
JukkaL opened this issue Dec 4, 2018 · 7 comments
Open

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 4, 2018

If a concrete attribute has a narrower type than protocol attribute, mypy will complain (adapted from #5998):

prog.py:59: error: Argument 1 to "f" has incompatible type "C"; expected "Proto"
prog.py:59: note: Following member(s) of "C" have conflicts:
prog.py:59: note:     name: expected "Optional[str]", got "str"

It could help if mypy would suggest making the protocol attribute read-only (property or final attribute), which is often the right thing to do.

We already generate a related suggestion to use a covariant collection type when using an invariant collection causes type incompatibility.

@ilevkivskyi
Copy link
Member

We can also add a link to the docs (again, similarly to the invariant collection note).

@petergaultney
Copy link

petergaultney commented Nov 6, 2019

What are the chances that there will someday be a less verbose way of declaring a Protocol attribute read-only?

moving from

class User(Protocol):
    email: str

to

class User(Protocol):
    @property
    def email(self) -> str: ...

is quite painful in terms of readability, especially when you've got 10 or so attributes that should all be considered read-only.

@ilevkivskyi
Copy link
Member

What are the chances that there will someday be a less verbose way of declaring a Protocol attribute read-only?

I would say chances are low, but theoretically non-zero.

@JelleZijlstra
Copy link
Member

Maybe we could support syntax like:

class User(Protocol):
    email: Final[str]

@petergaultney
Copy link

I would love that. I tried it but of course Protocols don't support it currently.

I assume the hesitation there is around the fact that Final in its other contexts means that the value is assigned at class/module definition time, rather than at the time of object instantiation?

@ilevkivskyi
Copy link
Member

I assume the hesitation there is around the fact that Final in its other contexts means that the value is assigned at class/module definition time, rather than at the time of object instantiation?

More because being final and being read-only are different concepts: a read-only attribute can be overridden in a subclass, a final one can't. Final is a stronger concept.

The notion of being final is in fact already quite complex (and has subtle differences in different languages), I don't want to overload it even more.

DMRobertson pushed a commit to matrix-org/python-canonicaljson that referenced this issue Feb 15, 2023
This is suggested/recommended at
python/mypy#6002

Discovered as part of matrix-org/synapse#15052
DMRobertson added a commit to matrix-org/python-canonicaljson that referenced this issue Feb 15, 2023
* Use a property to define `Jsonlibrary`

This is suggested/recommended at
python/mypy#6002

Discovered as part of matrix-org/synapse#15052

* Okay fine, coverage

* Fix the JsonLibrary protocol

This is fiddlier than I expected.

* Test under mypy 1.0

* Include mypy in default `tox` jobs
DMRobertson pushed a commit to matrix-org/synapse that referenced this issue Feb 16, 2023
* 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
@jace
Copy link

jace commented Dec 27, 2023

Maybe we could support syntax like:

class User(Protocol):
    email: Final[str]

Looks like this works in Pyright. It'll be nice to have this in Mypy too (or a different keyword to indicate read-only in Protocol).

The @property decorator is not optimal because it doesn't allow class-level access. Pyright considers it an error, while Mypy types it as property.fget — an unbound method. This is a problem when the protocol member is a descriptor with distinct behaviour for class-level access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants