Skip to content

Add additional examples about TypeIs #1814

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mikeshardmind
Copy link

This is a followup from discussion on discourse, and specifically this example which was constructed with the help of many going back and forth to determine a situation that could have a parallel in real world code, and which was possibly unsound. To not overly deter users from using this when it is appropriate, counter examples of "tolerably safe" narrowing functions that operate on generic types are included.

@mikeshardmind mikeshardmind force-pushed the typeis-added-guidance branch from c43be83 to d3bb2d2 Compare July 20, 2024 22:36
@srittau srittau added Typing Council decision Needs to be approved by the Typing Council. Do not merge until approved. topic: typing spec For improving the typing spec labels Jul 21, 2024
@mikeshardmind
Copy link
Author

I'm not sure why the typing council decision label was added to this, this is purely an extended example of existing behavior to help better document the sharp edges that already exist on a typing feature and not a specification change, which does not appear to need one.

Quoting: https://github.com/python/typing-council/blob/main/README.md

The Council makes decisions in response to issues opened on this repo. Such decisions include:

If there's anything I need to do to have this proceed, please let me know.

@JelleZijlstra
Copy link
Member

This PR is to the spec, but I'm not sure why this text needs to be in the spec; it doesn't seem to specify any additional behavior. It might be a better fit for the guide on type narrowing that I added recently.

@erictraut
Copy link
Collaborator

Reviewing the proposed addition, I don't think this belongs in the typing spec. The typing spec is intended to specify how the type system works and how type checkers must behave. The proposed addition doesn't serve this purpose. It is instead directed at users of TypeIs. It therefore belongs in the typing guide, not the typing spec.

@mikeshardmind
Copy link
Author

Hmm. I thought placing it next to the existing example of behavior around TypeIs would make more sense, but I can retool the PR to move it to the guide, thanks for the feedback.

@mikeshardmind mikeshardmind force-pushed the typeis-added-guidance branch from 2b8c1f2 to 3b1983e Compare August 5, 2024 17:00
@srittau srittau removed Typing Council decision Needs to be approved by the Typing Council. Do not merge until approved. topic: typing spec For improving the typing spec labels Aug 5, 2024
@srittau srittau added the topic: documentation Documentation-related issues and PRs label Mar 17, 2025
@srittau srittau requested a review from JelleZijlstra March 17, 2025 10:38
@@ -195,15 +195,91 @@ This behavior can be seen in the following example::
else:
reveal_type(x) # Unrelated

There are also cases beyond just mutability. In some cases, it may not be
Copy link
Member

Choose a reason for hiding this comment

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

This fits better under "Safety and soundness" below, probably as a new subheader.

The example could be made more convincing; I think it's not going to be clear to most readers why it's unsafe.

I think something like this would demonstrate it:


def takes_any_a(a: A[int | str]):
    if possible_problem(a):
        assert_type(a, A[int])  # OK because A[int] is a subtype of A[int | str]
        if isinstance(a, B):
            assert_type(a, B[int])  # A[int] & B -> B[int]
            print(b.j + 1)  # boom
takes_any_a(B(i=1, j="x"))

attempt to limit type narrowing in a way that minimizes unsafety while remaining
useful, but not all safety violations can be detected.

One example of this tradeoff building off of TypeIs
Copy link
Member

Choose a reason for hiding this comment

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

This also fits better as a new subparagraph instead of as part of the header

return all(isinstance(i, int) for i in s)

However, many cases of this sort can be extracted for safe use with an
alternative construction if soundness is of a high priority,
Copy link
Member

Choose a reason for hiding this comment

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

Note this has other tradeoffs, e.g. potentially higher memory usage. For example, range(1000) is a Sequence, turning it into the equivalent tuple takes a lot more memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: documentation Documentation-related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants