-
-
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
[mypyc] Add match
statement support
#13953
Conversation
Still getting a grip with how mypyc does things, but I feel like I am at a spot where I can actually start doing things now.
@brandtbucher can you please take a look? :) |
Looking at int match = Py_TYPE(subject)->tp_flags & Py_TPFLAGS_SEQUENCE;
PyObject *res = match ? Py_True : Py_False; The mapping check is similar: int match = Py_TYPE(subject)->tp_flags & Py_TPFLAGS_MAPPING;
PyObject *res = match ? Py_True : Py_False;
|
We shouldn't assert in the compiler, if it's possible the assertion to fail. Instead, we can generate report an error from mypyc, or guard against the error in mypy. It mypy already generates an error if In general, it's preferable to move checks that happen at runtime in CPython to happen during compilation, since it's usually more efficient, and we'll get more compile-time error reporting, which is also nice. |
Hmm mypy should already infer a union type in this case: def f(x: object) -> None:
match x:
case [*rest]:
y = 1
case {**rest}:
y = 2
reveal_type(rest) # Union[builtins.list[builtins.object], builtins.dict[builtins.object, builtins.object]] What kinds of problems are you having with this? |
These lines will be moved to a separate PR so that they can be reviewed separately from the `match` stuff going on here.
Ok I will start changing the assertions into error messages then. Indeed, having actual error messages will be a lot better then a traceback. About the The following test:
Fails with the following error:
I don't know if there is a way to express union types in the IR, but if you could just use |
Since the `Py_TPFLAGS_MAPPING` and `Py_TPFLAGS_SEQUENCE` are specific to the new pattern matching feature, they are not defined for Python 3.9 and below. Now they are defined if they don't exist, which should fix the issue.
Closes mypyc/mypyc#956 This was originally a part of python#13953, see python#13953 (comment)
@JukkaL , as far as I can tell, Mypy already checks that the class is able to be matched, and that class A:
__match_args__ = None
class B:
num: int
__match_args__ = (123,)
something: str
class C:
num: int
__match_args__ = (something,)
class D:
x: int
def f(x):
match x:
case D(123):
pass
case len(123):
pass Produces the following:
Meaning we never even get to the mypyc stage. With that in mind, I think that it is perfectly fine to leave the assertions as-is, given that they don't seem to be able to be hit. |
Closes mypyc/mypyc#956 This was originally a part of #13953, see #13953 (comment)
It would appear that 9125155 is now causing issues with the IR tests. Reverting that commit seems to fix it, but I cannot seem to remember why I added it in the first place, and the build artifacts have long since expired.
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.
Looks good. Left some comments -- after you've addressed them (possibly by adding TODOs) this is ready to merge.
And sorry for the slow review! I've been focused on the mypy 1.0 release and was traveling recently.
slow_isinstance_op, | ||
[self.subject, self.builder.accept(pattern.class_ref)], | ||
pattern.line, | ||
) |
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.
Here we could possibly use a more efficient isinstance op if we are dealing with native classes or primitive types. Can you at least add a TODO comment about this (this can be improved in a follow-up PR)?
* Use faster "isinstance" op if the expression is a built-in type * Add type annotations to run tests * Add type annotated IR example * Add todo comments
Thank you for taking the time to review this! I know you guys have been very busy with the v1 release, so I completely understand the delay. The optimizations for the Also, the issue where captured expressions with the same name (ie, Thanks again! |
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.
This good to merge now! Thanks once again for working on this -- this is an important feature.
I may have time to look at the remaining TODOs, or perhaps another contributor can work on them. This is already a mostly complete implementation.
Closes mypyc/mypyc#911
Like the title says, this PR adds support for compiling
match
statements in mypyc. Most of the work has been done, but there are some things which are still a WIP, though I do not know how to fix them (see below).A todo list of what has been done, and the (small) number of things that need to be worked out:
1 | 2 | 3
123
,x.y.z
, etc.True
,False
, andNone
[1, 2, 3]
[*prev, 4, 5, 6]
,[1, 2, 3, *rest]
, etc:[*rest]
is currently not working, but should be an easy fix{"key": value}
):{"key": value, **rest}
isinstance()
checkClass(1, 2, 3)
Class(x=1, y=2, z=3)
int(x)
->int() as x
_
123 as num
x
Some features which I am unsure how to implement are:
*rest
and**rest
star patterns name collisions. Basically, you cannot userest
(or any other name) twice in the same match statement ifrest
is a different type (ie,dict
vslist
). If it was defined asobject
instead ofdict
/list
everything would be fine.TypeError
should be thrown if__match_args__
is not a tuple, or is otherwise invalid (there are other instances whereTypeError
should be thrown as well). In general though, should we follow the spec and move these errors to run-time? Or, should we keep it as it is, which is anassert
statement at compile time?PySequence_Check
andPyMapping_Check
, though the Mapping check function returned true forstr
types, which is not ideal. If anyone has a better (more reliable) way of checking whether a type supports the Sequence/Mapping protocols, let me know.Also, sorry for the sheer number of commits! I was going to rebase this before I submitted it, but decided that would cut out too much of my thought process.