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

Fix enum attributes are not members #17207

Merged

Conversation

terencehonles
Copy link
Contributor

@terencehonles terencehonles commented May 2, 2024

This adds on to the change in #17182 and fixes enum attributes being used as members.

@hamdanal / @hauntsaninja I noticed there was the function get_enum_values in mypy types and instead of adding more to the case in try_expanding_sum_type_to_union it seemed like it might make sense to move the changes from #17182 there. I don't think the other code touched in that PR can use get_enum_values.

fixes: #16730

@terencehonles terencehonles force-pushed the fix-enum-with-property-match-exhaustion branch from f319685 to ba435ac Compare May 2, 2024 15:21

This comment has been minimized.

isinstance(sym.node, mypy.nodes.Var)
and name not in ENUM_REMOVED_PROPS
and not name.startswith("__")
and sym.node.has_explicit_value
Copy link
Collaborator

Choose a reason for hiding this comment

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

This last condition changes the behavior of mypy which currently considers unassigned but annotated attributes as members of the enum.
I would say the new behavior is better but it must be noted in the PR title or description that this is a breaking change that must be considered on its own.
Looking at the mypy primer output, it looks like members of enums constructed using the enum call syntax do not have the has_explicit_value set which breaks type narrowing. I suggest changing the variable here to also set this flag perhaps with a comment explaining the reason -- something like "E = Enum("E", "A") is equivalent to class E(Enum): A = auto()"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing that, I've not really used them in that form, but hopefully the comment is not too wordy. I only glanced at the code that you pointed to, but I was cross referencing https://github.com/python/cpython/blob/7528b84e947f727734bd802356e380673553387d/Lib/enum.py#L828-L839 and how much of this is implemented by mypy? 🤔

This comment has been minimized.

@terencehonles
Copy link
Contributor Author

It looks like this is actually related to the ongoing discussion python/typing-council#11

Copy link
Collaborator

@hamdanal hamdanal left a comment

Choose a reason for hiding this comment

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

Thank you @terencehonles.
The typing spec change has been accepted and this PR looks good to me.

Regarding the mypy primer diff, it is related to the linked typing spec change that requires assignment for enum attributes for them to be considered members of the enumeration and the fact that tanjun vendors an old copy of the stdlib inspect module and stub that relies on the old behavior. So the new errors are expected.

This adds on to the change in python#17182
and fixes enum attributes being used as members.
@terencehonles terencehonles force-pushed the fix-enum-with-property-match-exhaustion branch from 5383c49 to dfcc0c4 Compare June 5, 2024 13:55

This comment has been minimized.

@JelleZijlstra JelleZijlstra self-requested a review June 5, 2024 15:05

This comment has been minimized.

@terencehonles
Copy link
Contributor Author

@hamdanal sorry for the delay getting back to this, I went on vacation and then coming back was pretty busy. Work's on break for the week so I figured I could pick this up to keep it from getting delayed any further.

I was having trouble forcing mypy to not use the explicitly provided annotation for an enum member and I will probably have to leave it at this.

I left a comment https://github.com/python/mypy/pull/17207/files#diff-8b48b1eca587106e3806bfa9e14b7f7f344e117b203ffc716ce6fef7f2e68fe6R1610 about the test that was added for #11971. The code in question will need to ignore this new error, but as mypy better supports enums the code will need to change as it seems like it's abusing enums a bit.

This comment has been minimized.

@terencehonles
Copy link
Contributor Author

@hamdanal / @JelleZijlstra friendly ping if you have a chance to look over the PR or suggestions on how to fix ^^^

This comment has been minimized.

@terencehonles
Copy link
Contributor Author

@hamdanal / @JelleZijlstra friendly ping

This comment has been minimized.

Copy link
Collaborator

@hamdanal hamdanal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

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.

Thanks! This change brings mypy closer in line with the new typing spec chapter on enums

@hauntsaninja
Copy link
Collaborator

I'm confused by the primer hits on Tanjun

This comment has been minimized.

@JelleZijlstra
Copy link
Member

I'm confused by the primer hits on Tanjun

I looked a little and couldn't figure out what's going on there either. Both cases involve narrowing an enum attribute inside a loop. I wasn't able to reproduce a similar error with a simple test case, or even by cloning Tanjun and running mypy from this PR directly on those files.

@hauntsaninja
Copy link
Collaborator

I wasn't able to repro minimised, but I can repro locally via mypy_primer --repo https://github.com/hauntsaninja/mypy --new 46527644f --old 7f3d7f8f1 -k Tanjun. Note that --warn-unreachable is off by default

@hauntsaninja
Copy link
Collaborator

Oh wait, it vendors inspect

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Oct 26, 2024

This is the regression:

λ cat test10.py
import enum
from typing import ClassVar, Literal, cast

class _ParameterKind(enum.IntEnum):
    POSITIONAL_ONLY: int
    POSITIONAL_OR_KEYWORD: int
    VAR_POSITIONAL: int
    KEYWORD_ONLY: int
    VAR_KEYWORD: int

class Parameter:

    POSITIONAL_ONLY: ClassVar[Literal[_ParameterKind.POSITIONAL_ONLY]]
    POSITIONAL_OR_KEYWORD: ClassVar[Literal[_ParameterKind.POSITIONAL_OR_KEYWORD]]
    VAR_POSITIONAL: ClassVar[Literal[_ParameterKind.VAR_POSITIONAL]]
    KEYWORD_ONLY: ClassVar[Literal[_ParameterKind.KEYWORD_ONLY]]
    VAR_KEYWORD: ClassVar[Literal[_ParameterKind.VAR_KEYWORD]]
    @property
    def kind(self) -> _ParameterKind: ...


def get_kwargs() -> None:
    parameter = cast(Parameter, ...)

    reveal_type(parameter)
    reveal_type(parameter.kind)

    if parameter.kind is parameter.VAR_KEYWORD:
        return

    print("should be reachable")

λ mypy test10.py --warn-unreachable --strict --disable-error-code empty-body
test10.py:25: note: Revealed type is "test10.Parameter"
test10.py:26: note: Revealed type is "test10._ParameterKind"
test10.py:31: error: Statement is unreachable  [unreachable]
Found 1 error in 1 file (checked 1 source file)

@terencehonles
Copy link
Contributor Author

terencehonles commented Oct 28, 2024

@hauntsaninja I believe this might be related to my comment:

I was having trouble forcing mypy to not use the explicitly provided annotation for an enum member and I will probably have to leave it at this.

But this seems to also not be warning about the:

class _ParameterKind(enum.IntEnum):
    POSITIONAL_ONLY: int
    POSITIONAL_OR_KEYWORD: int
    VAR_POSITIONAL: int
    KEYWORD_ONLY: int
    VAR_KEYWORD: int

and I can minimally reproduce with:

[case testEnumWithAnnotationOnly]                                              
# flags: --warn-unreachable                                                    
import enum                                                                    
                                                                               
                                                                               
class E(enum.IntEnum):                                                         
    A: int                                                                     
    B: int                                                                     
                                                                               
                                                                               
def do_check(value: E) -> None:                                                
    reveal_type(value)  # N: Revealed type is "__main__.E"                     
    if value is E.A:                                                           
        return                                                                 
                                                                               
    reveal_type(value)  # N: Revealed type is "Literal[__main__.E.B]"         # <-- fails 
    "should be reachable"                                                     # <-- fails too
                                                                               
[builtins fixtures/primitives.pyi]                                             
[typing fixtures/typing-full.pyi] 

So maybe it's actually that? If I add a A = auto(); B = auto() it does then warn and is not unreachable anymore. I'm adding a test (unfixed) to make sure an enum without values also warns about being annotated.

This comment has been minimized.

@terencehonles
Copy link
Contributor Author

@hauntsaninja I got to the bottom of it, and I fixed it in a way that I figured would have the least amount of knock on effects. According to the spec _ParameterKind should not be warning, but it should be treated as having no members. Which is what I fixed here #17207 (comment)

The issue though is that try_expanding_sum_type_to_union code would return Never for an enum with no members, and because Never is also effectively the base class of all types Never is _ParameterKind.VAR_KEYWORD, and therefore mypy is "correctly" warning that the code is not reachable. I don't think Never should be treated that way, but I don't want to break other things, so instead I have the try_expanding_sum_type_to_union code check how many members it expands to, and if there are no members it doesn't expand the type and leaves it as is. This may need to stay regardless, but I think the Never is Any may cause other subtle bugs.

@terencehonles
Copy link
Contributor Author

terencehonles commented Oct 28, 2024

This may need to stay regardless

Actually I think if there was a warning if an expression is always true or always false, then this should be removed since the way I implemented things is that it hides Never and that detection would be suppressed here.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/scope.py:36: error: Enum members must be left unannotated  [misc]
+ src/_pytest/scope.py:36: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members
+ src/_pytest/scope.py:37: error: Enum members must be left unannotated  [misc]
+ src/_pytest/scope.py:37: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members
+ src/_pytest/scope.py:38: error: Enum members must be left unannotated  [misc]
+ src/_pytest/scope.py:38: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members
+ src/_pytest/scope.py:39: error: Enum members must be left unannotated  [misc]
+ src/_pytest/scope.py:39: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members
+ src/_pytest/scope.py:40: error: Enum members must be left unannotated  [misc]
+ src/_pytest/scope.py:40: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members

jax (https://github.com/google/jax)
+ jaxlib/cpu/_lapack/eig.pyi:20: error: Enum members must be left unannotated  [misc]
+ jaxlib/cpu/_lapack/eig.pyi:20: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members
+ jaxlib/cpu/_lapack/eig.pyi:21: error: Enum members must be left unannotated  [misc]
+ jaxlib/cpu/_lapack/eig.pyi:21: note: See https://typing.readthedocs.io/en/latest/spec/enums.html#defining-members

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.

Thanks, the fix looks great!

I think it might be worth issuing a warning for enums with no members, but only in type stubs. I'll make a PR for that.

@hauntsaninja hauntsaninja merged commit d81a9ef into python:master Oct 29, 2024
19 checks passed
@terencehonles terencehonles deleted the fix-enum-with-property-match-exhaustion branch October 29, 2024 08:51
hauntsaninja added a commit that referenced this pull request Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mypy treats enum fields as enum members
4 participants