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

Reconsider constraints involving parameter specifications #15272

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

@A5rocks A5rocks marked this pull request as ready for review May 19, 2023 22:14
@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor Author

A5rocks commented May 20, 2023

Mypy primer diff looks right!

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

one quick comment

mypy/constraints.py Show resolved Hide resolved
@github-actions

This comment has been minimized.

@A5rocks
Copy link
Contributor Author

A5rocks commented Jul 7, 2023

Fake error

Decently close to coming up with a reliable way to trigger this logic, but funnily enough I found a paramspec bug 👻

Just marking it here before I forget:

from typing import Generic, Callable, Union
from typing_extensions import ParamSpec, Concatenate

P = ParamSpec("P")
P1 = ParamSpec("P1")

class Tester(Generic[P1]):
    @staticmethod
    def test_something(x: Callable[P, None]) -> Tester[P]: ...  # type: ignore[empty-body]

    @staticmethod
    def test_concatenate(x: Callable[P, None]) -> Tester[Concatenate[int, P]]: ... # type: ignore[empty-body]

def func(
    action: Callable[P, None],
) -> None:
    job = Tester.test_concatenate(action) if True else Tester.test_something(action)
    reveal_type(job)

err, actually i'm decently sure that's just inference being annoying :(

@github-actions

This comment has been minimized.

@@ -79,15 +79,19 @@ def __repr__(self) -> str:
op_str = "<:"
if self.op == SUPERTYPE_OF:
op_str = ":>"
return f"{self.type_var} {op_str} {self.target}"
return f"{self.origin_type_var} {op_str} {self.target}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too sure why this was this way before and if this is entirely bad behavior. I added this (and updates to hashing and equality) because paramspec types have extra state that is important: their prefix (which Concatenate adds)

@A5rocks A5rocks requested a review from hauntsaninja July 8, 2023 01:57
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

discord.py (https://github.com/Rapptz/discord.py)
+ discord/ext/commands/core.py:1729: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]
+ discord/ext/commands/core.py:1799: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]

@A5rocks
Copy link
Contributor Author

A5rocks commented Aug 9, 2023

Hi, bumping for @hauntsaninja or maybe @JukkaL.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, looks great! Might be worth a follow up PR to see if we can deduplicate a little bit or factor some stuff out of visit_instance


# I don't think we can parametrize...
for direction in (SUPERTYPE_OF, SUBTYPE_OF):
print(f"direction is {direction}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: stray print

Copy link
Contributor Author

@A5rocks A5rocks Aug 9, 2023

Choose a reason for hiding this comment

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

Oh oops, lol.

Err wait, IIRC that's in order to make any test failures easier to check (it'll only show stdout on test failure).

@hauntsaninja hauntsaninja merged commit 2aaeda4 into python:master Aug 9, 2023
17 checks passed
@A5rocks A5rocks deleted the fix-paramspec-constraints branch August 9, 2023 06:20
@ilevkivskyi
Copy link
Member

I am going to revert this. IMO this is just a bunch of more hacks to compensate for older hacks. Current ParamSpec support is quite foundationally broken. That needs to be addressed first.

ilevkivskyi added a commit that referenced this pull request Aug 9, 2023
@A5rocks
Copy link
Contributor Author

A5rocks commented Aug 9, 2023

I do think this isn't necessarily hacky -- I basically updated the code directly based on how I expect things to work out.... but I understand that ParamSpec support as a whole is hacky so... Makes sense

(I'm still surprised that this solved any issues at all; I made this PR because things were being weird)

@ilevkivskyi
Copy link
Member

Yes, right, this doesn't really adds new hacks. I think I have an idea how to make ParamSpec a bit more principled. I am going to include some part of cleanups in coming inference PR (just because I was touching related code). Then I will make a separate second PR. I see you are interested in this topic. I will keep you posted on Discord.

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