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

add overload to tuple.__new__ to better express an empty tuple #7454

Merged
merged 4 commits into from
Jul 6, 2022
Merged

add overload to tuple.__new__ to better express an empty tuple #7454

merged 4 commits into from
Jul 6, 2022

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Mar 7, 2022

This PR adds an overload to the tuple() constructor to specify that an empty tuple is returned, not Tuple[Any]. It covers the following code snippet (using the Pylance type checker):

from typing import Tuple
Point3D = Tuple[int, int, int]

t1: Point3D = 1, 2, 3  # correct no error
t2: Point3D = 1, 2, 3, 4  # correct error; Element size mismatch; expected 3 but received 4
t3: Point3D = () # correct error; Element size mismatch; expected 3 but received 0
t4: Point3D = tuple() # expected error, none thrown

The docs specify that:

If no argument is given, the constructor creates a new empty tuple, ().

This is my first typeshed PR, so please feel free to correct any mistakes I've made!


For more context, see: microsoft/pylance-release#2445 (comment)

@xavdid
Copy link
Contributor Author

xavdid commented Mar 7, 2022

Ah, looks like CI is unhappy:

stdlib/builtins.pyi:788: error: Overloaded function signatures 1 and 2 overlap with incompatible return types [misc]

I see that most __new__ functions take cls: type[Self] as their first arg, but Self isn't subscriptable, so I wasn't sure how to specify that the return was Tuple[()] and not the default Tuple[Any]. Guidance here is appreciated 😅

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Mar 8, 2022

It's unclear to me way mypy primer explodes, but I think it's related to isinstance checks, i.e. mypy thinks that isinstance(x, tuple) means that x is an empty tuple. Could you try reordering the overloads (and add a comment)?

@xavdid
Copy link
Contributor Author

xavdid commented Mar 8, 2022

Sure thing! added in d556d17; hope that comment is descriptive enough- I don't quite follow the issue.

@github-actions

This comment has been minimized.

@xavdid
Copy link
Contributor Author

xavdid commented Mar 8, 2022

Ah, I think I'm following now. Using tuple as a class not the result of an instantiation is the issue.

tuple should return Self, but tuple() should return Tuple[()]. I'm not sure how that's expressed in the interfaces correctly.

@srittau
Copy link
Collaborator

srittau commented Mar 8, 2022

I'm not totally sure what's going on here myself. primer output looks much better, although it's weird that the remaining problems are in mypy itself. Maybe a mypy developer could look over this? Cc @JukkaL @hauntsaninja

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Mar 8, 2022

I'm not sure either, will look into it soon.

I think the first error on config_files = tuple(map(os.path.expanduser, defaults.CONFIG_FILES)) is not specific to mypy's codebase and is a genuine false positive. The other two errors involve a custom mypy plugin that mypy uses on the mypy codebase; it's possible there's a bug in that plugin, but I'd need to investigate more to know what's going on.

@xavdid
Copy link
Contributor Author

xavdid commented Mar 8, 2022

Thanks for taking a look! Let me know if there's anything else I can do to be helpful.

hauntsaninja pushed a commit to hauntsaninja/mypy that referenced this pull request Mar 9, 2022
@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Mar 9, 2022

Here, python/mypy#12319 will fix the errors from the plugin.

I unfortunately don't have a fix for the errors from config_files = tuple(map(os.path.expanduser, defaults.CONFIG_FILES)), but that's life.

I also filed python/mypy#12320 since it looks like a similar issue could crop up in other parts of mypy's codebase.

JukkaL pushed a commit to python/mypy that referenced this pull request Mar 9, 2022
This will be broken by python/typeshed#7454

Co-authored-by: hauntsaninja <>
@JelleZijlstra
Copy link
Member

I'm not sure this change is worth it if we lose correct typing for subclasses of tuple. After class X(tuple), what does reveal_type(X()) do?

@AlexWaygood
Copy link
Member

I'm not sure this change is worth it if we lose correct typing for subclasses of tuple. After class X(tuple), what does reveal_type(X()) do?

Empty tuples and tuple subclasses are both reasonably common, but I can't say I've ever seen an example of an empty tuple subclass instance. And for non-empty tuple subclass instances, we'll still have correct type inference, as that overload still returns Self. So I think it will probably be okay?

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 3, 2022

Seems like this change might also fix

@github-actions
Copy link
Contributor

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

mypy (https://github.com/python/mypy)
+ mypy/config_parser.py:182: error: Argument 1 to "map" has incompatible type overloaded function; expected "Callable[[str], AnyStr]"  [arg-type]

@AlexWaygood
Copy link
Member

I'm not sure this change is worth it if we lose correct typing for subclasses of tuple. After class X(tuple), what does reveal_type(X()) do?

For both master and this PR, if I do mypy --custom-typeshed-dir on the following snippet, mypy yields the following result:

# test.py
class X(tuple): ...
reveal_type(X())  # Revealed type is "test.X"

So this doesn't seem to cause any regressions in that regard.

@AlexWaygood AlexWaygood requested a review from JelleZijlstra July 3, 2022 14:11
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.

5 participants