mirror of
https://github.com/astral-sh/ruff.git
synced 2025-08-04 10:48:32 +00:00
[syntax-errors] Detect duplicate keys in match mapping patterns (#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.
This commit is contained in:
Brent Westbrook
GitHub
parent
ca0cce3f9c
commit
6e2b8f9696
7 changed files with 1298 additions and 6 deletions
|
|
@ -0,0 +1,19 @@
|
|||
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}: ...
|
||||
|
|
@ -0,0 +1,2 @@
|
|||
match x:
|
||||
case {x.a: 1, x.a: 2}: ...
|
||||
|
|
@ -8,6 +8,7 @@ use std::fmt::Display;
|
|||
|
||||
use ruff_python_ast::{
|
||||
self as ast,
|
||||
comparable::ComparableExpr,
|
||||
visitor::{walk_expr, Visitor},
|
||||
Expr, ExprContext, IrrefutablePatternKind, Pattern, PythonVersion, Stmt, StmtExpr,
|
||||
StmtImportFrom,
|
||||
|
|
@ -65,6 +66,7 @@ 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);
|
||||
}
|
||||
Stmt::FunctionDef(ast::StmtFunctionDef { type_params, .. })
|
||||
| Stmt::ClassDef(ast::StmtClassDef { type_params, .. })
|
||||
|
|
@ -270,6 +272,58 @@ impl SemanticSyntaxChecker {
|
|||
}
|
||||
}
|
||||
|
||||
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:
|
||||
|
|
@ -514,6 +568,13 @@ impl Display for SemanticSyntaxError {
|
|||
write!(f, "cannot delete `__debug__` on Python {python_version} (syntax was removed in 3.9)")
|
||||
}
|
||||
},
|
||||
SemanticSyntaxErrorKind::DuplicateMatchKey(key) => {
|
||||
write!(
|
||||
f,
|
||||
"mapping pattern checks duplicate key `{}`",
|
||||
EscapeDefault(key)
|
||||
)
|
||||
}
|
||||
SemanticSyntaxErrorKind::LoadBeforeGlobalDeclaration { name, start: _ } => {
|
||||
write!(f, "name `{name}` is used prior to global declaration")
|
||||
}
|
||||
|
|
@ -634,6 +695,41 @@ pub enum SemanticSyntaxErrorKind {
|
|||
/// [BPO 45000]: https://github.com/python/cpython/issues/89163
|
||||
WriteToDebug(WriteToDebugKind),
|
||||
|
||||
/// Represents a duplicate key in a `match` mapping pattern.
|
||||
///
|
||||
/// The [CPython grammar] allows keys in mapping patterns to be literals or attribute accesses:
|
||||
///
|
||||
/// ```text
|
||||
/// key_value_pattern:
|
||||
/// | (literal_expr | attr) ':' pattern
|
||||
/// ```
|
||||
///
|
||||
/// But only literals are checked for duplicates:
|
||||
///
|
||||
/// ```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}: ...
|
||||
/// ...
|
||||
/// >>>
|
||||
/// ```
|
||||
///
|
||||
/// ## Examples
|
||||
///
|
||||
/// ```python
|
||||
/// match x:
|
||||
/// case {"x": 1, "x": 2}: ...
|
||||
/// ```
|
||||
///
|
||||
/// [CPython grammar]: https://docs.python.org/3/reference/grammar.html
|
||||
DuplicateMatchKey(String),
|
||||
|
||||
/// Represents the use of a `global` variable before its `global` declaration.
|
||||
///
|
||||
/// ## Examples
|
||||
|
|
@ -789,6 +885,9 @@ pub trait SemanticSyntaxContext {
|
|||
/// The target Python version for detecting backwards-incompatible syntax changes.
|
||||
fn python_version(&self) -> PythonVersion;
|
||||
|
||||
/// Returns the source text under analysis.
|
||||
fn source(&self) -> &str;
|
||||
|
||||
/// Return the [`TextRange`] at which a name is declared as `global` in the current scope.
|
||||
fn global(&self, name: &str) -> Option<TextRange>;
|
||||
|
||||
|
|
@ -828,3 +927,20 @@ where
|
|||
ruff_python_ast::visitor::walk_expr(self, expr);
|
||||
}
|
||||
}
|
||||
|
||||
/// Modified version of [`std::str::EscapeDefault`] that does not escape single or double quotes.
|
||||
struct EscapeDefault<'a>(&'a str);
|
||||
|
||||
impl Display for EscapeDefault<'_> {
|
||||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
|
||||
use std::fmt::Write;
|
||||
|
||||
for c in self.0.chars() {
|
||||
match c {
|
||||
'\'' | '\"' => f.write_char(c)?,
|
||||
_ => write!(f, "{}", c.escape_default())?,
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -89,7 +89,7 @@ fn test_valid_syntax(input_path: &Path) {
|
|||
let parsed = parsed.try_into_module().expect("Parsed with Mode::Module");
|
||||
|
||||
let mut visitor = SemanticSyntaxCheckerVisitor::new(
|
||||
TestContext::default().with_python_version(options.target_version()),
|
||||
TestContext::new(&source).with_python_version(options.target_version()),
|
||||
);
|
||||
|
||||
for stmt in parsed.suite() {
|
||||
|
|
@ -185,7 +185,7 @@ fn test_invalid_syntax(input_path: &Path) {
|
|||
let parsed = parsed.try_into_module().expect("Parsed with Mode::Module");
|
||||
|
||||
let mut visitor = SemanticSyntaxCheckerVisitor::new(
|
||||
TestContext::default().with_python_version(options.target_version()),
|
||||
TestContext::new(&source).with_python_version(options.target_version()),
|
||||
);
|
||||
|
||||
for stmt in parsed.suite() {
|
||||
|
|
@ -462,13 +462,22 @@ impl<'ast> SourceOrderVisitor<'ast> for ValidateAstVisitor<'ast> {
|
|||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
struct TestContext {
|
||||
#[derive(Debug)]
|
||||
struct TestContext<'a> {
|
||||
diagnostics: RefCell<Vec<SemanticSyntaxError>>,
|
||||
python_version: PythonVersion,
|
||||
source: &'a str,
|
||||
}
|
||||
|
||||
impl TestContext {
|
||||
impl<'a> TestContext<'a> {
|
||||
fn new(source: &'a str) -> Self {
|
||||
Self {
|
||||
diagnostics: RefCell::default(),
|
||||
python_version: PythonVersion::default(),
|
||||
source,
|
||||
}
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
fn with_python_version(mut self, python_version: PythonVersion) -> Self {
|
||||
self.python_version = python_version;
|
||||
|
|
@ -476,7 +485,7 @@ impl TestContext {
|
|||
}
|
||||
}
|
||||
|
||||
impl SemanticSyntaxContext for TestContext {
|
||||
impl SemanticSyntaxContext for TestContext<'_> {
|
||||
fn seen_docstring_boundary(&self) -> bool {
|
||||
false
|
||||
}
|
||||
|
|
@ -489,6 +498,10 @@ impl SemanticSyntaxContext for TestContext {
|
|||
self.diagnostics.borrow_mut().push(error);
|
||||
}
|
||||
|
||||
fn source(&self) -> &str {
|
||||
self.source
|
||||
}
|
||||
|
||||
fn global(&self, _name: &str) -> Option<TextRange> {
|
||||
None
|
||||
}
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load diff
|
|
@ -0,0 +1,115 @@
|
|||
---
|
||||
source: crates/ruff_python_parser/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_parser/resources/inline/ok/duplicate_match_key_attr.py
|
||||
---
|
||||
## AST
|
||||
|
||||
```
|
||||
Module(
|
||||
ModModule {
|
||||
range: 0..40,
|
||||
body: [
|
||||
Match(
|
||||
StmtMatch {
|
||||
range: 0..39,
|
||||
subject: Name(
|
||||
ExprName {
|
||||
range: 6..7,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
cases: [
|
||||
MatchCase {
|
||||
range: 13..39,
|
||||
pattern: MatchMapping(
|
||||
PatternMatchMapping {
|
||||
range: 18..34,
|
||||
keys: [
|
||||
Attribute(
|
||||
ExprAttribute {
|
||||
range: 19..22,
|
||||
value: Name(
|
||||
ExprName {
|
||||
range: 19..20,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
attr: Identifier {
|
||||
id: Name("a"),
|
||||
range: 21..22,
|
||||
},
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
Attribute(
|
||||
ExprAttribute {
|
||||
range: 27..30,
|
||||
value: Name(
|
||||
ExprName {
|
||||
range: 27..28,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
attr: Identifier {
|
||||
id: Name("a"),
|
||||
range: 29..30,
|
||||
},
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
],
|
||||
patterns: [
|
||||
MatchValue(
|
||||
PatternMatchValue {
|
||||
range: 24..25,
|
||||
value: NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 24..25,
|
||||
value: Int(
|
||||
1,
|
||||
),
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
MatchValue(
|
||||
PatternMatchValue {
|
||||
range: 32..33,
|
||||
value: NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 32..33,
|
||||
value: Int(
|
||||
2,
|
||||
),
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
rest: None,
|
||||
},
|
||||
),
|
||||
guard: None,
|
||||
body: [
|
||||
Expr(
|
||||
StmtExpr {
|
||||
range: 36..39,
|
||||
value: EllipsisLiteral(
|
||||
ExprEllipsisLiteral {
|
||||
range: 36..39,
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
},
|
||||
],
|
||||
},
|
||||
),
|
||||
],
|
||||
},
|
||||
)
|
||||
```
|
||||
Loading…
Add table
Add a link
Reference in a new issue