[syntax-errors] Detect duplicate keys in `match` mapping patterns by ntBre · Pull Request #17129 · astral-sh/ruff

ntBre · 2025-04-01T18:06:24Z

Summary

Detects duplicate literals in match mapping keys.

This PR also adds a source method to SemanticSyntaxContext to display the duplicated key in the error message by slicing out its range.

Test Plan

New inline tests.

Summary -- Detects duplicate literals in `match` mapping keys. This PR also adds a `source` method to `SemanticSyntaxContext` to display the duplicated key in the error message by slicing out its range. Test Plan -- New inline tests.

ntBre · 2025-04-01T18:08:17Z

crates/ruff_python_parser/src/semantic_errors.rs

+                // complex numbers (`1 + 2j`) are allowed as keys but are not literals
+                // because they are represented as a `BinOp::Add` between a real number and
+                // an imaginary number
+                .filter(|key| key.is_literal_expr() || key.is_bin_op_expr())


We could be a bit stricter here if we want (checking that the BinOp is an Add and the arguments are numbers), but we already report syntax errors for non-complex literals like 1 + 2, so I thought this might be sufficient.

Yeah, I think that's fine, the parser should catch invalid complex literals.

We also have to allow f-string expressions. They're allowed for as long as they contain no placeholders and they should compare equal to their string equivalent (I think this is already handled by ComparableExpr).

I think f-strings are not allowed here by CPython, even without placeholders:

>>> match x: ... case {f"x": 1}: ... ... File "<python-input-2>", line 2 case {f"x": 1}: ... ^^^^^^^^^ SyntaxError: mapping pattern keys may only match literals and attribute lookups

In that case, I think our is_literal_expr is doing the right thing.

Yeah, f-strings are not allowed as literal patterns.

github-actions · 2025-04-01T18:15:36Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

dhruvmanila

Looks good, I'd suggest that we improve the diagnostic range and expand the check for all keys unless it creates an issue.

dhruvmanila · 2025-04-01T20:06:59Z

crates/ruff_python_parser/src/semantic_errors.rs

+                    Self::add_error(
+                        ctx,
+                        SemanticSyntaxErrorKind::DuplicateMatchKey(duplicate_key),
+                        mapping.range,
+                    );
+                    break;


I think we should avoid breaking here and report all duplicate keys within a single mapping pattern. Or, do you see any limitation or challenges in that approach? What do you think?

I was just copying CPython, but that sounds reasonable too!

>>> match x: ... case {"x": 1, "x": 2}: ... ... File "<python-input-223>", line 2 case {"x": 1, "x": 2}: ... ^^^^^^^^^^^^^^^^ SyntaxError: mapping pattern checks duplicate key ('x')

Yeah, in general, we should prefer to surface all errors whenever possible which is one of the main motivation to have an error resilient parser :)

dhruvmanila · 2025-04-01T20:07:57Z

crates/ruff_python_parser/src/semantic_errors.rs

+    /// ```pycon
+    /// >>> match x:
+    /// ...     case {"x": 1, "x": 2}: ...
+    /// ...
+    ///   File "<python-input-160>", line 2
+    ///     case {"x": 1, "x": 2}: ...
+    ///          ^^^^^^^^^^^^^^^^
+    /// SyntaxError: mapping pattern checks duplicate key ('x')
+    /// >>> match x:
+    /// ...     case {x.a: 1, x.a: 2}: ...
+    /// ...
+    /// >>>
+    /// ```


Interesting!

Also, I think you meant "python" or "py" and not "pycon" xD

Oh I thought pycon was for the Python console [1]. That's what I use here on GitHub, although it doesn't usually do much syntax highlighting anyway.

Oh lol, I had no idea about that. I don't think that matters as those are going to be rendered in an editor or crates.io.

dhruvmanila · 2025-04-01T20:11:15Z

crates/ruff_python_parser/tests/snapshots/invalid_syntax@duplicate_match_key.py.snap

+1 | match x:
+2 |     case {"x": 1, "x": 2}: ...
+  |          ^^^^^^^^^^^^^^^^ Syntax Error: mapping pattern checks duplicate key `"x"`
+3 |     case {b"x": 1, b"x": 2}: ...
+4 |     case {0: 1, 0: 2}: ...


What happens when there are multiple different duplicate keys? Like case {0: 1, "x": 1, 0: 2, "x": 2}: ....

I think we should highlight all the subsequent duplicate keys i.e., in the above case we should highlight the second 0 and the second "x" instance.

Yeah CPython just notes the first one (also inline with the break above), but I think this is a good idea.

>>> match x: ... case {"x": 1, "x": 2, 0: 3, 0: 4}: ... ... File "<python-input-224>", line 2 case {"x": 1, "x": 2, 0: 3, 0: 4}: ... ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SyntaxError: mapping pattern checks duplicate key ('x')

dhruvmanila · 2025-04-01T20:12:31Z

crates/ruff_python_parser/src/semantic_errors.rs

+                // complex numbers (`1 + 2j`) are allowed as keys but are not literals
+                // because they are represented as a `BinOp::Add` between a real number and
+                // an imaginary number
+                .filter(|key| key.is_literal_expr() || key.is_bin_op_expr())


Yeah, I think that's fine, the parser should catch invalid complex literals.

MichaReiser · 2025-04-02T07:29:42Z

crates/ruff_python_parser/src/semantic_errors.rs

+                // complex numbers (`1 + 2j`) are allowed as keys but are not literals
+                // because they are represented as a `BinOp::Add` between a real number and
+                // an imaginary number
+                .filter(|key| key.is_literal_expr() || key.is_bin_op_expr())


We also have to allow f-string expressions. They're allowed for as long as they contain no placeholders and they should compare equal to their string equivalent (I think this is already handled by ComparableExpr).

MichaReiser · 2025-04-02T07:32:36Z

crates/ruff_python_parser/tests/snapshots/invalid_syntax@duplicate_match_key.py.snap

+  |
+1 | match x:
+2 |     case {"x": 1, "x": 2}: ...
+  |                   ^^^ Syntax Error: mapping pattern checks duplicate key `"x"`


Another use case for multi-span diagostics :)

MichaReiser · 2025-04-02T07:34:53Z

crates/ruff_python_parser/tests/snapshots/invalid_syntax@duplicate_match_key.py.snap

+   | |_______^ Syntax Error: mapping pattern checks duplicate key `"""x
+    y
+    z
+    """`


This will break our concise rendering where each message should only be a single line long. I don't have a good recommendation but it's a general concern with including source text as is in diagnostic messages. You can either replace new lines, truncate before the new line (and replace with ...), or not include the name if it is multiline.

I went with escaping the newlines to match CPython (as we discussed on Discord):

>>> match x: ... case { ... """x ... y ... z ... """: 1, ... """x ... y ... z ... """: 2}: ... ... File "<python-input-0>", line 2 case { ^ SyntaxError: mapping pattern checks duplicate key ('x\n y\n z\n ')

I added a modified version of std::str::EscapeDefault from the Rust standard library. It's a little more heavily modified than I wanted because many of the functions in the real implementation are private, but it gets the job done and without too much code.

* origin/main: [red-knot] Add `Type::TypeVar` variant (#17102) [red-knot] update to latest Salsa with fixpoint caching fix (#17179) Upgrade to Rust 1.86 and bump MSRV to 1.84 (#17171) [red-knot] Avoid unresolved-reference in unreachable code (#17169) Fix relative import resolution in `site-packages` packages when the `site-packages` search path is a subdirectory of the first-party search path (#17178) [DO NOT LAND] bump Salsa version (#17176) [syntax-errors] Detect duplicate keys in `match` mapping patterns (#17129)

…tral-sh#17129) Summary -- Detects duplicate literals in `match` mapping keys. This PR also adds a `source` method to `SemanticSyntaxContext` to display the duplicated key in the error message by slicing out its range. Test Plan -- New inline tests.

ntBre added rule Implementing or modifying a lint rule preview Related to preview mode features labels Apr 1, 2025

ntBre requested review from MichaReiser and dhruvmanila as code owners April 1, 2025 18:06

ntBre commented Apr 1, 2025

View reviewed changes

ntBre mentioned this pull request Apr 1, 2025

☂️ Syntax errors raised by the compiler #11934

Closed

18 tasks

dhruvmanila reviewed Apr 1, 2025

View reviewed changes

ntBre added 2 commits April 1, 2025 16:23

narrow diagnostic to key range and don't break

3bab92e

add cases with multiple duplicates

47635c1

MichaReiser approved these changes Apr 2, 2025

View reviewed changes

escape newlines to keep diagnostics on one line like CPython

0ed466a

dhruvmanila approved these changes Apr 2, 2025

View reviewed changes

Merge branch 'main' into brent/syn-duplicate-match-key

0abcc18

ntBre mentioned this pull request Apr 2, 2025

[pycodestyle] Auto-fix redundant boolean comparison (E712) #17090

Merged

ntBre merged commit 6e2b8f9 into main Apr 3, 2025
22 checks passed

ntBre deleted the brent/syn-duplicate-match-key branch April 3, 2025 14:22

ntBre mentioned this pull request Apr 4, 2025

Use new diagnostic functionality #17203

Open

7 tasks

BrewTestBot mentioned this pull request Apr 4, 2025

ruff 0.11.4 Homebrew/homebrew-core#218282

Merged

Conversation

ntBre commented Apr 1, 2025

Summary

Test Plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

dhruvmanila left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 1, 2025 •

edited

Loading

`ruff-ecosystem` results