Skip to content

Commit a088186

Browse files
ntBremaxmynter
ntBre
authored and
maxmynter
committed
[syntax-errors] Detect duplicate keys in match mapping patterns (astral-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.
1 parent b1e5dc8 commit a088186

File tree

7 files changed

+1298
-6
lines changed

7 files changed

+1298
-6
lines changed

‎crates/ruff_linter/src/checkers/ast/mod.rs

+5
Original file line numberDiff line numberDiff line change
@@ -573,13 +573,18 @@ impl SemanticSyntaxContext for Checker<'_> {
573573
| SemanticSyntaxErrorKind::IrrefutableCasePattern(_)
574574
| SemanticSyntaxErrorKind::SingleStarredAssignment
575575
| SemanticSyntaxErrorKind::WriteToDebug(_)
576+
| SemanticSyntaxErrorKind::DuplicateMatchKey(_)
576577
| SemanticSyntaxErrorKind::InvalidStarExpression => {
577578
if self.settings.preview.is_enabled() {
578579
self.semantic_errors.borrow_mut().push(error);
579580
}
580581
}
581582
}
582583
}
584+
585+
fn source(&self) -> &str {
586+
self.source()
587+
}
583588
}
584589

585590
impl<'a> Visitor<'a> for Checker<'a> {

‎crates/ruff_python_parser/resources/inline/err/duplicate_match_key.py

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
match x:
2+
case {"x": 1, "x": 2}: ...
3+
case {b"x": 1, b"x": 2}: ...
4+
case {0: 1, 0: 2}: ...
5+
case {1.0: 1, 1.0: 2}: ...
6+
case {1.0 + 2j: 1, 1.0 + 2j: 2}: ...
7+
case {True: 1, True: 2}: ...
8+
case {None: 1, None: 2}: ...
9+
case {
10+
"""x
11+
y
12+
z
13+
""": 1,
14+
"""x
15+
y
16+
z
17+
""": 2}: ...
18+
case {"x": 1, "x": 2, "x": 3}: ...
19+
case {0: 1, "x": 1, 0: 2, "x": 2}: ...

‎crates/ruff_python_parser/resources/inline/ok/duplicate_match_key_attr.py

+2
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
match x:
2+
case {x.a: 1, x.a: 2}: ...

‎crates/ruff_python_parser/src/semantic_errors.rs

+116
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use std::fmt::Display;
88

99
use ruff_python_ast::{
1010
self as ast,
11+
comparable::ComparableExpr,
1112
visitor::{walk_expr, Visitor},
1213
Expr, ExprContext, IrrefutablePatternKind, Pattern, PythonVersion, Stmt, StmtExpr,
1314
StmtImportFrom,
@@ -65,6 +66,7 @@ impl SemanticSyntaxChecker {
6566
Stmt::Match(match_stmt) => {
6667
Self::irrefutable_match_case(match_stmt, ctx);
6768
Self::multiple_case_assignment(match_stmt, ctx);
69+
Self::duplicate_match_mapping_keys(match_stmt, ctx);
6870
}
6971
Stmt::FunctionDef(ast::StmtFunctionDef { type_params, .. })
7072
| Stmt::ClassDef(ast::StmtClassDef { type_params, .. })
@@ -270,6 +272,58 @@ impl SemanticSyntaxChecker {
270272
}
271273
}
272274

275+
fn duplicate_match_mapping_keys<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
276+
for mapping in stmt
277+
.cases
278+
.iter()
279+
.filter_map(|case| case.pattern.as_match_mapping())
280+
{
281+
let mut seen = FxHashSet::default();
282+
for key in mapping
283+
.keys
284+
.iter()
285+
// complex numbers (`1 + 2j`) are allowed as keys but are not literals
286+
// because they are represented as a `BinOp::Add` between a real number and
287+
// an imaginary number
288+
.filter(|key| key.is_literal_expr() || key.is_bin_op_expr())
289+
{
290+
if !seen.insert(ComparableExpr::from(key)) {
291+
let key_range = key.range();
292+
let duplicate_key = ctx.source()[key_range].to_string();
293+
// test_ok duplicate_match_key_attr
294+
// match x:
295+
// case {x.a: 1, x.a: 2}: ...
296+
297+
// test_err duplicate_match_key
298+
// match x:
299+
// case {"x": 1, "x": 2}: ...
300+
// case {b"x": 1, b"x": 2}: ...
301+
// case {0: 1, 0: 2}: ...
302+
// case {1.0: 1, 1.0: 2}: ...
303+
// case {1.0 + 2j: 1, 1.0 + 2j: 2}: ...
304+
// case {True: 1, True: 2}: ...
305+
// case {None: 1, None: 2}: ...
306+
// case {
307+
// """x
308+
// y
309+
// z
310+
// """: 1,
311+
// """x
312+
// y
313+
// z
314+
// """: 2}: ...
315+
// case {"x": 1, "x": 2, "x": 3}: ...
316+
// case {0: 1, "x": 1, 0: 2, "x": 2}: ...
317+
Self::add_error(
318+
ctx,
319+
SemanticSyntaxErrorKind::DuplicateMatchKey(duplicate_key),
320+
key_range,
321+
);
322+
}
323+
}
324+
}
325+
}
326+
273327
fn irrefutable_match_case<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
274328
// test_ok irrefutable_case_pattern_at_end
275329
// match x:
@@ -514,6 +568,13 @@ impl Display for SemanticSyntaxError {
514568
write!(f, "cannot delete `__debug__` on Python {python_version} (syntax was removed in 3.9)")
515569
}
516570
},
571+
SemanticSyntaxErrorKind::DuplicateMatchKey(key) => {
572+
write!(
573+
f,
574+
"mapping pattern checks duplicate key `{}`",
575+
EscapeDefault(key)
576+
)
577+
}
517578
SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start: _ } => {
518579
write!(f, "name `{name}` is used prior to global declaration")
519580
}
@@ -634,6 +695,41 @@ pub enum SemanticSyntaxErrorKind {
634695
/// [BPO 45000]: https://github.com/python/cpython/issues/89163
635696
WriteToDebug(WriteToDebugKind),
636697

698+
/// Represents a duplicate key in a `match` mapping pattern.
699+
///
700+
/// The [CPython grammar] allows keys in mapping patterns to be literals or attribute accesses:
701+
///
702+
/// ```text
703+
/// key_value_pattern:
704+
/// | (literal_expr | attr) ':' pattern
705+
/// ```
706+
///
707+
/// But only literals are checked for duplicates:
708+
///
709+
/// ```pycon
710+
/// >>> match x:
711+
/// ... case {"x": 1, "x": 2}: ...
712+
/// ...
713+
/// File "<python-input-160>", line 2
714+
/// case {"x": 1, "x": 2}: ...
715+
/// ^^^^^^^^^^^^^^^^
716+
/// SyntaxError: mapping pattern checks duplicate key ('x')
717+
/// >>> match x:
718+
/// ... case {x.a: 1, x.a: 2}: ...
719+
/// ...
720+
/// >>>
721+
/// ```
722+
///
723+
/// ## Examples
724+
///
725+
/// ```python
726+
/// match x:
727+
/// case {"x": 1, "x": 2}: ...
728+
/// ```
729+
///
730+
/// [CPython grammar]: https://docs.python.org/3/reference/grammar.html
731+
DuplicateMatchKey(String),
732+
637733
/// Represents the use of a `global` variable before its `global` declaration.
638734
///
639735
/// ## Examples
@@ -789,6 +885,9 @@ pub trait SemanticSyntaxContext {
789885
/// The target Python version for detecting backwards-incompatible syntax changes.
790886
fn python_version(&self) -> PythonVersion;
791887

888+
/// Returns the source text under analysis.
889+
fn source(&self) -> &str;
890+
792891
/// Return the [`TextRange`] at which a name is declared as `global` in the current scope.
793892
fn global(&self, name: &str) -> Option<TextRange>;
794893

@@ -828,3 +927,20 @@ where
828927
ruff_python_ast::visitor::walk_expr(self, expr);
829928
}
830929
}
930+
931+
/// Modified version of [`std::str::EscapeDefault`] that does not escape single or double quotes.
932+
struct EscapeDefault<'a>(&'a str);
933+
934+
impl Display for EscapeDefault<'_> {
935+
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
936+
use std::fmt::Write;
937+
938+
for c in self.0.chars() {
939+
match c {
940+
'\'' | '\"' => f.write_char(c)?,
941+
_ => write!(f, "{}", c.escape_default())?,
942+
}
943+
}
944+
Ok(())
945+
}
946+
}

‎crates/ruff_python_parser/tests/fixtures.rs

+19-6
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ fn test_valid_syntax(input_path: &Path) {
8989
let parsed = parsed.try_into_module().expect("Parsed with Mode::Module");
9090

9191
let mut visitor = SemanticSyntaxCheckerVisitor::new(
92-
TestContext::default().with_python_version(options.target_version()),
92+
TestContext::new(&source).with_python_version(options.target_version()),
9393
);
9494

9595
for stmt in parsed.suite() {
@@ -185,7 +185,7 @@ fn test_invalid_syntax(input_path: &Path) {
185185
let parsed = parsed.try_into_module().expect("Parsed with Mode::Module");
186186

187187
let mut visitor = SemanticSyntaxCheckerVisitor::new(
188-
TestContext::default().with_python_version(options.target_version()),
188+
TestContext::new(&source).with_python_version(options.target_version()),
189189
);
190190

191191
for stmt in parsed.suite() {
@@ -462,21 +462,30 @@ impl<'ast> SourceOrderVisitor<'ast> for ValidateAstVisitor<'ast> {
462462
}
463463
}
464464

465-
#[derive(Debug, Default)]
466-
struct TestContext {
465+
#[derive(Debug)]
466+
struct TestContext<'a> {
467467
diagnostics: RefCell<Vec<SemanticSyntaxError>>,
468468
python_version: PythonVersion,
469+
source: &'a str,
469470
}
470471

471-
impl TestContext {
472+
impl<'a> TestContext<'a> {
473+
fn new(source: &'a str) -> Self {
474+
Self {
475+
diagnostics: RefCell::default(),
476+
python_version: PythonVersion::default(),
477+
source,
478+
}
479+
}
480+
472481
#[must_use]
473482
fn with_python_version(mut self, python_version: PythonVersion) -> Self {
474483
self.python_version = python_version;
475484
self
476485
}
477486
}
478487

479-
impl SemanticSyntaxContext for TestContext {
488+
impl SemanticSyntaxContext for TestContext<'_> {
480489
fn seen_docstring_boundary(&self) -> bool {
481490
false
482491
}
@@ -489,6 +498,10 @@ impl SemanticSyntaxContext for TestContext {
489498
self.diagnostics.borrow_mut().push(error);
490499
}
491500

501+
fn source(&self) -> &str {
502+
self.source
503+
}
504+
492505
fn global(&self, _name: &str) -> Option<TextRange> {
493506
None
494507
}

0 commit comments

Comments
 (0)