Skip to content

[syntax-errors] Duplicate attributes in match class pattern #17186

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Apr 3, 2025
Merged

[syntax-errors] Duplicate attributes in match class pattern #17186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d0703d
[syntax-errors] Fix multiple assignment for class keyword argument
ntBre Apr 3, 2025
9007519
[syntax-errors] Duplicate attributes in `match` class pattern
ntBre Apr 3, 2025
bda7e7f
add false-negative cases that need recursion
ntBre Apr 3, 2025
b48c83e
move duplicate key checks to visitor, fixing false negatives
ntBre Apr 3, 2025
c053f4b
inline multiple_case_assignment and rename visitor
ntBre Apr 3, 2025
87bccbe
add more deeply nested test cases
ntBre Apr 3, 2025
3226d1e
Merge branch 'main' into brent/syn-duplicate-pattern-class-attribute
ntBre Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/ast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,7 @@ impl SemanticSyntaxContext for Checker<'_> {
| SemanticSyntaxErrorKind::SingleStarredAssignment
| SemanticSyntaxErrorKind::WriteToDebug(_)
| SemanticSyntaxErrorKind::DuplicateMatchKey(_)
| SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(_)
| SemanticSyntaxErrorKind::InvalidStarExpression => {
if self.settings.preview.is_enabled() {
self.semantic_errors.borrow_mut().push(error);
Expand Down
6 changes: 6 additions & 0 deletions crates/ruff_python_parser/resources/inline/err/duplicate_match_class_attr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
match x:
case Class(x=1, x=2): ...
case [Class(x=1, x=2)]: ...
case {"x": x, "y": Foo(x=1, x=2)}: ...
case [{}, {"x": x, "y": Foo(x=1, x=2)}]: ...
case Class(x=1, d={"x": 1, "x": 2}, other=Class(x=1, x=2)): ...
3 changes: 3 additions & 0 deletions crates/ruff_python_parser/resources/inline/err/duplicate_match_key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,6 @@
""": 2}: ...
case {"x": 1, "x": 2, "x": 3}: ...
case {0: 1, "x": 1, 0: 2, "x": 2}: ...
case [{"x": 1, "x": 2}]: ...
case Foo(x=1, y={"x": 1, "x": 2}): ...
case [Foo(x=1), Foo(x=1, y={"x": 1, "x": 2})]: ...
158 changes: 91 additions & 67 deletions crates/ruff_python_parser/src/semantic_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,13 @@ impl SemanticSyntaxChecker {
}
Stmt::Match(match_stmt) => {
Self::irrefutable_match_case(match_stmt, ctx);
Self::multiple_case_assignment(match_stmt, ctx);
Self::duplicate_match_mapping_keys(match_stmt, ctx);
for case in &match_stmt.cases {
let mut visitor = MatchPatternVisitor {
names: FxHashSet::default(),
ctx,
};
visitor.visit_pattern(&case.pattern);
}
}
Stmt::FunctionDef(ast::StmtFunctionDef { type_params, .. })
| Stmt::ClassDef(ast::StmtClassDef { type_params, .. })
Expand Down Expand Up @@ -262,68 +267,6 @@ impl SemanticSyntaxChecker {
}
}

fn multiple_case_assignment<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
for case in &stmt.cases {
let mut visitor = MultipleCaseAssignmentVisitor {
names: FxHashSet::default(),
ctx,
};
visitor.visit_pattern(&case.pattern);
}
}

fn duplicate_match_mapping_keys<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
for mapping in stmt
.cases
.iter()
.filter_map(|case| case.pattern.as_match_mapping())
{
let mut seen = FxHashSet::default();
for key in mapping
.keys
.iter()
// 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())
{
if !seen.insert(ComparableExpr::from(key)) {
let key_range = key.range();
let duplicate_key = ctx.source()[key_range].to_string();
// test_ok duplicate_match_key_attr
// match x:
// case {x.a: 1, x.a: 2}: ...

// test_err duplicate_match_key
// match x:
// case {"x": 1, "x": 2}: ...
// case {b"x": 1, b"x": 2}: ...
// case {0: 1, 0: 2}: ...
// case {1.0: 1, 1.0: 2}: ...
// case {1.0 + 2j: 1, 1.0 + 2j: 2}: ...
// case {True: 1, True: 2}: ...
// case {None: 1, None: 2}: ...
// case {
// """x
// y
// z
// """: 1,
// """x
// y
// z
// """: 2}: ...
// case {"x": 1, "x": 2, "x": 3}: ...
// case {0: 1, "x": 1, 0: 2, "x": 2}: ...
Self::add_error(
ctx,
SemanticSyntaxErrorKind::DuplicateMatchKey(duplicate_key),
key_range,
);
}
}
}
}

fn irrefutable_match_case<Ctx: SemanticSyntaxContext>(stmt: &ast::StmtMatch, ctx: &Ctx) {
// test_ok irrefutable_case_pattern_at_end
// match x:
Expand Down Expand Up @@ -575,6 +518,9 @@ impl Display for SemanticSyntaxError {
EscapeDefault(key)
)
}
SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(name) => {
write!(f, "attribute name `{name}` repeated in class pattern",)
}
SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start: _ } => {
write!(f, "name `{name}` is used prior to global declaration")
}
Expand Down Expand Up @@ -730,6 +676,16 @@ pub enum SemanticSyntaxErrorKind {
/// [CPython grammar]: https://docs.python.org/3/reference/grammar.html
DuplicateMatchKey(String),

/// Represents a duplicate attribute name in a `match` class pattern.
///
/// ## Examples
///
/// ```python
/// match x:
/// case Class(x=1, x=2): ...
/// ```
DuplicateMatchClassAttribute(ast::name::Name),

/// Represents the use of a `global` variable before its `global` declaration.
///
/// ## Examples
Expand Down Expand Up @@ -787,12 +743,12 @@ impl Visitor<'_> for ReboundComprehensionVisitor<'_> {
}
}

struct MultipleCaseAssignmentVisitor<'a, Ctx> {
struct MatchPatternVisitor<'a, Ctx> {
names: FxHashSet<&'a ast::name::Name>,
ctx: &'a Ctx,
}

impl<'a, Ctx: SemanticSyntaxContext> MultipleCaseAssignmentVisitor<'a, Ctx> {
impl<'a, Ctx: SemanticSyntaxContext> MatchPatternVisitor<'a, Ctx> {
fn visit_pattern(&mut self, pattern: &'a Pattern) {
// test_ok class_keyword_in_case_pattern
// match 2:
Expand Down Expand Up @@ -821,19 +777,87 @@ impl<'a, Ctx: SemanticSyntaxContext> MultipleCaseAssignmentVisitor<'a, Ctx> {
self.visit_pattern(pattern);
}
}
Pattern::MatchMapping(ast::PatternMatchMapping { patterns, rest, .. }) => {
Pattern::MatchMapping(ast::PatternMatchMapping {
keys,
patterns,
rest,
..
}) => {
for pattern in patterns {
self.visit_pattern(pattern);
}
if let Some(rest) = rest {
self.insert(rest);
}

let mut seen = FxHashSet::default();
for key in keys
.iter()
// 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())
{
if !seen.insert(ComparableExpr::from(key)) {
let key_range = key.range();
let duplicate_key = self.ctx.source()[key_range].to_string();
// test_ok duplicate_match_key_attr
// match x:
// case {x.a: 1, x.a: 2}: ...

// test_err duplicate_match_key
// match x:
// case {"x": 1, "x": 2}: ...
// case {b"x": 1, b"x": 2}: ...
// case {0: 1, 0: 2}: ...
// case {1.0: 1, 1.0: 2}: ...
// case {1.0 + 2j: 1, 1.0 + 2j: 2}: ...
// case {True: 1, True: 2}: ...
// case {None: 1, None: 2}: ...
// case {
// """x
// y
// z
// """: 1,
// """x
// y
// z
// """: 2}: ...
// case {"x": 1, "x": 2, "x": 3}: ...
// case {0: 1, "x": 1, 0: 2, "x": 2}: ...
// case [{"x": 1, "x": 2}]: ...
// case Foo(x=1, y={"x": 1, "x": 2}): ...
// case [Foo(x=1), Foo(x=1, y={"x": 1, "x": 2})]: ...
SemanticSyntaxChecker::add_error(
self.ctx,
SemanticSyntaxErrorKind::DuplicateMatchKey(duplicate_key),
key_range,
);
}
}
}
Pattern::MatchClass(ast::PatternMatchClass { arguments, .. }) => {
for pattern in &arguments.patterns {
self.visit_pattern(pattern);
}
let mut seen = FxHashSet::default();
for keyword in &arguments.keywords {
if !seen.insert(&keyword.attr.id) {
// test_err duplicate_match_class_attr
// match x:
// case Class(x=1, x=2): ...
// case [Class(x=1, x=2)]: ...
// case {"x": x, "y": Foo(x=1, x=2)}: ...
// case [{}, {"x": x, "y": Foo(x=1, x=2)}]: ...
// case Class(x=1, d={"x": 1, "x": 2}, other=Class(x=1, x=2)): ...
SemanticSyntaxChecker::add_error(
self.ctx,
SemanticSyntaxErrorKind::DuplicateMatchClassAttribute(
keyword.attr.id.clone(),
),
keyword.attr.range,
);
}
self.visit_pattern(&keyword.pattern);
}
}
Expand Down
Loading
Loading