8000 Predict enum value type for unknown member names. by mgilson · Pull Request #9443 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Predict enum value type for unknown member names. #9443

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Sep 17, 2020
Merged

Predict enum value type for unknown member names. #9443

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
fa67a31
Predict enum value type for unknown member names.
mgilson-argo Sep 15, 2020
077d3e7
Fix a typo. Squash before merging.
mgilson-argo Sep 15, 2020
e861117
Get enum-value inference to work with auto.
mgilson-argo Sep 16, 2020
db16871
Fix flake8 complaint.
mgilson-argo Sep 16, 2020
bfee76d
Fixup the unit-tests.
mgilson-argo Sep 16, 2020
b3653d7
Remove some unused fixtures.
mgilson-argo Sep 16, 2020
f1adaf5
Make the self-test happy.
mgilson-argo Sep 16, 2020
22c1474
Invert logic for inferring with value-type in presence of auto.
mgilson-argo Sep 17, 2020
aae9de3
Change a ValueError to an `assert` for type collapsing.
mgilson-argo Sep 17, 2020
d392d05
Shorten some names and drop a branch or two.
mgilson-argo Sep 17, 2020
050fdc2
Get my conditional right ... I think ...
mgilson-argo Sep 17, 2020
5fa5a93
Fix the fixture application.
mgilson-argo Sep 17, 2020
5b8baa9
Stop using `next` for now.
mgilson-argo Sep 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Predict enum value type for unknown member names. 
It is very common for enums to have homogenous member-value types.
In the case where we do not know what enum member we are dealing
with, we should sniff for that case and still collapse to a known
type if that assuption holds.
  • Loading branch information
@mgilson-argo
mgilson-argo committed Sep 15, 2020
commit fa67a318f1ec3d3df9c2cbd422d7af1dd1a34c26
16 changes: 16 additions & 0 deletions mypy/plugins/enums.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,22 @@ class SomeEnum:
"""
enum_field_name = _extract_underlying_field_name(ctx.type)
if enum_field_name is None:
# We do not know the ennum field name (perhaps it was passed to a function and we only
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We do not know the ennum field name (perhaps it was passed to a function and we only
# We do not know the enum field name (perhaps it was passed to a function and we only

# know that it _is_ a member). All is not lost however, if we can prove that the all
# of the enum members have the same value-type, then it doesn't matter which member
# was passed in. The value-type is still known.
if isinstance(ctx.type, Instance):
info = ctx.type.type
stnodes = (info.get(name) for name in info.names)
first_node = next(stnodes, None)
if first_node is None:
return ctx.default_attr_type
first_node_type = first_node.type
if all(node is not None and node.type == first_node_type for node in stnodes):
underlying_type = get_proper_type(first_node_type)
if underlying_type is not None:
return underlying_type
Copy link
Member

Choose a reason for hiding this comment

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

If I read the docstring of get_proper_type() correctly, it suggest that here you should actually be returning first_node_type. And in fact, perhaps you should probably adjust the all() check to go through get_proper_type() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a stab at this, but I'm not completely sure what you're saying here :). It's unclear to me whether the result of enum_value_callback should be a ProperType or not. The existing code seems to try to return a ProperType so I just followed suit there. I did change the "all-equal" to compare the entities after figuring out their ProperType though.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think I misunderstood this. I thought that using get_proper_type() would distinguish A from int if we have

A = int
def f(a: A): ...

but it doesn't seem to work that way. So you're forgiven for not understanding me. :-)

I think what you have now is fine.


return ctx.default_attr_type

assert isinstance(ctx.type, Instance)
Expand Down
20 changes: 20 additions & 0 deletions test-data/unit/check-enum.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,26 @@ reveal_type(Truth.true.name) # N: Revealed type is 'Literal['true']?'
reveal_type(Truth.false.value) # N: Revealed type is 'builtins.bool'
[builtins fixtures/bool.pyi]

[case testEnumValueExtended]
from enum import Enum
class Truth(Enum):
true = True
false = False

def infer_truth(truth: Truth) -> None:
reveal_type(truth.value) # N: Revealed type is 'builtins.bool'
[builtins fixtures/bool.pyi]

[case testEnumValueInhomogenous]
from enum import Enum
class Truth(Enum):
true = 'True'
false = 0

def cannot_infer_truth(truth: Truth) -> None:
reveal_type(truth.value) # N: Revealed type is 'Any'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this shouldn't infer object instead of Any?

In favor of Any: That's what we did before in this case; it prevents false positives.

In favor of object: That's what you'd see for e.g. reveal_type("" if a else 0), assuming a has an unknown value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to just follow the existing art. I imagine that changing to object could cause some code to fail with false positives (If a user is actually using the .value, it seems unlikely that they will be restricting themselves to the interface provided by object so this would probably force them to do additional cast or other methods of type-narrowing)

With that said ... once this is implemented, it shouldn't matter to me either way. Hopefully mypy will know the types of all of my enum values since I see no reason for inhomogenous values and it should never bother me in either case :). I'll do whatever you like.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, let's stick with tradition and keep Any.

[builtins fixtures/bool.pyi]

[case testEnumUnique]
import enum
@enum.unique
Expand Down
0