-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 mypyc crash with enum type aliases #18725
Conversation
mypyc was crashing because it couldn't find the type in the type map. This PR adds a generic AnyType to the type map if an expression isn't in the map already. Tried actually changing mypy to accept these type alias expressions, but ran into problems with nested type aliases where the inner one doesn't have the "analyzed" value and ending up with wrong results. fixes mypyc/mypyc#1064
class SomeEnum(Enum): | ||
AVALUE = "a" | ||
|
||
ALIAS = Literal[SomeEnum.AVALUE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also test nested Literal
types that aren't nested within an IndexExpr, such as Literal[<...>] | None
. You must target 3.10 to enable the new style union type syntax. I think you can add _python3_10
suffix to the test case name to only run it on 3.10 and later.
Test explicit type aliases (e.g. AliasExplicit: TypeAlias = Literal[<...>]
).
Test type alias statements (e.g. type Alias = Literal[<...>]
). Since it only works on 3.12 and later, it's best to add it to testPEP695Basics
in mypyc/test-data/run-python312.test
. It already has some similar test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Literal[<...>] | None
didn't seem to work:
[...]
File "/home/svalentin/src/mypy-svalentin/mypy/checkexpr.py", line 4890, in alias_type_in_runtime_context
return self.chk.named_generic_type("types.UnionType", item.items)
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/svalentin/src/mypy-svalentin/mypy/checker.py", line 7285, in named_generic_type
info = self.lookup_typeinfo(name)
File "/home/svalentin/src/mypy-svalentin/mypy/checker.py", line 7292, in lookup_typeinfo
sym = self.lookup_qualified(fullname)
File "/home/svalentin/src/mypy-svalentin/mypy/checker.py", line 7370, in lookup_qualified
n = self.modules[parts[0]]
~~~~~~~~~~~~^^^^^^^^^^
KeyError: 'types'
Even with [typing fixtures/typing-full.pyi]
.
I haven't seen any other test use the X | Y
form of type definition. I think we should leave the supporting of X | Y
types as a separate issue.
I'll add it in run-python312.test since it seems to be fine there.
Separately, is there any reason why we can't have Test_python3_12
tests and we must use run-python312.test
?
I think we should update the testing README.md with the differences between Test_python3_12
, run-python312.test
and # flags: --python-version 3.12
. I realize that the last one is just for mypy, not for mypyc, but might be worth documenting the differences in both set of tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's fine to support X | Y
in a follow-up issue. It might be a stub issue -- maybe adding import types
to the test case would help to simulate how the real typeshed stubs work.
mypyc/irbuild/prebuildvisitor.py
Outdated
def visit_assignment_stmt(self, stmt: AssignmentStmt) -> None: | ||
# These are cases where mypy may not have types for certain expressions, | ||
# but mypyc needs some form type to exist. | ||
if isinstance(stmt.rvalue, IndexExpr) and stmt.rvalue.analyzed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better check would be if stmt.is_alias_def:
, since this would trigger for all aliases, and if the IndexExpr
is nested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I missed the is_alias_def
!
bb46323
to
c6afe2b
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good now!
class SomeEnum(Enum): | ||
AVALUE = "a" | ||
|
||
ALIAS = Literal[SomeEnum.AVALUE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it's fine to support X | Y
in a follow-up issue. It might be a stub issue -- maybe adding import types
to the test case would help to simulate how the real typeshed stubs work.
mypyc was crashing because it couldn't find the type in the type map. This PR adds a generic AnyType to the type map if an expression isn't in the map already. Tried actually changing mypy to accept these type alias expressions, but ran into problems with nested type aliases where the inner one doesn't have the "analyzed" value and ending up with wrong results. fixes mypyc/mypyc#1064
mypyc was crashing because it couldn't find the type in the type map. This PR adds a generic AnyType to the type map if an expression isn't in the map already.
Tried actually changing mypy to accept these type alias expressions, but ran into problems with nested type aliases where the inner one doesn't have the "analyzed" value and ending up with wrong results.
fixes mypyc/mypyc#1064