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.

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

Conversation

GtrMo
Copy link
Contributor

@GtrMo GtrMo commented Mar 7, 2024

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.

@GtrMo GtrMo requested a review from MichaReiser as a code owner March 7, 2024 23:57
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the diff supposed to be that big? I used the same version of lalrpop (0.20.0) to update this file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, lalrpop loves to generate large diffs :(

@MichaReiser MichaReiser added the parser Related to the parser label Mar 8, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Mar 8, 2024

Changing the ranges drips over the formatter. There are multiple places where the formatter goes back to introspecting the source to identify the comment placement and I fear that the code has now been written in a way that assumes the incorrect *arg and **kwargs ranges.

I've been able to identify a problematic example:

(
    lambda # comment 1
    * # comment 2
    x: # comment 3
    x
)

But I haven't yet been successful to narrow it down where the range handling is happening. I suspect that it is related to #10281 (CC: @dhruvmanila) but haven't been able to confirm it yet.

I suspect that the relevant code is

pub(crate) fn find_parameter_separators(
contents: &str,
parameters: &Parameters,
) -> (Option<ParameterSeparator>, Option<ParameterSeparator>) {
// We only compute preceding_end and token location here since following_start depends on the
// star location, but the star location depends on slash's position
let slash = if let Some(preceding_end) = parameters.posonlyargs.last().map(Ranged::end) {
// ```text
// def f(a1=1, a2=2, /, a3, a4): pass
// ^^^^^^^^^^^ the range (defaults)
// def f(a1, a2, /, a3, a4): pass
// ^^^^^^^^^^^^ the range (no default)
// ```
let range = TextRange::new(preceding_end, parameters.end());
let mut tokens = SimpleTokenizer::new(contents, range).skip_trivia();
let comma = tokens
.next()
.expect("The function definition can't end here");
debug_assert!(comma.kind() == SimpleTokenKind::Comma, "{comma:?}");
let slash = tokens
.next()
.expect("The function definition can't end here");
debug_assert!(slash.kind() == SimpleTokenKind::Slash, "{slash:?}");
Some((preceding_end, slash.range))
} else {
None
};
// If we have a vararg we have a node that the comments attach to
let star = if parameters.vararg.is_some() {
// When the vararg is present the comments attach there and we don't need to do manual
// formatting
None
} else if let Some(first_keyword_argument) = parameters.kwonlyargs.first() {
// Check in that order:
// * `f(a, /, b, *, c)` and `f(a=1, /, b=2, *, c)`
// * `f(a, /, *, b)`
// * `f(*, b)` (else branch)
let after_parameters = parameters
.args
.last()
.map(|arg| arg.range.end())
.or(slash.map(|(_, slash)| slash.end()));
if let Some(preceding_end) = after_parameters {
let range = TextRange::new(preceding_end, parameters.end());
let mut tokens = SimpleTokenizer::new(contents, range).skip_trivia();
let comma = tokens
.next()
.expect("The function definition can't end here");
debug_assert!(comma.kind() == SimpleTokenKind::Comma, "{comma:?}");
let star = tokens
.next()
.expect("The function definition can't end here");
debug_assert!(star.kind() == SimpleTokenKind::Star, "{star:?}");
Some(ParameterSeparator {
preceding_end,
separator: star.range,
following_start: first_keyword_argument.start(),
})
} else {
let mut tokens = SimpleTokenizer::new(contents, parameters.range).skip_trivia();
let lparen_or_star = tokens
.next()
.expect("The function definition can't end here");
// In a function definition, the first token should always be a `(`; in a lambda
// definition, it _can't_ be a `(`.
let star = if lparen_or_star.kind == SimpleTokenKind::LParen {
tokens
.next()
.expect("The function definition can't end here")
} else {
lparen_or_star
};
debug_assert!(star.kind() == SimpleTokenKind::Star, "{star:?}");
Some(ParameterSeparator {
preceding_end: parameters.start(),
separator: star.range,
following_start: first_keyword_argument.start(),
})
}
} else {
None
};
// Now that we have star, compute how long slash trailing comments can go
// Check in that order:
// * `f(a, /, b)`
// * `f(a, /, *b)`
// * `f(a, /, *, b)`
// * `f(a, /)`
let slash_following_start = parameters
.args
.first()
.map(Ranged::start)
.or(parameters.vararg.as_ref().map(|first| first.start()))
.or(star.as_ref().map(|star| star.separator.start()))
.unwrap_or(parameters.end());
let slash = slash.map(|(preceding_end, slash)| ParameterSeparator {
preceding_end,
separator: slash,
following_start: slash_following_start,
});
(slash, star)
}
/// Locates positional only parameters separator `/` or the keywords only parameters
/// separator `*` comments.
///
/// ```python
/// def test(
/// a,
/// # Positional only parameters after here
/// /, # trailing positional argument comment.
/// b,
/// ):
/// pass
/// ```
/// or
/// ```python
/// def f(
/// a="",
/// # Keyword only parameters only after here
/// *, # trailing keyword argument comment.
/// b="",
/// ):
/// pass
/// ```
/// or
/// ```python
/// def f(
/// a,
/// # positional only comment, leading
/// /, # positional only comment, trailing
/// b,
/// # keyword only comment, leading
/// *, # keyword only comment, trailing
/// c,
/// ):
/// pass
/// ```
/// Notably, the following is possible:
/// ```python
/// def f32(
/// a,
/// # positional only comment, leading
/// /, # positional only comment, trailing
/// # keyword only comment, leading
/// *, # keyword only comment, trailing
/// c,
/// ):
/// pass
/// ```
///
/// ## Background
///
/// ```text
/// def f(a1, a2): pass
/// ^^^^^^ parameters (args)
/// ```
/// Use a star to separate keyword only parameters:
/// ```text
/// def f(a1, a2, *, a3, a4): pass
/// ^^^^^^ parameters (args)
/// ^^^^^^ keyword only parameters (kwargs)
/// ```
/// Use a slash to separate positional only parameters. Note that this changes the parameters left
/// of the slash while the star change the parameters right of it:
/// ```text
/// def f(a1, a2, /, a3, a4): pass
/// ^^^^^^ positional only parameters (posonlyargs)
/// ^^^^^^ parameters (args)
/// ```
/// You can combine both:
/// ```text
/// def f(a1, a2, /, a3, a4, *, a5, a6): pass
/// ^^^^^^ positional only parameters (posonlyargs)
/// ^^^^^^ parameters (args)
/// ^^^^^^ keyword only parameters (kwargs)
/// ```
/// They can all have defaults, meaning that the preceding node ends at the default instead of the
/// argument itself:
/// ```text
/// def f(a1=1, a2=2, /, a3=3, a4=4, *, a5=5, a6=6): pass
/// ^ ^ ^ ^ ^ ^ defaults
/// ^^^^^^^^^^ positional only parameters (posonlyargs)
/// ^^^^^^^^^^ parameters (args)
/// ^^^^^^^^^^ keyword only parameters (kwargs)
/// ```
/// An especially difficult case is having no regular parameters, so comments from both slash and
/// star will attach to either a2 or a3 and the next token is incorrect.
/// ```text
/// def f(a1, a2, /, *, a3, a4): pass
/// ^^^^^^ positional only parameters (posonlyargs)
/// ^^^^^^ keyword only parameters (kwargs)
/// ```
pub(crate) fn assign_argument_separator_comment_placement(
slash: Option<&ParameterSeparator>,
star: Option<&ParameterSeparator>,
comment_range: TextRange,
text_position: CommentLinePosition,
) -> Option<ArgumentSeparatorCommentLocation> {
if let Some(ParameterSeparator {
preceding_end,
separator: slash,
following_start,
}) = slash
{
// ```python
// def f(
// # start too early
// a, # not own line
// # this is the one
// /, # too late (handled later)
// b,
// )
// ```
if comment_range.start() > *preceding_end
&& comment_range.start() < slash.start()
&& text_position.is_own_line()
{
return Some(ArgumentSeparatorCommentLocation::SlashLeading);
}
// ```python
// def f(
// a,
// # too early (handled above)
// /, # this is the one
// # not end-of-line
// b,
// )
// ```
if comment_range.start() > slash.end()
&& comment_range.start() < *following_start
&& text_position.is_end_of_line()
{
return Some(ArgumentSeparatorCommentLocation::SlashTrailing);
}
}
if let Some(ParameterSeparator {
preceding_end,
separator: star,
following_start,
}) = star
{
// ```python
// def f(
// # start too early
// a, # not own line
// # this is the one
// *, # too late (handled later)
// b,
// )
// ```
if comment_range.start() > *preceding_end
&& comment_range.start() < star.start()
&& text_position.is_own_line()
{
return Some(ArgumentSeparatorCommentLocation::StarLeading);
}
// ```python
// def f(
// a,
// # too early (handled above)
// *, # this is the one
// # not end-of-line
// b,
// )
// ```
if comment_range.start() > star.end()
&& comment_range.start() < *following_start
&& text_position.is_end_of_line()
{
return Some(ArgumentSeparatorCommentLocation::StarTrailing);
}
}
None
}

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a fix for the formatter.

Looks good to me. That's a non-trivial parser change. Well done!

Would you mind rebasing and regenerating the parser (after rebase)?

@charliermarsh
Copy link
Member

I can rebase, regen, and merge this later.

@GtrMo
Copy link
Contributor Author

GtrMo commented Mar 8, 2024

Thanks for the formatter fix! I rebased and regenerated the parser

Copy link
Contributor

github-actions bot commented Mar 8, 2024

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+3218 -3218 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)

apache/airflow (+2275 -2275 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

+ airflow/api/auth/backend/default.py:38:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/default.py:38:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/default.py:38:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/default.py:38:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/deny_all.py:40:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/deny_all.py:40:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/deny_all.py:40:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/deny_all.py:40:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/kerberos_auth.py:153:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/kerberos_auth.py:153:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/kerberos_auth.py:153:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/kerberos_auth.py:153:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/session.py:41:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/session.py:41:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/session.py:41:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/session.py:41:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:102:14: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:102:16: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:108:15: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:108:17: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:114:15: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:114:17: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:121:16: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:121:18: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:128:17: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:128:19: ANN003 Missing type annotation for `**kwargs`
... 3595 additional changes omitted for rule ANN003
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:39:15: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:39:16: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/parameters.py:100:30: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/parameters.py:100:31: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:118:23: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/security.py:118:24: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:163:23: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/security.py:163:24: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:180:23: ANN002 Missing type annotation for `*args`
... 4515 additional changes omitted for project

bokeh/bokeh (+943 -943 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

+ examples/basic/data/ajax_source.py:34:26: ANN002 Missing type annotation for `*args`
- examples/basic/data/ajax_source.py:34:27: ANN002 Missing type annotation for `*args`
+ examples/basic/data/ajax_source.py:34:33: ANN003 Missing type annotation for `**kwargs`
- examples/basic/data/ajax_source.py:34:35: ANN003 Missing type annotation for `**kwargs`
+ examples/basic/data/server_sent_events_source.py:36:26: ANN002 Missing type annotation for `*args`
- examples/basic/data/server_sent_events_source.py:36:27: ANN002 Missing type annotation for `*args`
+ examples/basic/data/server_sent_events_source.py:36:33: ANN003 Missing type annotation for `**kwargs`
- examples/basic/data/server_sent_events_source.py:36:35: ANN003 Missing type annotation for `**kwargs`
+ examples/models/graphs.py:22:78: ANN003 Missing type annotation for `**kwargs`
- examples/models/graphs.py:22:80: ANN003 Missing type annotation for `**kwargs`
+ src/bokeh/core/property/struct.py:58:24: ANN003 Missing type annotation for `**fields`
- src/bokeh/core/property/struct.py:58:26: ANN003 Missing type annotation for `**fields`
... 971 additional changes omitted for rule ANN003
+ src/bokeh/core/property/validation.py:93:14: ANN002 Missing type annotation for `*args`
- src/bokeh/core/property/validation.py:93:15: ANN002 Missing type annotation for `*args`
... 1872 additional changes omitted for project

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
ANN003 4590 2295 2295 0 0
ANN002 1846 923 923 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+3218 -3218 violations, +0 -0 fixes in 2 projects; 41 projects unchanged)

apache/airflow (+2275 -2275 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/api/auth/backend/default.py:38:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/default.py:38:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/default.py:38:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/default.py:38:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/deny_all.py:40:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/deny_all.py:40:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/deny_all.py:40:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/deny_all.py:40:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/kerberos_auth.py:153:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/kerberos_auth.py:153:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/kerberos_auth.py:153:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/kerberos_auth.py:153:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api/auth/backend/session.py:41:19: ANN002 Missing type annotation for `*args`
- airflow/api/auth/backend/session.py:41:20: ANN002 Missing type annotation for `*args`
+ airflow/api/auth/backend/session.py:41:26: ANN003 Missing type annotation for `**kwargs`
- airflow/api/auth/backend/session.py:41:28: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:102:14: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:102:16: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:108:15: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:108:17: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:114:15: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:114:17: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:121:16: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:121:18: ANN003 Missing type annotation for `**kwargs`
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:128:17: ANN003 Missing type annotation for `**kwargs`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:128:19: ANN003 Missing type annotation for `**kwargs`
... 3595 additional changes omitted for rule ANN003
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:39:15: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:39:16: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/parameters.py:100:30: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/parameters.py:100:31: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:118:23: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/security.py:118:24: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:163:23: ANN002 Missing type annotation for `*args`
- airflow/api_connexion/security.py:163:24: ANN002 Missing type annotation for `*args`
+ airflow/api_connexion/security.py:180:23: ANN002 Missing type annotation for `*args`
... 4515 additional changes omitted for project

bokeh/bokeh (+943 -943 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ examples/basic/data/ajax_source.py:34:26: ANN002 Missing type annotation for `*args`
- examples/basic/data/ajax_source.py:34:27: ANN002 Missing type annotation for `*args`
+ examples/basic/data/ajax_source.py:34:33: ANN003 Missing type annotation for `**kwargs`
- examples/basic/data/ajax_source.py:34:35: ANN003 Missing type annotation for `**kwargs`
+ examples/basic/data/server_sent_events_source.py:36:26: ANN002 Missing type annotation for `*args`
- examples/basic/data/server_sent_events_source.py:36:27: ANN002 Missing type annotation for `*args`
+ examples/basic/data/server_sent_events_source.py:36:33: ANN003 Missing type annotation for `**kwargs`
- examples/basic/data/server_sent_events_source.py:36:35: ANN003 Missing type annotation for `**kwargs`
+ examples/models/graphs.py:22:78: ANN003 Missing type annotation for `**kwargs`
- examples/models/graphs.py:22:80: ANN003 Missing type annotation for `**kwargs`
+ src/bokeh/core/property/struct.py:58:24: ANN003 Missing type annotation for `**fields`
- src/bokeh/core/property/struct.py:58:26: ANN003 Missing type annotation for `**fields`
... 971 additional changes omitted for rule ANN003
+ src/bokeh/core/property/validation.py:93:14: ANN002 Missing type annotation for `*args`
- src/bokeh/core/property/validation.py:93:15: ANN002 Missing type annotation for `*args`
... 1872 additional changes omitted for project

Changes by rule (2 rules affected)

code total + violation - violation + fix - fix
ANN003 4590 2295 2295 0 0
ANN002 1846 923 923 0 0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@charliermarsh charliermarsh merged commit a067d87 into astral-sh:main Mar 8, 2024
17 checks passed
@charliermarsh charliermarsh added the bug Something isn't working label Mar 8, 2024
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
…#10283)

## Summary

Fix astral-sh#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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser Related to the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parameter range is incorrect for *args and **kwargs
4 participants