gvanrossum / patma

Pattern Matching
1.02k stars 64 forks source link

Is abc.Sequence the right parent for sequence matches? #124

Open dmoisset opened 4 years ago

dmoisset commented 4 years ago

The PEP says that a sequence match:

I got why the first rule is there, I'm not sure if using abcs in the semantics of language constructs is something done elsewhere in Python, and I'm not sure if it's the correct thing to do (I thought operations tended to go towards the pure duck-typing style of attempting the method calls).

I found the following side effects of this decision surprising (can be checked in the binder playground):

The current implementation assumes that negative indexing works, which again I can't find (I'm looking here and here) is a guaranteed contract of sequences. Should we document this?

I haven't looked carefully in the mapping pattern, but it's possible that similar things may be happening with it.

brandtbucher commented 4 years ago

Prior discussion starting at https://github.com/gvanrossum/patma/issues/15#issuecomment-635047668.

gvanrossum commented 4 years ago

The alternative is probing the type using duck typing, which is even worse -- in particular, there is no good way to distinguish a mapping from a sequence since mappings have __len__, __contains__, and __getitem__. (The traditional hack is to check for keys but I find that much more distasteful than requiring collections.abc.Sequence.)

The simplest solution to array.array matching is to register it as a Sequence in Python 3.10.

I didn't know we used slicing and negative indexing, maybe we shouldn't, but I'll let Brandt respond.

brandtbucher commented 4 years ago

The simplest solution to array.array matching is to register it as a Sequence in Python 3.10.

Yeah, I think that is a separate issue that looks like an oversight (although maybe there is some reason why). Probably somebody should just open a new BPO issue.

I didn't know we used slicing and negative indexing, maybe we shouldn't, but I'll let Brandt respond.

I agree that the target should not be required to support slicing and negative indexing. I'll get something up tonight/tomorrow.

The current implementation was mostly driven by a desire to keep new instructions to a minimum and piggyback off of the existing VM's capabilities for the time being. However, this change will probably be cleaner and faster for common cases, since we will avoid using the stack to manage Python int and slice objects just to index into common types like list or tuple.

brandtbucher commented 4 years ago

array.array doesn't define __reversed__. Adding that should be enough to safely register it as a MutableSequence.

~I'll open a BPO.~ Ha, looks like you already agreed this was a good idea 3 years ago @gvanrossum:

https://bugs.python.org/issue29727

Free for anyone who wants to take it. Tag me for review if you want.

brandtbucher commented 4 years ago

The new implementation never uses slices or negative indices (it's cheap because we've already gotten the length of the sequence by the time we reach this point):

>>> class S(tuple):
...     def __getitem__(self, i):
...         print(f"Getting {i}...")
...         return super().__getitem__(i)
... 
>>> match S(range(5)):
...     case first, *middle, last:
...         print(f"Got: {first}, {middle}, {last}")
... 
Getting 0...
Getting 1...
Getting 2...
Getting 3...
Getting 4...
Got: 0, [1, 2, 3], 4
pablogsal commented 4 years ago

array.array objects are now recognized as MutableSequence (and therefore Reversible) in Python3.10:

Python 3.10.0a0 (heads/bpo-29727-dirty:af8fd07361, Jul  5 2020, 19:35:13)
[GCC 10.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import array
>>> x = array.array("i", [1,2,3])
>>> import collections.abc
>>> isinstance(x, collections.abc.MutableSequence)
True
>>> isinstance(x, collections.abc.Reversible)
True