Title: Pattern Matching - duplicate keys in mapping patterns
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.11
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: brandtbucher Nosy List: brandtbucher, gvanrossum, jack__d, miss-islington, pablogsal, quentel
Priority: normal Keywords: patch

Created on 2021-07-09 10:16 by quentel, last changed 2021-07-17 04:20 by pablogsal. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27131 merged jack__d, 2021-07-14 03:35
PR 27157 merged miss-islington, 2021-07-15 00:38
Messages (12)
msg397195 - (view) Author: Pierre Quentel (quentel) * Date: 2021-07-09 10:16
PEP 634 specifies that

"A mapping pattern may not contain duplicate key values. (If all key patterns are literal patterns this is considered a syntax error; otherwise this is a runtime error and will raise ValueError.)"

but this is not what happens with the latest release:

Python 3.10.0b3 (tags/v3.10.0b3:865714a, Jun 17 2021, 20:39:25) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> x = {'a': 1}
>>> match x:
...  case {'a': 1, 'a': 2}: # (A)
...   print('ok')
>>> x = {'a': 3}
>>> match x:
...  case {'a': 1, 'a': 2}: # (B)
...   print('ok')
>>> x = {'a': 1, 'b': 2}
>>> match x:
...  case {'a': 1, 'a': 2}: # (C)
...   print('ok')
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
ValueError: mapping pattern checks duplicate key ('a')

If I understand the PEP correctly, all these examples should raise a SyntaxError for the line

case {'a': 1, 'a': 2}:

since all key patterns are literal patterns, and the key 'a' is duplicated.

Cases (A) where the subject matches one of the key-value patterns, and (B) when it doesn't, fail without raising SyntaxError.

Case (C) where one of the keys in the subject is not present in the mapping pattern raises a ValueError at runtime instead of SyntaxError.

This behaviour is tested in

    def test_patma_251(self):
        x = {"a": 0, "b": 1}
        w = y = z = None
        with self.assertRaises(ValueError):
            match x:
                case {"a": y, "a": z}:
                    w = 0
        self.assertIs(w, None)
        self.assertIs(y, None)
        self.assertIs(z, None)

but this doesn't seem compliant with the specification.

BTW, it's not clear to me why the SyntaxError should be limited to the case when all keys are literal patterns; it could be raised whenever a literal pattern is repeated, even when there are value patterns or a double-star pattern, like in

case {'a': 1, 'a': 2, c.imag, **rest}:
msg397203 - (view) Author: Brandt Bucher (brandtbucher) * (Python committer) Date: 2021-07-09 17:06
Nice find! In my opinion, we should do two things here:

- Update PEP 638 to specify that a SyntaxError is raised "if *any duplicate* key patterns are literal patterns". This was absolutely our original intention... I think the nuances were just lost in the final phrasing. I can take care of that PR.

- Update the compiler to raise SyntaxErrors for duplicate literal keys. It should be as simple as updating the first loop in compiler_pattern_mapping to build a set containing the values of any literal keys and raise on duplicates (and adding/updating tests, of course). We'll want to make sure we have test coverage of edge cases like {0: _, False: _}, {0: _, 0.0: _}, {0: _, -0: _}, {0: _, 0j: _}, etc.

Since you found this, you get first dibs on a PR. Otherwise, I have a few first-time contributors who would probably be interested.
msg397204 - (view) Author: Brandt Bucher (brandtbucher) * (Python committer) Date: 2021-07-09 17:07
Sorry, that should be PEP 634, not 638.
msg397215 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2021-07-09 20:10
Agreed with everything that Brandt said. These wording issues are subtle!
Note in particular that the original wording implies that {‘x’: 1, ‘x’: 1,
z: 1} would be a runtime error, but it clearly should be a compile time
--Guido (mobile)
msg397240 - (view) Author: Pierre Quentel (quentel) * Date: 2021-07-10 05:41
Sorry, I don't know C so I can't write a PR for this change.
msg397460 - (view) Author: Jack DeVries (jack__d) * Date: 2021-07-14 04:02
@brandtbucher, I think that my PR implements the solution you've described here. What do you think?
msg397493 - (view) Author: Brandt Bucher (brandtbucher) * (Python committer) Date: 2021-07-14 16:23
Thanks for the patch @jack__d! I'll try to find time to review it today.

I do wish you would have coordinated with me here before writing it, though. I'd already begun working on a patch with a few new contributors yesterday, as I mentioned in my comment.

In the future, please check in with those involved in the PR discussion first (*especially* if the issue is already "assigned"). We're doing this for fun, too. :)
msg397498 - (view) Author: Jack DeVries (jack__d) * Date: 2021-07-14 17:38
@brandtbucher, I'm sorry for the miscommunication. I started working on this patch at the beginning of the week after message 397215... I'm a new contributor too, and I wasn't sure if I would be able to make something that worked, so I just kept my mouth shut. Then, all of a sudden, it came together and I had a (mostly) working patch.

I'm going to incorporate your review now; thank you for the feedback, and I'm very sorry for any toes I may have stepped on.
msg397519 - (view) Author: Brandt Bucher (brandtbucher) * (Python committer) Date: 2021-07-15 00:38
New changeset 2693132292b2acf381ac6fa729bf3acf41d9d72b by Jack DeVries in branch 'main':
bpo-44589: raise a SyntaxError when mapping patterns have duplicate literal keys (GH-27131)
msg397522 - (view) Author: miss-islington (miss-islington) Date: 2021-07-15 01:00
New changeset 016af14d93cfba43e7a95721a97fa954c534af8e by Miss Islington (bot) in branch '3.10':
[3.10] bpo-44589: raise a SyntaxError when mapping patterns have duplicate literal keys (GH-27131) (GH-27157)
msg397687 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2021-07-17 03:38
Is something left here?
msg397697 - (view) Author: Jack DeVries (jack__d) * Date: 2021-07-17 04:16
The PR and backport to 3.10 have both been merged, so I think this issue can be closed.
Date User Action Args
2021-07-17 04:20:16pablogsalsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-07-17 04:16:50jack__dsetmessages: + msg397697
2021-07-17 03:38:22pablogsalsetnosy: + pablogsal
messages: + msg397687
2021-07-15 01:00:54miss-islingtonsetmessages: + msg397522
2021-07-15 00:38:57brandtbuchersetmessages: + msg397519
2021-07-15 00:38:51miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request25694
2021-07-14 17:38:00jack__dsetmessages: + msg397498
2021-07-14 16:23:24brandtbuchersetmessages: + msg397493
2021-07-14 04:02:03jack__dsetmessages: + msg397460
2021-07-14 03:35:17jack__dsetkeywords: + patch
nosy: + jack__d

pull_requests: + pull_request25674
stage: patch review
2021-07-10 19:33:14brandtbuchersetassignee: brandtbucher
2021-07-10 05:41:41quentelsetmessages: + msg397240
2021-07-09 20:10:56gvanrossumsetmessages: + msg397215
2021-07-09 17:07:04brandtbuchersetmessages: + msg397204
2021-07-09 17:06:33brandtbuchersetversions: + Python 3.11, - Python 3.10
nosy: + gvanrossum

messages: + msg397203

type: behavior
2021-07-09 11:24:36xtreaksetnosy: + brandtbucher
2021-07-09 10:16:54quentelcreate