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

Update after Erik's feedback #1

Merged
merged 21 commits into from
Aug 4, 2023

Conversation

rchiodo
Copy link
Owner

@rchiodo rchiodo commented Aug 2, 2023

Hope this works. Please feel free to comment.

@rchiodo rchiodo requested a review from debonte August 3, 2023 20:14
pep-0722.rst Show resolved Hide resolved
pep-0722.rst Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated
Comment on lines 33 to 40
.. code-block:: python

[Again, more context is needed for the user to understand what "the negative case" means.]
[Also what does "determine the negative case" mean? Maybe something like "narrow the type in the negative case" would be more clear? Also see the use of that phrase below the code block.]
[python should be capitalized]
def is_str(val: str | int) -> TypeGuard[str]:
return isinstance(val, str)

def func(val: str | int):
if is_str(val):
# Type checkers can assume val is a 'str' in this branch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this first code block could be removed since the second one is identical except for adding the else block?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it should be there cause I didn't want to introduce the else case until after the user had seen the if case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to make sure we're talking about the same thing. I was envisioning the following. And maybe to mention in the if block comment that the "val is a str" behavior is already defined by PEP 647.

`TypeGuards` are used throughout Python libraries to allow a
type checker to narrow the type of something when the ``TypeGuard``
returns true. However, in the ``else`` clause, :pep:`647` didn't prescribe
what the type might be:
    def is_str(val: str | int) -> TypeGuard[str]:
        return isinstance(val, str)

    def func(val: str | int):
        if is_str(val):
            # Type checkers can assume val is a 'str' in this branch
        else:
            # Type here is not narrowed. It is still 'str | int' 

pep-0722.rst Outdated Show resolved Hide resolved

def func(val: int | str):
if is_positive_int(val):
# Type checker assumes PosInt here
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type checker assumes PosInt here

This is true even though val is int | str (i.e. it doesn't include PosInt)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes that's true. This works because PosInt is an int.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You should be able to try it yourself in the latest pylance.

Create a pyrightconfig.json with this in it:

{
    "enableExperimentalFeatures" : true
}

Then the TypeGuard will behave as I described here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that that's how Pyright works. I guess I'm wondering:

a) Is this correct?

b) What would be the behavior if PosInt was unrelated to any of the types in the val union? Consider the following example. Within the if block, is val considered to be datetime because is_datetime returned true? Or is it Never because the intersection of datetime and int | str is empty? Oh, I guess this might be the multiple inheritance thing again. Maybe the val passed into func is derived from both int and datetime?

def is_datetime(val: datetime | int | str) -> TypeGuard[datetime]:
    return isinstance(val, datetime)

def func(val: int | str):
    if is_datetime(val):
        # Is val a datetime here or Never?

c) Should the PEP call more attention to these issues?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I believe the TypeGuard pep says that if the TypeGuard returns something nonsensical, that's the user's fault. It will just happily presume that's the actual type if the TypeGuard returns true.

This is exactly what Pyright does at the moment.

Copy link
Owner Author

Choose a reason for hiding this comment

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

pep-0722.rst Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated
Comment on lines 33 to 40
.. code-block:: python

[Again, more context is needed for the user to understand what "the negative case" means.]
[Also what does "determine the negative case" mean? Maybe something like "narrow the type in the negative case" would be more clear? Also see the use of that phrase below the code block.]
[python should be capitalized]
def is_str(val: str | int) -> TypeGuard[str]:
return isinstance(val, str)

def func(val: str | int):
if is_str(val):
# Type checkers can assume val is a 'str' in this branch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to make sure we're talking about the same thing. I was envisioning the following. And maybe to mention in the if block comment that the "val is a str" behavior is already defined by PEP 647.

`TypeGuards` are used throughout Python libraries to allow a
type checker to narrow the type of something when the ``TypeGuard``
returns true. However, in the ``else`` clause, :pep:`647` didn't prescribe
what the type might be:
    def is_str(val: str | int) -> TypeGuard[str]:
        return isinstance(val, str)

    def func(val: str | int):
        if is_str(val):
            # Type checkers can assume val is a 'str' in this branch
        else:
            # Type here is not narrowed. It is still 'str | int' 

pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Outdated Show resolved Hide resolved
pep-0722.rst Show resolved Hide resolved
pep-0722.rst Show resolved Hide resolved

def func(val: int | str):
if is_positive_int(val):
# Type checker assumes PosInt here
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that that's how Pyright works. I guess I'm wondering:

a) Is this correct?

b) What would be the behavior if PosInt was unrelated to any of the types in the val union? Consider the following example. Within the if block, is val considered to be datetime because is_datetime returned true? Or is it Never because the intersection of datetime and int | str is empty? Oh, I guess this might be the multiple inheritance thing again. Maybe the val passed into func is derived from both int and datetime?

def is_datetime(val: datetime | int | str) -> TypeGuard[datetime]:
    return isinstance(val, datetime)

def func(val: int | str):
    if is_datetime(val):
        # Is val a datetime here or Never?

c) Should the PEP call more attention to these issues?

rchiodo and others added 2 commits August 3, 2023 14:55
Co-authored-by: Erik De Bonte <[email protected]>
Co-authored-by: Erik De Bonte <[email protected]>
@rchiodo
Copy link
Owner Author

rchiodo commented Aug 3, 2023

I was thinking "Specification". Counter examples don't strike me as part of the motivation for this PEP.

Sounds good. I'll move it.

@rchiodo rchiodo merged commit 66bdf14 into rchiodo/pep-722 Aug 4, 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.

2 participants