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

First draft of an update to the Overloads chapter (DRAFT: DO NOT MERGE) #1839

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

erictraut
Copy link
Collaborator

  • Attempts to clearly define the algorithm for overload matching.
  • Describes checks for overload consistency, overlapping overloads, and implementation consistency.

erictraut and others added 2 commits August 13, 2024 17:06
* Attempts to clearly define the algorithm for overload matching.
* Describes checks for overload consistency, overlapping overloads, and implementation consistency.
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
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.

(two quick comments)

docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Show resolved Hide resolved
docs/spec/overload.rst Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Thanks for working on this tricky area! I haven't finished review yet, but may be called away soon, so I'm submitting the comments I have so far. (EDIT: I've now completed my review.)

docs/spec/overload.rst Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
docs/spec/overload.rst Outdated Show resolved Hide resolved
@erictraut
Copy link
Collaborator Author

erictraut commented Aug 28, 2024

We typically wait for a proposed spec change to be accepted by the TC prior to writing conformance tests. In this case, I think it's advisable to write the conformance tests prior to acceptance. This will help us validate the proposed spec changes and tell us if (and to what extent) these changes will be disruptive for existing stubs and current type checker implementations.

I would normally volunteer to write the conformance tests, but in this case I think it would be preferable for someone else to write the tests based on their reading of the spec update. If I write the tests, there's a real possibility that they will match what's in my head but not accurately reflect the letter of the spec. There's also a possibility that I'll miss some important cases in the tests. If someone else writes the tests, they can help identify holes and ambiguities in the spec language.

Is there anyone willing to volunteer to write a draft set of conformance tests for this overload functionality? I'm thinking that there should be four new test files:

  1. overloads_definitions: Tests the rules defined in the "Invalid overload definitions" section
  2. overloads_consistency: Tests the rules defined in the "Implementation consistency" section
  3. overloads_overlap: Tests the rules defined in the "Overlapping overloads" section
  4. overloads_evaluation: Tests the rules defined in the "Overload call evaluation" section

If this is more work than any one person wants to volunteer for, we could split it up.

@carljm
Copy link
Member

carljm commented Aug 28, 2024

I am willing to work on conformance tests for this, but I probably can't get to it until the core dev sprint, Sept 23-27. I realize that implies a delay to moving forward with this PR. Happy for someone else to get to it first.

Comment on lines 192 to 203
to "partially overlap". If two overloads partially overlap, the return type
of the former overload should be assignable to the return type of the
latter overload. If this condition doesn't hold, it is indicative of a
programming error and should be reported by type checkers. The purpose of
this check is to prevent unsoundness of this form::

@overload
def is_one(x: Literal[0]) -> Literal[True]: ...
@overload
def is_one(x: int) -> Literal[False]: ...

reveal_type(is_one(int(1))) # Reveals Literal[False], but True at runtime
Copy link

@samwgoldman samwgoldman Sep 26, 2024

Choose a reason for hiding this comment

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

This unsoundness does not only constrain returns, but also other arguments. For example, let's consider PEP 728's __extra_items__.

For example, given this example from the PEP:

class Movie(TypedDict, closed=True):
    name: str
    __extra_items__: bool

we could imagine what the __setitem__ signature would be as an overload:

  @overload
  def __setitem__(self, key: Literal['name'], value: str) -> None: ...
  @overload
  def __setitem__(self, key: string, value: bool) -> None: ...

Where the unsoundness is exposed in a similar way as quoted, by exploiting Literal['name'] <: str.

key: str = 'name'
movie[key] = false
oops: str = movie['name']

Now, maybe this case would fall under the exemption below, as an exempted magic method. However, would we consider specifying that user-defined overloads constrain the arguments, like we have done for returns?

Copy link
Member

Choose a reason for hiding this comment

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

I think in your example you wrote movie[str] = where you meant movie[key] =?

(Still thinking about the underlying issue.)

Choose a reason for hiding this comment

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

Thanks! Updated the example.

Copy link
Member

Choose a reason for hiding this comment

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

I think for one thing, this suggests that the following line in PEP 728 requires more consideration:

Different from index signatures, the types of the known items do not need to be assignable to the extra_items argument.

There is a soundness reason for this restriction in index signatures, and perhaps we should re-consider whether it is advisable to discard this restriction in PEP 728.

I agree that this is also a soundness issue for user-defined overloads in general. I think in order to close this hole, we would need to specify that if any argument type overlaps between two overloads, then the latter overload must accept all calls that would be accepted by the earlier overload. Does that seem right to you?

I think we would need some empirical testing to understand what the impact of such a specification would be on real-world overloads. It might not be too bad. But I suspect this should be handled as a separate change to the spec, since I think it would be a significant change to current type-checker behavior, whereas I think this PR mostly intends to clarify and specify existing behavior.

Comment on lines +282 to +289
Type expansion should be performed one argument at a time from left to
right. Each expansion results in sets of effective argument types.
For example, if there are two arguments whose types evaluate to
``int | str`` and ``int | bytes``, expanding the first argument type
results in two sets of argument types: ``(int, int | bytes)`` and
``(str, int | bytes)``. If type expansion for the second argument is required,
four sets of argument types are produced: ``(int, int)``, ``(int, bytes)``,
``(str, int)``, and ``(str, bytes)``.
Copy link

@alwaysmpe alwaysmpe Nov 19, 2024

Choose a reason for hiding this comment

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

I'd like to suggest adding that when a function is an overload implementation then some combinations may be further eliminated based on the overload definitions. This makes it easier to call one overloaded function from the implementation of another overloaded function with compatible type arguments. For example:

from typing import overload

@overload
def take_varg(*arg: int) -> int:
    ...
@overload
def take_varg(*arg: str) -> str:
    ...

@overload
def take_varg2(*arg: int) -> int:
    ...
@overload
def take_varg2(*arg: str) -> str:
    ...
def take_varg2[T: (int, str)](*arg: T) -> T:
    return take_varg(*arg)

@overload
def take_pair(arg: int, arg2: int) -> int:
    ...
@overload
def take_pair(arg: str, arg2: str) -> str:
    ...
@overload
def take_pair2(arg: int, arg2: int) -> int:
    ...
@overload
def take_pair2(arg: str, arg2: str) -> str:
    ...
def take_pair2[T: (int, str)](arg: T, arg2: T) -> T:
    return take_pair(arg, arg2)

The above currently type checks in mypy but not pyright. Even so, more complicated signatures wouldn't. I've used "value-constrained type variables" which based on a related discussion on pyright repo "are not well specified in the Python typing spec", and you recommend against using them, but I couldn't find any better way to type this.

@carljm
Copy link
Member

carljm commented Nov 22, 2024

I am willing to work on conformance tests for this

Just wanted to note that I haven't forgotten this, it remains on my TODO list, I just wasn't able to get to it during the core dev sprint and haven't been able to prioritize it quite yet :/ But I still plan to do it. I also remain open to anyone else interested picking it up, just comment here first :)

docs/spec/overload.rst Outdated Show resolved Hide resolved
Copy link

@Enegg Enegg left a comment

Choose a reason for hiding this comment

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

Minor typo

docs/spec/overload.rst Outdated Show resolved Hide resolved
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.