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

Make Counter generic over the value #11632

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Mar 20, 2024

Closes: #3438

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.



# Test the constructor
# assert_type(Counter(), Counter[Never, int]) # pyright derives "Unknown" instead of "Never"
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth I think that's better than Never. We could test the value type with something like:

for value in Counter().values:
    assert_type(value, int)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, this makes pyright unhappy as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

assert_type(Counter(), "Counter[Any, int]") works.


custom_c: Counter[str, Foo] = Counter()
assert_type(custom_c, "Counter[str, Foo]")
assert_type(custom_c["a"], Foo)
Copy link
Member

Choose a reason for hiding this comment

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

At runtime this is actually an int though. I wonder if we need to make all these methods return _C | int.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line should probably not be accepted, as Counter() is a Counter[unknown, int], which is incompatible with Counter[..., Foo]. I'm not sure the test makes much sense.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this sort of problem apply to any Counter with a non-int value type, though? This seems like a fundamental problem with this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

At runtime this is actually an int though. I wonder if we need to make all these methods return _C | int.

I wonder whether type checkers support __missing__, in which case, this should happen automatically when we add it to the stubs. But returning _C | int makes some sense to me for getter methods.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

1 similar comment
Copy link
Contributor

github-actions bot commented Apr 9, 2024

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau
Copy link
Collaborator Author

srittau commented Apr 9, 2024

@erictraut: I'm unsure why pyright fails the test case as written: https://github.com/python/typeshed/actions/runs/8613668710/job/23605460935?pr=11632

custom_c = cast("Counter[str, Foo]", Counter())
# [...]
custom_c["a"] += 42  # type: ignore

pyright claims that the type ignore is unnecessary here, but unless I'm missing something, both __getitem__() and __setitem()__ are annotated to require a Foo instance, not int in this case.

@erictraut
Copy link
Contributor

I'm not able to repro the problem when I copy and paste your modified definition of Counter along with the minimal test case snippet. When I do that, pyright generates an error for custom_c["a"] += 42 as I would expect. So there must be something else going on with your test if you're not seeing this error.

@srittau
Copy link
Collaborator Author

srittau commented Apr 9, 2024

I remember that this is at least the second time, where a pyright "bug" can't be reproduced outside our CI environment. Something strange is going on with our CI it seems.

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

Successfully merging this pull request may close these issues.

Counter() support non-int
3 participants