mirror of
https://github.com/astral-sh/ruff
synced 2026-01-11 00:24:13 -05:00
[parser] Flag single unparenthesized generator expr with trailing comma in arguments. (#17893)
Fixes #17867 ## Summary The CPython parser does not allow generator expressions which are the sole arguments in an argument list to have a trailing comma. With this change, we start flagging such instances. ## Test Plan Added new inline tests.
This commit is contained in:
Abhijeet Prasad Bodas
committed by
GitHub
GitHub
parent
895b6161a6
commit
f5096f2050
@@ -1,2 +1,3 @@
|
||||
sum(x for x in range(10), 5)
|
||||
total(1, 2, x for x in range(5), 6)
|
||||
sum(x for x in range(10),)
|
||||
|
||||
@@ -1 +1,3 @@
|
||||
zip((x for x in range(10)), (y for y in range(10)))
|
||||
sum(x for x in range(10))
|
||||
sum((x for x in range(10)),)
|
||||
|
||||
@@ -661,117 +661,120 @@ impl<'src> Parser<'src> {
|
||||
let mut seen_keyword_argument = false; // foo = 1
|
||||
let mut seen_keyword_unpacking = false; // **foo
|
||||
|
||||
self.parse_comma_separated_list(RecoveryContextKind::Arguments, |parser| {
|
||||
let argument_start = parser.node_start();
|
||||
if parser.eat(TokenKind::DoubleStar) {
|
||||
let value = parser.parse_conditional_expression_or_higher();
|
||||
|
||||
keywords.push(ast::Keyword {
|
||||
arg: None,
|
||||
value: value.expr,
|
||||
range: parser.node_range(argument_start),
|
||||
});
|
||||
|
||||
seen_keyword_unpacking = true;
|
||||
} else {
|
||||
let start = parser.node_start();
|
||||
let mut parsed_expr = parser
|
||||
.parse_named_expression_or_higher(ExpressionContext::starred_conditional());
|
||||
|
||||
match parser.current_token_kind() {
|
||||
TokenKind::Async | TokenKind::For => {
|
||||
if parsed_expr.is_unparenthesized_starred_expr() {
|
||||
parser.add_error(
|
||||
ParseErrorType::IterableUnpackingInComprehension,
|
||||
&parsed_expr,
|
||||
);
|
||||
}
|
||||
|
||||
parsed_expr = Expr::Generator(parser.parse_generator_expression(
|
||||
parsed_expr.expr,
|
||||
start,
|
||||
Parenthesized::No,
|
||||
))
|
||||
.into();
|
||||
}
|
||||
_ => {
|
||||
if seen_keyword_unpacking && parsed_expr.is_unparenthesized_starred_expr() {
|
||||
parser.add_error(
|
||||
ParseErrorType::InvalidArgumentUnpackingOrder,
|
||||
&parsed_expr,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let arg_range = parser.node_range(start);
|
||||
if parser.eat(TokenKind::Equal) {
|
||||
seen_keyword_argument = true;
|
||||
let arg = if let ParsedExpr {
|
||||
expr: Expr::Name(ident_expr),
|
||||
is_parenthesized,
|
||||
} = parsed_expr
|
||||
{
|
||||
// test_ok parenthesized_kwarg_py37
|
||||
// # parse_options: {"target-version": "3.7"}
|
||||
// f((a)=1)
|
||||
|
||||
// test_err parenthesized_kwarg_py38
|
||||
// # parse_options: {"target-version": "3.8"}
|
||||
// f((a)=1)
|
||||
// f((a) = 1)
|
||||
// f( ( a ) = 1)
|
||||
|
||||
if is_parenthesized {
|
||||
parser.add_unsupported_syntax_error(
|
||||
UnsupportedSyntaxErrorKind::ParenthesizedKeywordArgumentName,
|
||||
arg_range,
|
||||
);
|
||||
}
|
||||
|
||||
ast::Identifier {
|
||||
id: ident_expr.id,
|
||||
range: ident_expr.range,
|
||||
}
|
||||
} else {
|
||||
// TODO(dhruvmanila): Parser shouldn't drop the `parsed_expr` if it's
|
||||
// not a name expression. We could add the expression into `args` but
|
||||
// that means the error is a missing comma instead.
|
||||
parser.add_error(
|
||||
ParseErrorType::OtherError("Expected a parameter name".to_string()),
|
||||
&parsed_expr,
|
||||
);
|
||||
ast::Identifier {
|
||||
id: Name::empty(),
|
||||
range: parsed_expr.range(),
|
||||
}
|
||||
};
|
||||
|
||||
let has_trailing_comma =
|
||||
self.parse_comma_separated_list(RecoveryContextKind::Arguments, |parser| {
|
||||
let argument_start = parser.node_start();
|
||||
if parser.eat(TokenKind::DoubleStar) {
|
||||
let value = parser.parse_conditional_expression_or_higher();
|
||||
|
||||
keywords.push(ast::Keyword {
|
||||
arg: Some(arg),
|
||||
arg: None,
|
||||
value: value.expr,
|
||||
range: parser.node_range(argument_start),
|
||||
});
|
||||
|
||||
seen_keyword_unpacking = true;
|
||||
} else {
|
||||
if !parsed_expr.is_unparenthesized_starred_expr() {
|
||||
if seen_keyword_unpacking {
|
||||
parser.add_error(
|
||||
ParseErrorType::PositionalAfterKeywordUnpacking,
|
||||
&parsed_expr,
|
||||
);
|
||||
} else if seen_keyword_argument {
|
||||
parser.add_error(
|
||||
ParseErrorType::PositionalAfterKeywordArgument,
|
||||
&parsed_expr,
|
||||
);
|
||||
let start = parser.node_start();
|
||||
let mut parsed_expr = parser
|
||||
.parse_named_expression_or_higher(ExpressionContext::starred_conditional());
|
||||
|
||||
match parser.current_token_kind() {
|
||||
TokenKind::Async | TokenKind::For => {
|
||||
if parsed_expr.is_unparenthesized_starred_expr() {
|
||||
parser.add_error(
|
||||
ParseErrorType::IterableUnpackingInComprehension,
|
||||
&parsed_expr,
|
||||
);
|
||||
}
|
||||
|
||||
parsed_expr = Expr::Generator(parser.parse_generator_expression(
|
||||
parsed_expr.expr,
|
||||
start,
|
||||
Parenthesized::No,
|
||||
))
|
||||
.into();
|
||||
}
|
||||
_ => {
|
||||
if seen_keyword_unpacking
|
||||
&& parsed_expr.is_unparenthesized_starred_expr()
|
||||
{
|
||||
parser.add_error(
|
||||
ParseErrorType::InvalidArgumentUnpackingOrder,
|
||||
&parsed_expr,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
args.push(parsed_expr.expr);
|
||||
|
||||
let arg_range = parser.node_range(start);
|
||||
if parser.eat(TokenKind::Equal) {
|
||||
seen_keyword_argument = true;
|
||||
let arg = if let ParsedExpr {
|
||||
expr: Expr::Name(ident_expr),
|
||||
is_parenthesized,
|
||||
} = parsed_expr
|
||||
{
|
||||
// test_ok parenthesized_kwarg_py37
|
||||
// # parse_options: {"target-version": "3.7"}
|
||||
// f((a)=1)
|
||||
|
||||
// test_err parenthesized_kwarg_py38
|
||||
// # parse_options: {"target-version": "3.8"}
|
||||
// f((a)=1)
|
||||
// f((a) = 1)
|
||||
// f( ( a ) = 1)
|
||||
|
||||
if is_parenthesized {
|
||||
parser.add_unsupported_syntax_error(
|
||||
UnsupportedSyntaxErrorKind::ParenthesizedKeywordArgumentName,
|
||||
arg_range,
|
||||
);
|
||||
}
|
||||
|
||||
ast::Identifier {
|
||||
id: ident_expr.id,
|
||||
range: ident_expr.range,
|
||||
}
|
||||
} else {
|
||||
// TODO(dhruvmanila): Parser shouldn't drop the `parsed_expr` if it's
|
||||
// not a name expression. We could add the expression into `args` but
|
||||
// that means the error is a missing comma instead.
|
||||
parser.add_error(
|
||||
ParseErrorType::OtherError("Expected a parameter name".to_string()),
|
||||
&parsed_expr,
|
||||
);
|
||||
ast::Identifier {
|
||||
id: Name::empty(),
|
||||
range: parsed_expr.range(),
|
||||
}
|
||||
};
|
||||
|
||||
let value = parser.parse_conditional_expression_or_higher();
|
||||
|
||||
keywords.push(ast::Keyword {
|
||||
arg: Some(arg),
|
||||
value: value.expr,
|
||||
range: parser.node_range(argument_start),
|
||||
});
|
||||
} else {
|
||||
if !parsed_expr.is_unparenthesized_starred_expr() {
|
||||
if seen_keyword_unpacking {
|
||||
parser.add_error(
|
||||
ParseErrorType::PositionalAfterKeywordUnpacking,
|
||||
&parsed_expr,
|
||||
);
|
||||
} else if seen_keyword_argument {
|
||||
parser.add_error(
|
||||
ParseErrorType::PositionalAfterKeywordArgument,
|
||||
&parsed_expr,
|
||||
);
|
||||
}
|
||||
}
|
||||
args.push(parsed_expr.expr);
|
||||
}
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
self.expect(TokenKind::Rpar);
|
||||
|
||||
@@ -781,7 +784,7 @@ impl<'src> Parser<'src> {
|
||||
keywords: keywords.into_boxed_slice(),
|
||||
};
|
||||
|
||||
self.validate_arguments(&arguments);
|
||||
self.validate_arguments(&arguments, has_trailing_comma);
|
||||
|
||||
arguments
|
||||
}
|
||||
@@ -2521,9 +2524,9 @@ impl<'src> Parser<'src> {
|
||||
|
||||
/// Performs the following validations on the function call arguments:
|
||||
/// 1. There aren't any duplicate keyword argument
|
||||
/// 2. If there are more than one argument (positional or keyword), all generator expressions
|
||||
/// present should be parenthesized.
|
||||
fn validate_arguments(&mut self, arguments: &ast::Arguments) {
|
||||
/// 2. If there are more than one argument (positional or keyword) or a single argument with a
|
||||
/// trailing comma, all generator expressions present should be parenthesized.
|
||||
fn validate_arguments(&mut self, arguments: &ast::Arguments, has_trailing_comma: bool) {
|
||||
let mut all_arg_names =
|
||||
FxHashSet::with_capacity_and_hasher(arguments.keywords.len(), FxBuildHasher);
|
||||
|
||||
@@ -2541,7 +2544,7 @@ impl<'src> Parser<'src> {
|
||||
}
|
||||
}
|
||||
|
||||
if arguments.len() > 1 {
|
||||
if has_trailing_comma || arguments.len() > 1 {
|
||||
for arg in &*arguments.args {
|
||||
if let Some(ast::ExprGenerator {
|
||||
range,
|
||||
@@ -2550,11 +2553,14 @@ impl<'src> Parser<'src> {
|
||||
}) = arg.as_generator_expr()
|
||||
{
|
||||
// test_ok args_unparenthesized_generator
|
||||
// zip((x for x in range(10)), (y for y in range(10)))
|
||||
// sum(x for x in range(10))
|
||||
// sum((x for x in range(10)),)
|
||||
|
||||
// test_err args_unparenthesized_generator
|
||||
// sum(x for x in range(10), 5)
|
||||
// total(1, 2, x for x in range(5), 6)
|
||||
// sum(x for x in range(10),)
|
||||
self.add_error(ParseErrorType::UnparenthesizedGeneratorExpression, range);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -539,17 +539,19 @@ impl<'src> Parser<'src> {
|
||||
}
|
||||
|
||||
/// Parses a comma separated list of elements where each element is parsed
|
||||
/// sing the given `parse_element` function.
|
||||
/// using the given `parse_element` function.
|
||||
///
|
||||
/// The difference between this function and `parse_comma_separated_list_into_vec`
|
||||
/// is that this function does not return the parsed elements. Instead, it is the
|
||||
/// caller's responsibility to handle the parsed elements. This is the reason
|
||||
/// that the `parse_element` parameter is bound to [`FnMut`] instead of [`Fn`].
|
||||
///
|
||||
/// Returns `true` if there is a trailing comma present.
|
||||
fn parse_comma_separated_list(
|
||||
&mut self,
|
||||
recovery_context_kind: RecoveryContextKind,
|
||||
mut parse_element: impl FnMut(&mut Parser<'src>),
|
||||
) {
|
||||
) -> bool {
|
||||
let mut progress = ParserProgress::default();
|
||||
|
||||
let saved_context = self.recovery_context;
|
||||
@@ -659,6 +661,8 @@ impl<'src> Parser<'src> {
|
||||
}
|
||||
|
||||
self.recovery_context = saved_context;
|
||||
|
||||
trailing_comma_range.is_some()
|
||||
}
|
||||
|
||||
#[cold]
|
||||
|
||||
@@ -1,14 +1,13 @@
|
||||
---
|
||||
source: crates/ruff_python_parser/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_parser/resources/inline/err/args_unparenthesized_generator.py
|
||||
snapshot_kind: text
|
||||
---
|
||||
## AST
|
||||
|
||||
```
|
||||
Module(
|
||||
ModModule {
|
||||
range: 0..65,
|
||||
range: 0..92,
|
||||
body: [
|
||||
Expr(
|
||||
StmtExpr {
|
||||
@@ -194,6 +193,82 @@ Module(
|
||||
),
|
||||
},
|
||||
),
|
||||
Expr(
|
||||
StmtExpr {
|
||||
range: 65..91,
|
||||
value: Call(
|
||||
ExprCall {
|
||||
range: 65..91,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 65..68,
|
||||
id: Name("sum"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 68..91,
|
||||
args: [
|
||||
Generator(
|
||||
ExprGenerator {
|
||||
range: 69..89,
|
||||
elt: Name(
|
||||
ExprName {
|
||||
range: 69..70,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
generators: [
|
||||
Comprehension {
|
||||
range: 71..89,
|
||||
target: Name(
|
||||
ExprName {
|
||||
range: 75..76,
|
||||
id: Name("x"),
|
||||
ctx: Store,
|
||||
},
|
||||
),
|
||||
iter: Call(
|
||||
ExprCall {
|
||||
range: 80..89,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 80..85,
|
||||
id: Name("range"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 85..89,
|
||||
args: [
|
||||
NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 86..88,
|
||||
value: Int(
|
||||
10,
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
ifs: [],
|
||||
is_async: false,
|
||||
},
|
||||
],
|
||||
parenthesized: false,
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
},
|
||||
)
|
||||
@@ -204,6 +279,7 @@ Module(
|
||||
1 | sum(x for x in range(10), 5)
|
||||
| ^^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here
|
||||
2 | total(1, 2, x for x in range(5), 6)
|
||||
3 | sum(x for x in range(10),)
|
||||
|
|
||||
|
||||
|
||||
@@ -211,4 +287,13 @@ Module(
|
||||
1 | sum(x for x in range(10), 5)
|
||||
2 | total(1, 2, x for x in range(5), 6)
|
||||
| ^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here
|
||||
3 | sum(x for x in range(10),)
|
||||
|
|
||||
|
||||
|
||||
|
|
||||
1 | sum(x for x in range(10), 5)
|
||||
2 | total(1, 2, x for x in range(5), 6)
|
||||
3 | sum(x for x in range(10),)
|
||||
| ^^^^^^^^^^^^^^^^^^^^ Syntax Error: Unparenthesized generator expression cannot be used here
|
||||
|
|
||||
|
||||
@@ -1,67 +1,195 @@
|
||||
---
|
||||
source: crates/ruff_python_parser/tests/fixtures.rs
|
||||
input_file: crates/ruff_python_parser/resources/inline/ok/args_unparenthesized_generator.py
|
||||
snapshot_kind: text
|
||||
---
|
||||
## AST
|
||||
|
||||
```
|
||||
Module(
|
||||
ModModule {
|
||||
range: 0..26,
|
||||
range: 0..107,
|
||||
body: [
|
||||
Expr(
|
||||
StmtExpr {
|
||||
range: 0..25,
|
||||
range: 0..51,
|
||||
value: Call(
|
||||
ExprCall {
|
||||
range: 0..25,
|
||||
range: 0..51,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 0..3,
|
||||
id: Name("sum"),
|
||||
id: Name("zip"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 3..25,
|
||||
range: 3..51,
|
||||
args: [
|
||||
Generator(
|
||||
ExprGenerator {
|
||||
range: 4..24,
|
||||
range: 4..26,
|
||||
elt: Name(
|
||||
ExprName {
|
||||
range: 4..5,
|
||||
range: 5..6,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
generators: [
|
||||
Comprehension {
|
||||
range: 6..24,
|
||||
range: 7..25,
|
||||
target: Name(
|
||||
ExprName {
|
||||
range: 10..11,
|
||||
range: 11..12,
|
||||
id: Name("x"),
|
||||
ctx: Store,
|
||||
},
|
||||
),
|
||||
iter: Call(
|
||||
ExprCall {
|
||||
range: 15..24,
|
||||
range: 16..25,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 15..20,
|
||||
range: 16..21,
|
||||
id: Name("range"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 20..24,
|
||||
range: 21..25,
|
||||
args: [
|
||||
NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 21..23,
|
||||
range: 22..24,
|
||||
value: Int(
|
||||
10,
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
ifs: [],
|
||||
is_async: false,
|
||||
},
|
||||
],
|
||||
parenthesized: true,
|
||||
},
|
||||
),
|
||||
Generator(
|
||||
ExprGenerator {
|
||||
range: 28..50,
|
||||
elt: Name(
|
||||
ExprName {
|
||||
range: 29..30,
|
||||
id: Name("y"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
generators: [
|
||||
Comprehension {
|
||||
range: 31..49,
|
||||
target: Name(
|
||||
ExprName {
|
||||
range: 35..36,
|
||||
id: Name("y"),
|
||||
ctx: Store,
|
||||
},
|
||||
),
|
||||
iter: Call(
|
||||
ExprCall {
|
||||
range: 40..49,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 40..45,
|
||||
id: Name("range"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 45..49,
|
||||
args: [
|
||||
NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 46..48,
|
||||
value: Int(
|
||||
10,
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
ifs: [],
|
||||
is_async: false,
|
||||
},
|
||||
],
|
||||
parenthesized: true,
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
Expr(
|
||||
StmtExpr {
|
||||
range: 52..77,
|
||||
value: Call(
|
||||
ExprCall {
|
||||
range: 52..77,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 52..55,
|
||||
id: Name("sum"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 55..77,
|
||||
args: [
|
||||
Generator(
|
||||
ExprGenerator {
|
||||
range: 56..76,
|
||||
elt: Name(
|
||||
ExprName {
|
||||
range: 56..57,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
generators: [
|
||||
Comprehension {
|
||||
range: 58..76,
|
||||
target: Name(
|
||||
ExprName {
|
||||
range: 62..63,
|
||||
id: Name("x"),
|
||||
ctx: Store,
|
||||
},
|
||||
),
|
||||
iter: Call(
|
||||
ExprCall {
|
||||
range: 67..76,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 67..72,
|
||||
id: Name("range"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 72..76,
|
||||
args: [
|
||||
NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 73..75,
|
||||
value: Int(
|
||||
10,
|
||||
),
|
||||
@@ -86,6 +214,82 @@ Module(
|
||||
),
|
||||
},
|
||||
),
|
||||
Expr(
|
||||
StmtExpr {
|
||||
range: 78..106,
|
||||
value: Call(
|
||||
ExprCall {
|
||||
range: 78..106,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 78..81,
|
||||
id: Name("sum"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 81..106,
|
||||
args: [
|
||||
Generator(
|
||||
ExprGenerator {
|
||||
range: 82..104,
|
||||
elt: Name(
|
||||
ExprName {
|
||||
range: 83..84,
|
||||
id: Name("x"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
generators: [
|
||||
Comprehension {
|
||||
range: 85..103,
|
||||
target: Name(
|
||||
ExprName {
|
||||
range: 89..90,
|
||||
id: Name("x"),
|
||||
ctx: Store,
|
||||
},
|
||||
),
|
||||
iter: Call(
|
||||
ExprCall {
|
||||
range: 94..103,
|
||||
func: Name(
|
||||
ExprName {
|
||||
range: 94..99,
|
||||
id: Name("range"),
|
||||
ctx: Load,
|
||||
},
|
||||
),
|
||||
arguments: Arguments {
|
||||
range: 99..103,
|
||||
args: [
|
||||
NumberLiteral(
|
||||
ExprNumberLiteral {
|
||||
range: 100..102,
|
||||
value: Int(
|
||||
10,
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
ifs: [],
|
||||
is_async: false,
|
||||
},
|
||||
],
|
||||
parenthesized: true,
|
||||
},
|
||||
),
|
||||
],
|
||||
keywords: [],
|
||||
},
|
||||
},
|
||||
),
|
||||
},
|
||||
),
|
||||
],
|
||||
},
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user