-
Notifications
You must be signed in to change notification settings - Fork 73
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
type Distribution.get_command_obj
to not return None
with create=True
#307
Conversation
16433c1
to
83310f6
Compare
distutils/dist.py
Outdated
@overload | ||
def get_command_obj(self, command: str, create: Literal[False]) -> Command | None: ... |
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.
Is this overload needed? I don't see how it adds any useful information.
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.
Without overloads, false-positives (before):
distribution: Distribution = ...
some_command = distribution.get_command_obj("some_command")
# Mypy: Revealed type is "distutils.cmd.Command | None"
# Pyright: Type of "some_command" is "Command | None"
reveal_type(some_command)
# Mypy: Item "None" of "Command | None" has no attribute "some_command_create_true" (union-attr)
# Pyright: "some_command_create_true" is not a known attribute of "None" (reportOptionalMemberAccess)
some_command_create_true.some_command_create_true
With overloads (this PR):
distribution: Distribution = ...
some_command = distribution.get_command_obj("some_command")
# Mypy: Revealed type is "distutils.cmd.Command"
# Pyright: Type of "some_command" is "Command"
reveal_type(some_command)
some_command.sub_commands # OK
In both cases, create=False
doesn't change
# True-positive
some_command = distribution.get_command_obj("some_command", False)
# Mypy: Revealed type is "distutils.cmd.Command | None"
# Pyright: Type of "some_command" is "Command | None"
reveal_type(some_command)
# Mypy: Item "None" of "Command | None" has no attribute "sub_commands " (union-attr)
# Pyright: "sub_commands " is not a known attribute of "None" (reportOptionalMemberAccess)
some_command.sub_commands
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 don't think that answered my question. So I fumbled around until I was able to somewhat replicate your examples above. I created foo.py:
import distutils.dist
distribution: distutils.dist.Distribution = ...
some_command = distribution.get_command_obj("some_command", False)
# Mypy: Revealed type is "distutils.cmd.Command"
# Pyright: Type of "some_command" is "Command"
reveal_type(some_command)
some_command.sub_commands # OK
And I ran mypy foo.py
with this copy of distutils, and indeed I got:
foo.py:7: note: Revealed type is "Union[distutils.cmd.Command, None]"
Then I removed this overload and ran it again, and got a different result:
foo.py:4: error: Argument 2 to "get_command_obj" of "Distribution" has incompatible type "Literal[False]"; expected "Literal[True]" [arg-type]
foo.py:7: note: Revealed type is "distutils.cmd.Command"
That's interesting. I would have expected the third definition, the one with create: bool = True
to fulfill the Literal[False]
case, instead of having to overload it with exactly the same signature. Alas, it seems our type system isn't so elegant.
Any idea if that's an issue being tracked by the type checking ecosystem?
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 would have expected the third definition
There's only two overload definitions.
The "third" is the implementation. That is (and should) be completely ignored by type-checkers when using that method. Annotating the implementation (generally as a combination of all possible overloads) is only useful to type-check the content of the function.
f5b6dc2
to
cef37dd
Compare
cef37dd
to
deb1d5a
Compare
This mirrors the typeshed change python/typeshed#11950
This is only visible for distutils itself and setuptools in pypa/setuptools#4704