Introduce an Arguments AST node for function calls and class definitions
#6259
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
charliermarsh
commented
Aug 1, 2023
| args, | ||
| keywords, | ||
| range: _, | ||
| }, |
There was a problem hiding this comment.
I tended to favor destructuring to minimize code changes (i.e., we didn't have to change this method at all, only the let match here).
charliermarsh
commented
Aug 1, 2023
| decorator.expression.range(), | ||
| diagnostic.set_fix(Fix::automatic(Edit::deletion( | ||
| arguments.start(), | ||
| arguments.end(), |
There was a problem hiding this comment.
This is an improvement (delete from left to right parens).
charliermarsh
commented
Aug 1, 2023
| diagnostic.set_fix(Fix::automatic(Edit::deletion( | ||
| arguments.start(), | ||
| arguments.end(), | ||
| ))); |
There was a problem hiding this comment.
This is way simpler (delete from left to right paren).
PR Check ResultsEcosystem✅ ecosystem check detected no changes. BenchmarkLinuxWindows |
charliermarsh
force-pushed
the
charlie/class-base-comments
branch
from
e628cb0
to
0f87e9b
Compare
|
(I am going to leverage these in the formatter in a separate change, to fix the trailing comment bug we have today.) |
charliermarsh
force-pushed
the
charlie/class-base-comments
branch
from
0f87e9b
to
6bd209c
Compare
zanieb
reviewed
Aug 2, 2023
crates/ruff/src/rules/flake8_builtins/rules/builtin_attribute_shadowing.rs
Outdated
Show resolved
Hide resolved
charliermarsh
force-pushed
the
charlie/class-base-comments
branch
from
6bd209c
to
09fa5b0
Compare
charliermarsh
added a commit
that referenced
this pull request
Aug 2, 2023
## Summary Similar to #6259, this PR adds a `TypeParams` node to the AST, to capture the list of type parameters with their surrounding brackets. If a statement lacks type parameters, the `type_params` field will be `None`.
charliermarsh
added a commit
that referenced
this pull request
Aug 2, 2023
## Summary This PR leverages the `Arguments` AST node introduced in #6259 in the formatter, which ensures that we correctly handle trailing comments in calls, like: ```python f( 1, # comment ) pass ``` (Previously, this was treated as a leading comment on `pass`.) This also allows us to unify the argument handling across calls and class definitions. ## Test Plan A bunch of new fixture tests, plus improved Black compatibility.
MichaReiser
reviewed
Aug 3, 2023
Comment on lines
+169
to
+175
| pub fn bases(&self) -> impl Iterator<Item = &Expr> { | ||
| self.arguments | ||
| .as_ref() | ||
| .map(|arguments| &arguments.args) | ||
| .into_iter() | ||
| .flatten() | ||
| } |
There was a problem hiding this comment.
Nit: We could return a slice here by using
Suggested change
| pub fn bases(&self) -> impl Iterator<Item = &Expr> { | |
| self.arguments | |
| .as_ref() | |
| .map(|arguments| &arguments.args) | |
| .into_iter() | |
| .flatten() | |
| } | |
| pub fn bases(&self) -> &[Expr] { | |
| match &self.arguments { | |
| Some(arguments) => &arguments.args, | |
| None => &[], | |
| } | |
| } |
MichaReiser
reviewed
Aug 3, 2023
| @@ -2951,9 +3003,9 @@ mod size_assertions { | |||
| use super::*; | |||
| use static_assertions::assert_eq_size; | |||
|
|
|||
| assert_eq_size!(Stmt, [u8; 168]); | |||
| assert_eq_size!(Stmt, [u8; 176]); | |||
charliermarsh
added a commit
that referenced
this pull request
Aug 3, 2023
Slices are strictly more flexible, since you can always convert to an iterator, etc., but not the other way around. Suggested in #6259 (comment).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds a new
ArgumentsAST node, which we can use for function calls and class definitions.The
Argumentsnode spans from the left (open) to right (close) parentheses inclusive.In the case of classes, the
Argumentsis an option, to differentiate between:In this PR, we don't really leverage this change (except that a few rules get much simpler, since we don't need to lex to find the start and end ranges of the parentheses, e.g.,
crates/ruff/src/rules/pyupgrade/rules/lru_cache_without_parameters.rs,crates/ruff/src/rules/pyupgrade/rules/unnecessary_class_parentheses.rs).In future PRs, this will be especially helpful for the formatter, since we can track comments enclosed on the node itself.
Test Plan
cargo test