-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Don't duplicate ParamSpec prefixes and properly substitute Paramspecs #14677
Conversation
…tter I ran into the duplication issue personally, but this also fixes python#12734
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
FYI, while looking through issues to see if this PR fixes anything else, I found that #14169 is untagged (it should have the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I have a few questions.
old_prefix = param_spec.prefix | ||
|
||
# Check assumptions. I'm not sure what order to switch these: | ||
assert not old_prefix.arg_types or not prefix.arg_types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit suspicious to me. Why can't both the arg types truthy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found a case where they were both truthy and yet not equal to each other yet -- I don't know what order to mix their arguments together. My gut says what I wrote next is correct but I would rather have an explicit test case (or be shown that this never happens)
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?
* 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
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Just a note that this exposes a bug in the constraint finder around paramspec: I've got a local branch that fixes that. (I don't think I can have a PR depend on another?) PS This is ready for another review :D |
I have a question about implementation and I'm not sure where to ask it (is there a better spot?), so here goes! Hopefully people who know the implementation of mypy check here :P
Essentially, the goal of this PR (and the followup PR which ATM is just over at https://github.com/A5rocks/mypy/tree/fix-fundamental-paramspecs) is to get this following code to typecheck: from typing import ParamSpec, Concatenate, Generic, TypeVar, Any
from collections.abc import Callable
P = ParamSpec("P")
T = TypeVar("T")
V = TypeVar("V")
R = TypeVar("R")
# ----- INSIDE BLOOM -----
def command_builder() -> Callable[[Callable[Concatenate[T, P], R]], Callable[P, Callable[[T], R]]]:
def transformer(f: Callable[Concatenate[T, P], R], /) -> Callable[P, Callable[[T], R]]:
def returned(*args: P.args, **kwargs: P.kwargs) -> Callable[[T], R]:
def returned_transformer(z: T) -> R:
return f(z, *args, **kwargs)
return returned_transformer
return returned
return transformer
# ----- TEST CASES -----
class Example(Generic[P]):
pass
@command_builder()
def test1(ex: Example[P], /) -> Example[Concatenate[int, P]]:
...
reveal_type(test1) # Is OK... But def () -> def [P] (repro.Example[P`-1]) -> repro.Example[[int, **P`-1]]
reveal_type(test1()) # Should be def [P](?) (repro.Example[P`-1]) -> repro.Example[[int, **P`-1]] ATM on that branch mypy spits out:
Obviously that last thing is wrong. (It has bad consequences for actual usage but even just the revealed type is wrong). The problem here is that I am calling a "def [P] () -> def (repro.Example[P`-1]) -> repro.Example[[builtins.int, **P`-1]]", which in turn tries to infer Basically, how should I solve this? My main idea revolves about making solving for constraints check if that type var appears in the arguments and if it doesn't then it gets "assigned" to the return type (if that's possible, which is only with stuff like callables). However, I'm not sure how to even check whether a type var appears in the arguments, let alone if there's a better way to approach this. Update: I managed to in A5rocks@8642f3d |
Hello again! Another review would be much appreciated -- the followup PR fixes a few open issues and I can't open that until this is merged (as far as I know?) without messing up commit history for myself (?). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me and am excited to see your future ParamSpec improvements. I'll wait a day or two in case Jukka wants to take another look; if I don't merge soon, just ping me :-)
Thank you! |
I ran into the duplication issue personally, but this also fixes #12734
Also: