bpo-42128: Structural Pattern Matching (PEP 634)#22917
bpo-42128: Structural Pattern Matching (PEP 634)#22917brandtbucher merged 260 commits intopython:masterfrom
Conversation
This currently segfaults but I still like it
|
Gotcha. Yeah, I was referring to the next alpha, scheduled for 3/1. I thought maybe you might need a day or two between tagging and releasing or something. |
No, although is a multi-hour process, I normally do it on the same day as the release, at least for the alphas (assuming everything goes smoothly). |
willingc
left a comment
There was a problem hiding this comment.
@brandtbucher I did an initial pass and it looks good for alpha. I will look at it more closely this weekend.
|
found a bit of an odd case while trying this out -- not sure if it's intentional: x = 1
match x:
case 1 as y if 0:
print('unreachable')
print(f'{y=}') # expect NameErrorinstead of NameError I get |
IIRC I think that is covered in https://www.python.org/dev/peps/pep-0634/#side-effects-and-undefined-behavior, although the peephole could detect the branch and optimize it away. |
|
Yeah, we're required to bind any names used by the guard before evaluating it (obviously). The current naive implementation accomplishes this by binding all names before hitting any guard. Strange example, but legal by the PEP. I'm actually not sure the peephole/CFG could catch this (it can optimize out the body of the case, though, and it actually does here). Might be something for the AST optimizer. |
Python/ceval.c
Outdated
| if (dummy == NULL) { | ||
| goto fail; | ||
| } | ||
| values = PyTuple_New(nkeys); |
There was a problem hiding this comment.
This is dangerous. You are preparing a tuple with nkeys but then you are filling it calling PyObject_CallFunctionObjArgs, which can execute arbitrary Python code. At this point, the tuple is tracked by the GC so code executed by PyObject_CallFunctionObjArgs could see the tuple half-initialized (the same if there is a gc pass). The same thing applies to anything done meanwhile the tuple is not ready that can call into arbitrary Python.
There was a problem hiding this comment.
I see. So is filling it with None the right thing to do here, or can we cheat and just untrack/retrack?
There was a problem hiding this comment.
Actually, filling it with None can lead to problems (as we've seen before). So should we just untrack it temporarily?
I guess we could just fill a list and convert it to a tuple, too.
There was a problem hiding this comment.
Okay, I just went with list -> tuple for both. Let me know if you prefer something else.
There was a problem hiding this comment.
Another option it to untrack the tuple after its creation and track it back when is ready. Be careful because some APIs can retrack it (like resizing the tuple). I would recommend to add some asserts to check that is untracked where is supposed to be if you decide to go this way.
Python/ceval.c
Outdated
| return NULL; | ||
| } | ||
| // So far so good: | ||
| PyObject *attrs = PyTuple_New(nargs + PyTuple_GET_SIZE(kwargs)); |
There was a problem hiding this comment.
Python/ceval.c
Outdated
| // to raise TypeErrors for repeated lookups. On failure, return NULL (with | ||
| // no error set). Use _PyErr_Occurred(tstate) to disambiguate. | ||
| assert(PyUnicode_CheckExact(name)); | ||
| // XXX: Why doesn't PySet_CheckExact exist? |
|
I believe I have addressed all of the comments so far. If anyone still has additional feedback (or needs the weekend to review), please let me know. Otherwise, I'll plan on merging this Friday. @pablogsal, should we hit it with the buildbots again? |
|
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 61616fc 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
|
The three failed jobs look unrelated to these changes; I see the same failures (with the same logs) on #24643. |
|
Okay, I'm going to merge this. 🍾 Big thanks to everyone who took time to help out here! |
Co-authored-by: Guido van Rossum <[email protected]> Co-authored-by: Talin <[email protected]> Co-authored-by: Pablo Galindo <[email protected]>
| PyObject *attr = PyObject_GetAttr(subject, name); | ||
| if (attr == NULL && _PyErr_ExceptionMatches(tstate, PyExc_AttributeError)) { | ||
| _PyErr_Clear(tstate); | ||
| } |
There was a problem hiding this comment.
It is better to use _PyObject_LookupAttr() here. See #106303.
| else if (_PyErr_ExceptionMatches(tstate, PyExc_AttributeError)) { | ||
| _PyErr_Clear(tstate); |
PEP 634 has now been accepted, and we'd like to get this into the next alpha.
Several people have volunteered to review the implementation, since it's so huge. Other reviews are very welcome, if anybody has a bit of time to pitch in. This touches tons of stuff: the parser, the compiler, the VM, the builtins, the stdlib, the tests... I'd like as many eyeballs as possible!
https://bugs.python.org/issue42128