Skip to content

Commit

Permalink
Fix incorrect Parameter range for *args and **kwargs (#10283)
Browse files Browse the repository at this point in the history 
## Summary

Fix #10282 

This PR updates the Python grammar to include the `*` character in
`*args` `**kwargs` in the range of the `Parameter`
```
def f(*args, **kwargs): pass
#      ~~~~    ~~~~~~    <-- range before the PR
#     ^^^^^  ^^^^^^^^    <-- range after
```

The invalid syntax `def f(*, **kwargs): ...` is also now correctly
reported.

## Test Plan

Test cases were added to `function.rs`.
  • Loading branch information
@GtrMo
GtrMo committed Mar 8, 2024
1 parent b64f2ea commit a067d87
Show file tree
Hide file tree
Showing 23 changed files with 14,760 additions and 14,768 deletions.
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/parameter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::rules::{flake8_builtins, pep8_naming, pycodestyle};
pub(crate) fn parameter(parameter: &Parameter, checker: &mut Checker) {
if checker.enabled(Rule::AmbiguousVariableName) {
if let Some(diagnostic) =
pycodestyle::rules::ambiguous_variable_name(&parameter.name, parameter.range())
pycodestyle::rules::ambiguous_variable_name(&parameter.name, parameter.name.range())
{
checker.diagnostics.push(diagnostic);
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/flake8_builtins/rules/builtin_argument_shadowing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pub(crate) fn builtin_argument_shadowing(checker: &mut Checker, parameter: &Para
BuiltinArgumentShadowing {
name: parameter.name.to_string(),
},
parameter.range(),
parameter.name.range(),
));
}
}
8 changes: 8 additions & 0 deletions crates/ruff_python_formatter/src/comments/placement.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ fn handle_enclosed_comment<'a>(
}
})
}
AnyNodeRef::Parameter(parameter) => {
// E.g. a comment between the `*` or `**` and the parameter name.
if comment.preceding_node().is_none() || comment.following_node().is_none() {
CommentPlacement::leading(parameter, comment)
} else {
CommentPlacement::Default(comment)
}
}
AnyNodeRef::Arguments(_) | AnyNodeRef::TypeParams(_) | AnyNodeRef::PatternArguments(_) => {
handle_bracketed_end_of_line_comment(comment, locator)
}
Expand Down
28 changes: 19 additions & 9 deletions crates/ruff_python_parser/src/function.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,19 +176,27 @@ mod tests {
function_and_lambda! {
test_function_no_args: "def f(): pass",
test_function_pos_args: "def f(a, b, c): pass",
test_function_pos_args_with_defaults: "def f(a, b=20, c=30): pass",
test_function_posonly_and_pos_args: "def f(a, /, b, c): pass",
test_function_pos_args_with_defaults: "def f(a, b=20, /, c=30): pass",
test_function_pos_args_with_defaults_and_varargs_and_kwargs: "def f(a, b=20, /, c=30, *args, **kwargs): pass",
test_function_kw_only_args: "def f(*, a, b, c): pass",
test_function_kw_only_args_with_defaults: "def f(*, a, b=20, c=30): pass",
test_function_pos_and_kw_only_args: "def f(a, b, c, *, d, e, f): pass",
test_function_pos_and_kw_only_args_with_defaults: "def f(a, b, c, *, d, e=20, f=30): pass",
test_function_pos_and_kw_only_args_with_defaults_and_varargs: "def f(a, b, c, *args, d, e=20, f=30): pass",
test_function_pos_and_kw_only_args_with_defaults_and_varargs_and_kwargs: "def f(a, b, c, *args, d, e=20, f=30, **kwargs): pass",
test_function_kw_only_args_with_defaults_and_varargs: "def f(*args, a, b=20, c=30): pass",
test_function_kw_only_args_with_defaults_and_kwargs: "def f(*, a, b=20, c=30, **kwargs): pass",
test_function_kw_only_args_with_defaults_and_varargs_and_kwargs: "def f(*args, a, b=20, c=30, **kwargs): pass",
test_function_pos_and_kw_only_args: "def f(a, b, /, c, *, d, e, f): pass",
test_function_pos_and_kw_only_args_with_defaults: "def f(a, b, /, c, *, d, e=20, f=30): pass",
test_function_pos_and_kw_only_args_with_defaults_and_varargs: "def f(a, b, /, c, *args, d, e=20, f=30): pass",
test_function_pos_and_kw_only_args_with_defaults_and_kwargs: "def f(a, b, /, c, *, d, e=20, f=30, **kwargs): pass",
test_function_pos_and_kw_only_args_with_defaults_and_varargs_and_kwargs: "def f(a, b, /, c, *args, d, e=20, f=30, **kwargs): pass",
test_lambda_no_args: "lambda: 1",
test_lambda_pos_args: "lambda a, b, c: 1",
test_lambda_pos_args_with_defaults: "lambda a, b=20, c=30: 1",
test_lambda_posonly_args: "lambda a, b, /, c: 1",
test_lambda_pos_args_with_defaults: "lambda a, b=20, /, c=30: 1",
test_lambda_kw_only_args: "lambda *, a, b, c: 1",
test_lambda_kw_only_args_with_defaults: "lambda *, a, b=20, c=30: 1",
test_lambda_pos_and_kw_only_args: "lambda a, b, c, *, d, e: 0",
test_lambda_pos_and_kw_only_args: "lambda a, b, /, c, *, d, e: 0",
test_lambda_pos_and_kw_only_args_and_vararg_and_kwarg: "lambda a, b, /, c, *d, e, **f: 0",
}

fn function_parse_error(src: &str) -> LexicalErrorType {
Expand Down Expand Up @@ -219,14 +227,16 @@ mod tests {
test_duplicates_f2: "def f(a, *, a): pass", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_f3: "def f(a, a=20): pass", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_f4: "def f(a, *a): pass", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_f5: "def f(a, *, **a): pass", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_f5: "def f(a, *, b, **a): pass", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l1: "lambda a, a: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l2: "lambda a, *, a: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l3: "lambda a, a=20: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l4: "lambda a, *a: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l5: "lambda a, *, **a: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_duplicates_l5: "lambda a, *, b, **a: 1", LexicalErrorType::DuplicateArgumentError("a".to_string().into_boxed_str()),
test_default_arg_error_f: "def f(a, b=20, c): pass", LexicalErrorType::DefaultArgumentError,
test_default_arg_error_l: "lambda a, b=20, c: 1", LexicalErrorType::DefaultArgumentError,
test_named_arguments_follow_bare_star_1: "def f(*): pass", LexicalErrorType::OtherError("named arguments must follow bare *".to_string().into_boxed_str()),
test_named_arguments_follow_bare_star_2: "def f(*, **kwargs): pass", LexicalErrorType::OtherError("named arguments must follow bare *".to_string().into_boxed_str()),

// Check some calls.
test_positional_arg_error_f: "f(b=20, c)", LexicalErrorType::PositionalArgumentError,
Expand Down
66 changes: 23 additions & 43 deletions crates/ruff_python_parser/src/python.lalrpop
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1151,24 +1151,6 @@ ParameterList<ParameterType, StarParameterType, DoubleStarParameterType>: ast::P
range: (location..end_location).into()
})
},
<location:@L> <param1:ParameterDefs<ParameterType>> <kw:("," <KwargParameter<DoubleStarParameterType>>)> ","? <end_location:@R> =>? {
validate_pos_params(&param1)?;
let (posonlyargs, args) = param1;

// Now gather rest of parameters:
let vararg = None;
let kwonlyargs = vec![];
let kwarg = kw;

Ok(ast::Parameters {
posonlyargs,
args,
kwonlyargs,
vararg,
kwarg,
range: (location..end_location).into()
})
},
<location:@L> <params:ParameterListStarArgs<ParameterType, StarParameterType, DoubleStarParameterType>> ","? <end_location:@R> => {
let (vararg, kwonlyargs, kwarg) = params;
ast::Parameters {
Expand All @@ -1180,16 +1162,6 @@ ParameterList<ParameterType, StarParameterType, DoubleStarParameterType>: ast::P
range: (location..end_location).into()
}
},
<location:@L> <kwarg:KwargParameter<DoubleStarParameterType>> ","? <end_location:@R> => {
ast::Parameters {
posonlyargs: vec![],
args: vec![],
kwonlyargs: vec![],
vararg: None,
kwarg,
range: (location..end_location).into()
}
},
};

// Use inline here to make sure the "," is not creating an ambiguity.
Expand Down Expand Up @@ -1219,7 +1191,11 @@ UntypedParameter: ast::ParameterWithDefault = {
},
};
StarUntypedParameter: ast::Parameter = {
<location:@L> <arg:Identifier> <end_location:@R> => ast::Parameter { name:arg, annotation: None, range: (location..end_location).into() },
<location:@L> "*" <arg:Identifier> <end_location:@R> => ast::Parameter { name:arg, annotation: None, range: (location..end_location).into() },
};

DoubleStarUntypedParameter: ast::Parameter = {
<location:@L> "**" <arg:Identifier> <end_location:@R> => ast::Parameter { name:arg, annotation: None, range: (location..end_location).into() },
};

TypedParameter: ast::ParameterWithDefault = {
Expand All @@ -1231,14 +1207,14 @@ TypedParameter: ast::ParameterWithDefault = {
};

StarTypedParameter: ast::Parameter = {
<location:@L> <name:Identifier> <annotation:(":" <TestOrStarExpr>)?> <end_location:@R> => {
<location:@L> "*" <name:Identifier> <annotation:(":" <TestOrStarExpr>)?> <end_location:@R> => {
let annotation = annotation.map(ast::Expr::from).map(Box::new);
ast::Parameter { name, annotation, range: (location..end_location).into() }
},
};

DoubleStarTypedParameter: ast::Parameter = {
<location:@L> <name:Identifier> <annotation:(":" <Test<"all">>)?> <end_location:@R> => {
<location:@L> "**" <name:Identifier> <annotation:(":" <Test<"all">>)?> <end_location:@R> => {
let annotation = annotation.map(ast::Expr::from).map(Box::new);
ast::Parameter { name, annotation, range: (location..end_location).into() }
},
Expand All @@ -1248,25 +1224,29 @@ DoubleStarTypedParameter: ast::Parameter = {
// TODO: figure out another grammar that makes this inline no longer required.
#[inline]
ParameterListStarArgs<ParameterType, StarParameterType, DoubleStarParameterType>: (Option<Box<ast::Parameter>>, Vec<ast::ParameterWithDefault>, Option<Box<ast::Parameter>>) = {
<location:@L> "*" <va:StarParameterType?> <kwonlyargs:("," <ParameterDef<ParameterType>>)*> <kwarg:("," <KwargParameter<DoubleStarParameterType>>)?> =>? {
if va.is_none() && kwonlyargs.is_empty() && kwarg.is_none() {
<location:@L> <va:StarParameterType> <kwonlyargs:("," <ParameterDef<ParameterType>>)*> <kwarg:("," <DoubleStarParameterType>)?> =>? {
let kwarg = kwarg.map(Box::new);
let va = Some(Box::new(va));

Ok((va, kwonlyargs, kwarg))
},
<location:@L> "*" <kwonlyargs:("," <ParameterDef<ParameterType>>)*> <kwarg:("," <DoubleStarParameterType>)?> =>? {
if kwonlyargs.is_empty() {
return Err(LexicalError::new(
LexicalErrorType::OtherError("named arguments must follow bare *".to_string().into_boxed_str()),
location,
))?;
}

let kwarg = kwarg.flatten();
let va = va.map(Box::new);
let kwarg = kwarg.map(Box::new);

Ok((va, kwonlyargs, kwarg))
}
};
Ok((None, kwonlyargs, kwarg))
},
<location:@L> <kwarg:(<DoubleStarParameterType>)> =>? {
let kwarg = Some(Box::new(kwarg));

KwargParameter<ParameterType>: Option<Box<ast::Parameter>> = {
"**" <kwarg:ParameterType?> => {
kwarg.map(Box::new)
}
Ok((None, vec![], kwarg))
},
};

ClassDef: ast::Stmt = {
Expand Down Expand Up @@ -1365,7 +1345,7 @@ NamedExpression: crate::parser::ParenthesizedExpr = {
};

LambdaDef: crate::parser::ParenthesizedExpr = {
<location:@L> "lambda" <location_args:@L> <parameters:ParameterList<UntypedParameter, StarUntypedParameter, StarUntypedParameter>?> <end_location_args:@R> ":" <fstring_middle:fstring_middle?> <body:Test<"all">> <end_location:@R> =>? {
<location:@L> "lambda" <location_args:@L> <parameters:ParameterList<UntypedParameter, StarUntypedParameter, DoubleStarUntypedParameter>?> <end_location_args:@R> ":" <fstring_middle:fstring_middle?> <body:Test<"all">> <end_location:@R> =>? {
if fstring_middle.is_some() {
return Err(LexicalError::new(
LexicalErrorType::FStringError(FStringErrorType::LambdaWithoutParentheses),
Expand Down

0 comments on commit a067d87

Please sign in to comment.