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

PEP 705: Remove the requirement for bidirectional type inference #8

Closed
wants to merge 1 commit into from

Conversation

alicederyn
Copy link
Owner

@alicederyn alicederyn commented Oct 22, 2023

Possible resoluion to python#3504 (comment).

@erictraut @JelleZijlstra

  • The paragraphs on error messages being explicit about type-checker limitations versus soundness may be controversial. However, I think it is important to be up-front about such issues, as users are likely to spend time attempting to understand why their code is wrong, then get frustrated when it turns out to be valid but unsupported. I believe it should be relatively simple for a type checker to detect when errors fall into this category, by tracking synthesised TypedDicts and changing the error message emitted when a
    consistency check fails.
  • I added an "Open Questions" section. I actually meant to do this in the previous draft to explicitly call out other_keys as an open question, rather than using "Rejected Alternatives", but it slipped my mind during the drafting process.

📚 Documentation preview 📚: https://pep-previews--8.org.readthedocs.build/

@@ -363,6 +365,35 @@ Notes:
b: B = { "x": 3 }
c: C = a | b # Accepted by type checker: "x" will always come from b

Minimum support

Choose a reason for hiding this comment

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

I think the minimum support should be to "enforce the behaviors indicated in typeshed". Anything beyond that requires a type checker to override the typeshed stubs, which is something I'm very reluctant to do in pyright unless there's really strong reason to do otherwise. I've made exceptions in only a couple of places so far, and those have been ongoing maintenance burdens.

I've received one or two requests for improving the | operator for TypedDict, but that's nowhere near enough to compel me to add special-case behavior. I'd need to receive dozens of more requests before considering it. As for copy and deepcopy, I've never had a request for improving these for TypedDict operands, so I'm not be inclined to add such support.

I'm curious why you've focused on |, copy and deepcopy. Have you personally experienced issues with these? Or are you trying to solve a problem that is completely theoretical here?

The motivation section doesn't mention |, copy or deepcopy as being problems for TypedDict. If these are going to be a focus on this PEP, I think they should feature prominently in the motivation and/or rationale sections. Right now, these seem to come out of nowhere.

What you've outlined below seems like a good sketch of how a type checker could override the standard typeshed stubs if desired, but I'd prefer that it be completely optional and left up to each type checker to decide whether to do so. I'm not sure this even belongs in a PEP.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Re: merge. This was more important when other_keys was in the spec. I think I'm happy to defer.

Re: copy/deepcopy: I think the typeshed stubs will be functionally deficient for typeddicts with read-only entries, and users are likely to hit it and complain. Without these changes, AFAIK there's no way to copy a readonly typeddict and change an item in it without an unsafe cast? This is a new problem so it's not a surprise if it's not come up before.

I may be missing subtleties here though. For instance, are you only talking about copy.copy and not TypedDict.copy?

Choose a reason for hiding this comment

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

This is a new problem so it's not a surprise if it's not come up before.

Then I'd prefer to address it if and when pyright users run into the issue, not be required to do it speculatively because the PEP mandates it.

I don't think there's any precedent for a typing PEP to mandate that a type checker provide custom logic that overrides typeshed stubs. Not even the PEP 557 (dataclass) or PEP 589 (TypedDict) makes any such statement, although PEP 589 does come close (in that it specifies cases where the get and clear methods in this section.

Perhaps it would be best if PEP 705 stuck to defining the meaning of ReadOnly and how this affects type consistency rules. Individual type checkers can then choose how to implement support for it.

are you only talking about copy.copy and not TypedDict.copy?

I'm talking about both, but I'm more concerned about copy.copy and copy.deepcopy. The TypedDict.copy method doesn't bother me as much because TypedDict classes and methods are already special-cased by type checkers. The copy.copy and copy.deepcopy methods are very general functions. I suspect they are rarely used in conjunction with TypedDict, especially when there are straightforward alternatives including TypedDict.copy and {**td}.

Copy link
Owner Author

@alicederyn alicederyn Oct 22, 2023

Choose a reason for hiding this comment

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

I don't think there's any precedent for a typing PEP to mandate that a type checker provide custom logic that overrides typeshed stubs.

I've been missing the obvious: you're telling me I need to target this kind of change at the typeshed stubs, not the type checkers.

straightforward alternatives including...{**td}.

I just checked and mypy out-of-the box appears to support all the rules I specified if you write:

c: C = { **a, **b }

Yet the functionally identical code with | is rejected. The same issue holds for copy.

I am surprised that the status quo discourages users away from idiomatic and readable modern Python onto the spread operator, but you're quite right, this does make all of this technically unnecessary. (Except maybe deepcopy, which has no analogue.)

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, there are some active efforts to improve | support in mypy: python/mypy#16249


A type checker may reject sound assignments, for instance if it is unable to support bidirectional type inference. This section provides a minimum standard of support that a type checker should provide, so users can write code that will be consistently accepted across all type checkers.

Type checkers may reject any code not conforming to this model, but the error should be explicit that this is a type checker limitation, and that the user code is not necessarily unsound. It may be reasonable to use a distinct code for suppressing such issues, so if the checker later supports full inference and the code is indeed unsound, the issue will no longer be suppressed.

Choose a reason for hiding this comment

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

I don't think you should go into advice about error messages or "distinct codes". That's getting too deep into type checker behaviors and isn't something a PEP should get into.


Notes:

* For any TypedDict ``D`` with non-read-only items, the type constructed when merging two instances of type ``D`` will be equivalent to ``D``, and the type checker may reuse ``D`` internally.

Choose a reason for hiding this comment

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

and the type checker may reuse D internally

This is getting into implementation details of a type checker. Recommend deleting this phrase.


A type checker may reject sound assignments, for instance if it is unable to support bidirectional type inference. This section provides a minimum standard of support that a type checker should provide, so users can write code that will be consistently accepted across all type checkers.

Type checkers may reject any code not conforming to this model, but the error should be explicit that this is a type checker limitation, and that the user code is not necessarily unsound. It may be reasonable to use a distinct code for suppressing such issues, so if the checker later supports full inference and the code is indeed unsound, the issue will no longer be suppressed.

Choose a reason for hiding this comment

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

Here again, I don't think you should talk about error messages or error codes.


Notes:

* For any TypedDict ``D`` with non-read-only items, the type constructed when creating a shallow or deep copy of ``D`` will be equivalent to ``D``, and the type checker may reuse ``D`` internally.

Choose a reason for hiding this comment

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

Here again, I recommend deleting "and the type checker many reuse..." because this is an internal implementation detail.

@alicederyn alicederyn closed this Oct 22, 2023
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.

3 participants