Skip to content

Commit 24b1b1d

Browse files
ntBrentBre
authored
[syntax-errors] Duplicate attributes in match class pattern (#17186)
Summary -- Detects duplicate attributes in a `match` class pattern: ```python match x: case Class(x=1, x=2): ... ``` which are more analogous to the similar check for mapping patterns than to the multiple assignments rule. I also realized that both this and the mapping check would only work on top-level patterns, despite the possibility that they can be nested inside other patterns: ```python match x: case [{"x": 1, "x": 2}]: ... # false negative in the old version ``` and moved these checks into the recursive pattern visitor instead. I also tidied up some of the names like the `multiple_case_assignment` function and the `MultipleCaseAssignmentVisitor`, which are now doing more than checking for multiple assignments. Test Plan -- New inline tests for both classes and mappings.
1 parent 6a07dd2 commit 24b1b1d

File tree

6 files changed

+1257
-69
lines changed

6 files changed

+1257
-69
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -574,6 +574,7 @@ impl SemanticSyntaxContext for Checker<'_> {
574574
| SemanticSyntaxErrorKind::SingleStarredAssignment
575575
| SemanticSyntaxErrorKind::WriteToDebug(_)
576576
| SemanticSyntaxErrorKind::DuplicateMatchKey(_)
577+
| SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(_)
577578
| SemanticSyntaxErrorKind::InvalidStarExpression => {
578579
if self.settings.preview.is_enabled() {
579580
self.semantic_errors.borrow_mut().push(error);

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
match x:
2+
case Class(x=1, x=2): ...
3+
case [Class(x=1, x=2)]: ...
4+
case {"x": x, "y": Foo(x=1, x=2)}: ...
5+
case [{}, {"x": x, "y": Foo(x=1, x=2)}]: ...
6+
case Class(x=1, d={"x": 1, "x": 2}, other=Class(x=1, x=2)): ...

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,6 @@
1717
""": 2}: ...
1818
case {"x": 1, "x": 2, "x": 3}: ...
1919
case {0: 1, "x": 1, 0: 2, "x": 2}: ...
20+
case [{"x": 1, "x": 2}]: ...
21+
case Foo(x=1, y={"x": 1, "x": 2}): ...
22+
case [Foo(x=1), Foo(x=1, y={"x": 1, "x": 2})]: ...

‎crates/ruff_python_parser/src/semantic_errors.rs‎

Lines changed: 91 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,13 @@ impl SemanticSyntaxChecker {
6565
}
6666
Stmt::Match(match_stmt) => {
6767
Self::irrefutable_match_case(match_stmt, ctx);
68-
Self::multiple_case_assignment(match_stmt, ctx);
69-
Self::duplicate_match_mapping_keys(match_stmt, ctx);
68+
for case in &match_stmt.cases {
69+
let mut visitor = MatchPatternVisitor {
70+
names: FxHashSet::default(),
71+
ctx,
72+
};
73+
visitor.visit_pattern(&case.pattern);
74+
}
7075
}
7176
Stmt::FunctionDef(ast::StmtFunctionDef { type_params, .. })
7277
| Stmt::ClassDef(ast::StmtClassDef { type_params, .. })
@@ -262,68 +267,6 @@ impl SemanticSyntaxChecker {
262267
}
263268
}
264269

265-
fn multiple_case_assignment<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
266-
for case in &stmt.cases {
267-
let mut visitor = MultipleCaseAssignmentVisitor {
268-
names: FxHashSet::default(),
269-
ctx,
270-
};
271-
visitor.visit_pattern(&case.pattern);
272-
}
273-
}
274-
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-
327270
fn irrefutable_match_case<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
328271
// test_ok irrefutable_case_pattern_at_end
329272
// match x:
@@ -575,6 +518,9 @@ impl Display for SemanticSyntaxError {
575518
EscapeDefault(key)
576519
)
577520
}
521+
SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(name) => {
522+
write!(f, "attribute name `{name}` repeated in class pattern",)
523+
}
578524
SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start: _ } => {
579525
write!(f, "name `{name}` is used prior to global declaration")
580526
}
@@ -730,6 +676,16 @@ pub enum SemanticSyntaxErrorKind {
730676
/// [CPython grammar]: https://docs.python.org/3/reference/grammar.html
731677
DuplicateMatchKey(String),
732678

679+
/// Represents a duplicate attribute name in a `match` class pattern.
680+
///
681+
/// ## Examples
682+
///
683+
/// ```python
684+
/// match x:
685+
/// case Class(x=1, x=2): ...
686+
/// ```
687+
DuplicateMatchClassAttribute(ast::name::Name),
688+
733689
/// Represents the use of a `global` variable before its `global` declaration.
734690
///
735691
/// ## Examples
@@ -787,12 +743,12 @@ impl Visitor<'_> for ReboundComprehensionVisitor<'_> {
787743
}
788744
}
789745

790-
struct MultipleCaseAssignmentVisitor<'a, Ctx> {
746+
struct MatchPatternVisitor<'a, Ctx> {
791747
names: FxHashSet<&'a ast::name::Name>,
792748
ctx: &'a Ctx,
793749
}
794750

795-
impl<'a, Ctx: SemanticSyntaxContext> MultipleCaseAssignmentVisitor<'a, Ctx> {
751+
impl<'a, Ctx: SemanticSyntaxContext> MatchPatternVisitor<'a, Ctx> {
796752
fn visit_pattern(&mut self, pattern: &'a Pattern) {
797753
// test_ok class_keyword_in_case_pattern
798754
// match 2:
@@ -821,19 +777,87 @@ impl<'a, Ctx: SemanticSyntaxContext> MultipleCaseAssignmentVisitor<'a, Ctx> {
821777
self.visit_pattern(pattern);
822778
}
823779
}
824-
Pattern::MatchMapping(ast::PatternMatchMapping { patterns, rest, .. }) => {
780+
Pattern::MatchMapping(ast::PatternMatchMapping {
781+
keys,
782+
patterns,
783+
rest,
784+
..
785+
}) => {
825786
for pattern in patterns {
826787
self.visit_pattern(pattern);
827788
}
828789
if let Some(rest) = rest {
829790
self.insert(rest);
830791
}
792+
793+
let mut seen = FxHashSet::default();
794+
for key in keys
795+
.iter()
796+
// complex numbers (`1 + 2j`) are allowed as keys but are not literals
797+
// because they are represented as a `BinOp::Add` between a real number and
798+
// an imaginary number
799+
.filter(|key| key.is_literal_expr() || key.is_bin_op_expr())
800+
{
801+
if !seen.insert(ComparableExpr::from(key)) {
802+
let key_range = key.range();
803+
let duplicate_key = self.ctx.source()[key_range].to_string();
804+
// test_ok duplicate_match_key_attr
805+
// match x:
806+
// case {x.a: 1, x.a: 2}: ...
807+
808+
// test_err duplicate_match_key
809+
// match x:
810+
// case {"x": 1, "x": 2}: ...
811+
// case {b"x": 1, b"x": 2}: ...
812+
// case {0: 1, 0: 2}: ...
813+
// case {1.0: 1, 1.0: 2}: ...
814+
// case {1.0 + 2j: 1, 1.0 + 2j: 2}: ...
815+
// case {True: 1, True: 2}: ...
816+
// case {None: 1, None: 2}: ...
817+
// case {
818+
// """x
819+
// y
820+
// z
821+
// """: 1,
822+
// """x
823+
// y
824+
// z
825+
// """: 2}: ...
826+
// case {"x": 1, "x": 2, "x": 3}: ...
827+
// case {0: 1, "x": 1, 0: 2, "x": 2}: ...
828+
// case [{"x": 1, "x": 2}]: ...
829+
// case Foo(x=1, y={"x": 1, "x": 2}): ...
830+
// case [Foo(x=1), Foo(x=1, y={"x": 1, "x": 2})]: ...
831+
SemanticSyntaxChecker::add_error(
832+
self.ctx,
833+
SemanticSyntaxErrorKind::DuplicateMatchKey(duplicate_key),
834+
key_range,
835+
);
836+
}
837+
}
831838
}
832839
Pattern::MatchClass(ast::PatternMatchClass { arguments, .. }) => {
833840
for pattern in &arguments.patterns {
834841
self.visit_pattern(pattern);
835842
}
843+
let mut seen = FxHashSet::default();
836844
for keyword in &arguments.keywords {
845+
if !seen.insert(&keyword.attr.id) {
846+
// test_err duplicate_match_class_attr
847+
// match x:
848+
// case Class(x=1, x=2): ...
849+
// case [Class(x=1, x=2)]: ...
850+
// case {"x": x, "y": Foo(x=1, x=2)}: ...
851+
// case [{}, {"x": x, "y": Foo(x=1, x=2)}]: ...
852+
// case Class(x=1, d={"x": 1, "x": 2}, other=Class(x=1, x=2)): ...
853+
SemanticSyntaxChecker::add_error(
854+
self.ctx,
855+
SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(
856+
keyword.attr.id.clone(),
857+
),
858+
keyword.attr.range,
859+
);
860+
}
837861
self.visit_pattern(&keyword.pattern);
838862
}
839863
}

0 commit comments

Comments
 (0)