Skip to content
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

Fix incorrect Parameter range for *args and **kwargs #10283

Merged
merged 4 commits into from
Mar 8, 2024
Merged

Fix incorrect Parameter range for *args and **kwargs #10283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8c4a7d0
Update parser snapshots with correct range for starred parameters
GtrMo Mar 5, 2024
b79566d
Update grammar
GtrMo Mar 7, 2024
9f9dda3
`parameter.range` -> `parameter.name.range`
GtrMo Mar 7, 2024
1aeb5c1
Fix between `*` and parameter name comment handling
MichaReiser Mar 8, 2024
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
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/parameter.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading
Loading