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

Add basic support for enum literals #6668

Merged

Conversation

@Michael0x2a
Copy link
Collaborator

commented Apr 12, 2019

This pull request adds in basic support for enum literals. Basically, you can construct them and use them as types, but not much else.

(I originally said I was going to wait until the new semantic analyzer work was finished, but I'm bored.)

Some notes:

  1. I plan on submitting incremental and fine-grained-mode tests in a future PR.

  2. I added two copies of each test -- one using the old semantic analyzer and one using the new one. Let me know if we prefer not doing this and if I should pick one over the other.

  3. I wanted to add support for aliases to enums, but ran into some difficulty doing so. See #6667 for some analysis on the root cause.

Add basic support for enum literals
This pull request adds in basic support for enum literals. Basically,
you can construct them and use them as types, but not much else.

(I originally said I was going to wait until the new semantic analyzer
work was finished, but I'm bored.)

Some notes:

1. I plan on submitting incremental and fine-grained-mode tests in
   a future PR.

2. I added two copies of each test -- one using the old semantic
   analyzer and one using the new one. Let me know if we prefer not
   doing this and if I should pick one over the other.

3. I wanted to add support for aliases to enums, but ran into some
   difficulty doing so. See #6667
   for some analysis on the root cause.
@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks! The idea looks good. There is of course code duplication for both analyzers, but it is fine for me, if it is OK for you.

I left some comments.

for i in range(1, len(parts) - 1):
next_sym = n.names[parts[i]]
assert isinstance(next_sym.node, MypyFile)
assert isinstance(next_sym.node, (MypyFile, TypeInfo))

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

I think there is some misunderstanding/bug somewhere. This function should never be used for things like local lookup. It should be only used to construct a known type like builtins.str. For local lookup one needs to use lookup_qualified(). Why do you need this modification?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Apr 12, 2019

Author Collaborator

I made this change in order to get this example working:

from enum import Enum
from typing_extensions import Literal

class Outer:
    class Inner(Enum):
        FOO = 1
        BAR = 2

x: Literal[Outer.Inner.FOO]
reveal_type(x)

Basically, the literal-handling code in typeanal receives back the string "module.Outer.Inner". We then call typeanal.named_type_with_normalized_str which in turn calls typeanal.named_type which in turn calls semanal.lookup_fully_qualified. That function then ends up crashing because "Outer" is a TypeInfo, not a MypyFile.

FWIW I was under the impression that:

  1. lookup_qualified is for looking up fully-qualified types, and has a bunch of error handling for when the thing we're trying to look up doesn't exist.
  2. lookup_fully_qualified is a simpler version of lookup_qualified, except without error handling.
  3. lookup is for looking up some unqualified name across all namespaces (local and global).

I also noticed that lookup_qualified handles both MypyFile and TypeInfo, and so figured that the fact that lookup_fully_qualified doesn't was an oversight.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

FWIW I was under the impression that

This is a wrong impression. Which is normal, we have more than dozen lookup functions in the code-base (likely caused by the fact that many people who are not familiar with mypy contributed some reinvented bicycles, which is again normal). Largely, they can be separated in two classes:

  • Local lookup: what given name or member expression points to using Python name resolution
  • Global lookup: find a symbol using its "full address" used to construct some standard known types.

The named_type() and friends are from the second group. While the first group is roughly two methods lookup() and lookup_qualified().

Essentially what is going on here looks like a hack (and TBH I don't like it). You already have the TypeInfo needed to construct the literal type, but instead you get its full name and pass it to RawExpressionType that later passes the name to find the same TypeInfo again.

Initially, string name was passed to RawLiteralExpression because it is was constructed at very early stage. Here however, it looks unnecessary. Why can't we just immediately return LiteralType there with a fallback instance constructed from a known TypeInfo?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Apr 12, 2019

Author Collaborator

The named_type() and friends are from the second group. While the first group is roughly two methods lookup() and lookup_qualified().

Hmm, ok. In that case, what's the correct way of looking up a nested type via a global lookup?

It seems all of the "global lookup" functions assume that all types are defined within modules only, which I'm not so sure is correct.

Why can't we just immediately return LiteralType there with a fallback instance constructed from a known TypeInfo?

Sure, we can do that. Is it safe to just construct new whenever we want Instances though? E.g. there isn't a need to register them in some global registry?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

It seems all of the "global lookup" functions assume that all types are defined within modules only, which I'm not so sure is correct.

It is not correct, but this is kind of optimization. After the refactoring we will have two methods, one fast for builtin things like builtins.str, and another one that can be used in plugins etc., with full semantics.

E.g. there isn't a need to register them in some global registry?

What kind of registry do you mean? I think one can just call Instance(info, args, line, column) or am I missing something?.

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Apr 12, 2019

Author Collaborator

What kind of registry do you mean? I think one can just call Instance(info, args, line, column) or am I missing something?.

Honestly, I have no idea. Instances and TypeInfos are stuffed with so much functionality that I always get a little worried that touching either of them or creating new ones will break something or interfere with something related to incremental or fine-grained mode in some weird way.

mypy/semanal.py Outdated Show resolved Hide resolved
mypy/semanal.py Outdated Show resolved Hide resolved
mypy/typeanal.py Outdated Show resolved Hide resolved
mypy/typeanal.py Outdated Show resolved Hide resolved
test-data/unit/check-literal.test Outdated Show resolved Hide resolved
test-data/unit/check-literal.test Outdated Show resolved Hide resolved
test-data/unit/check-literal.test Outdated Show resolved Hide resolved
c: Literal[mod_b.Test.FOO]

f(a)
f(b) # E: Argument 1 to "f" has incompatible type "Literal[mod_a.Test2.FOO]"; expected "Literal[mod_a.Test.FOO]"

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

Even here I would skip the module name. Maybe you can somehow hack this with MessageBuilder.format_distinctly()?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Apr 12, 2019

Author Collaborator

Ooh, I forgot about MessageBuilder.format_distinctly! It turned out tweaking that function was sufficient to simplify most of the error messages above in the way you suggested.

test-data/unit/check-literal.test Outdated Show resolved Hide resolved

Michael0x2a added some commits Apr 12, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

OK, here is one more (long) comment. I also noticed a long comment and a TODO in visit_literal_type() in the third pass, is it still relevant? (I hope no, because there will be no third pass in new semantic analyzer).

for i in range(1, len(parts) - 1):
next_sym = n.names[parts[i]]
assert isinstance(next_sym.node, MypyFile)
assert isinstance(next_sym.node, (MypyFile, TypeInfo))

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 12, 2019

Collaborator

FWIW I was under the impression that

This is a wrong impression. Which is normal, we have more than dozen lookup functions in the code-base (likely caused by the fact that many people who are not familiar with mypy contributed some reinvented bicycles, which is again normal). Largely, they can be separated in two classes:

  • Local lookup: what given name or member expression points to using Python name resolution
  • Global lookup: find a symbol using its "full address" used to construct some standard known types.

The named_type() and friends are from the second group. While the first group is roughly two methods lookup() and lookup_qualified().

Essentially what is going on here looks like a hack (and TBH I don't like it). You already have the TypeInfo needed to construct the literal type, but instead you get its full name and pass it to RawExpressionType that later passes the name to find the same TypeInfo again.

Initially, string name was passed to RawLiteralExpression because it is was constructed at very early stage. Here however, it looks unnecessary. Why can't we just immediately return LiteralType there with a fallback instance constructed from a known TypeInfo?

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

I also noticed a long comment and a TODO in visit_literal_type() in the third pass, is it still relevant?

I think probably not? I'll make sure to double-check this and see if we can delete the comment while preparing the incremental mode and fine-grained mode tests PR.

@ilevkivskyi
Copy link
Collaborator

left a comment

I think probably not? I'll make sure to double-check this and see if we can delete the comment while preparing the incremental mode and fine-grained mode tests PR.

Great, thanks! This PR is good to go then.

@Michael0x2a Michael0x2a merged commit 11cc21c into python:master Apr 12, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:add-basic-support-for-enum-literals branch Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.