Skip to content
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

Pattern Matching - duplicate keys in mapping patterns #88755

Closed
PierreQuentel mannequin opened this issue Jul 9, 2021 · 12 comments
Closed

Pattern Matching - duplicate keys in mapping patterns #88755

PierreQuentel mannequin opened this issue Jul 9, 2021 · 12 comments
Assignees
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@PierreQuentel
Copy link
Mannequin

PierreQuentel mannequin commented Jul 9, 2021

BPO 44589
Nosy @gvanrossum, @PierreQuentel, @pablogsal, @miss-islington, @brandtbucher, @jdevries3133
PRs
  • bpo-44589: raise SyntaxError when mapping pattern has duplicate key #27131
  • [3.10] bpo-44589: raise a SyntaxError when mapping patterns have duplicate literal keys (GH-27131) #27157
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brandtbucher'
    closed_at = <Date 2021-07-17.04:20:16.498>
    created_at = <Date 2021-07-09.10:16:54.853>
    labels = ['interpreter-core', 'type-bug', '3.11']
    title = 'Pattern Matching - duplicate keys in mapping patterns'
    updated_at = <Date 2021-07-17.04:20:16.497>
    user = 'https://github.com/PierreQuentel'

    bugs.python.org fields:

    activity = <Date 2021-07-17.04:20:16.497>
    actor = 'pablogsal'
    assignee = 'brandtbucher'
    closed = True
    closed_date = <Date 2021-07-17.04:20:16.498>
    closer = 'pablogsal'
    components = ['Interpreter Core']
    creation = <Date 2021-07-09.10:16:54.853>
    creator = 'quentel'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44589
    keywords = ['patch']
    message_count = 12.0
    messages = ['397195', '397203', '397204', '397215', '397240', '397460', '397493', '397498', '397519', '397522', '397687', '397697']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'quentel', 'pablogsal', 'miss-islington', 'brandtbucher', 'jack__d']
    pr_nums = ['27131', '27157']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue44589'
    versions = ['Python 3.11']

    @PierreQuentel
    Copy link
    Mannequin Author

    PierreQuentel mannequin commented Jul 9, 2021

    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 test_patma.py:

        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}:

    @PierreQuentel PierreQuentel mannequin added 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jul 9, 2021
    @brandtbucher
    Copy link
    Member

    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.

    @brandtbucher brandtbucher added 3.11 only security fixes type-bug An unexpected behavior, bug, or error and removed 3.10 only security fixes labels Jul 9, 2021
    @brandtbucher
    Copy link
    Member

    Sorry, that should be PEP-634, not 638.

    @gvanrossum
    Copy link
    Member

    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
    error.--
    --Guido (mobile)

    @PierreQuentel
    Copy link
    Mannequin Author

    PierreQuentel mannequin commented Jul 10, 2021

    Sorry, I don't know C so I can't write a PR for this change.

    @jdevries3133
    Copy link
    Mannequin

    jdevries3133 mannequin commented Jul 14, 2021

    @brandtbucher, I think that my PR implements the solution you've described here. What do you think?

    @brandtbucher
    Copy link
    Member

    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. :)

    @jdevries3133
    Copy link
    Mannequin

    jdevries3133 mannequin commented Jul 14, 2021

    @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.

    @brandtbucher
    Copy link
    Member

    New changeset 2693132 by Jack DeVries in branch 'main':
    bpo-44589: raise a SyntaxError when mapping patterns have duplicate literal keys (GH-27131)
    2693132

    @miss-islington
    Copy link
    Contributor

    New changeset 016af14 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)
    016af14

    @pablogsal
    Copy link
    Member

    Is something left here?

    @jdevries3133
    Copy link
    Mannequin

    jdevries3133 mannequin commented Jul 17, 2021

    The PR and backport to 3.10 have both been merged, so I think this issue can be closed.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants