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

Add convenience methods for iterating over all parameter nodes in a function #11174

Merged

Conversation

AlexWaygood
Copy link
Member

Summary

A pretty common pattern in our linter rules is the need to iterate over all parameters in a function. That ends up looking pretty verbose currently -- something like this:

for parameter in parameters
    .posonlyargs
    .iter()
    .chain(&parameters.args)
    .map(|param| param.parameter)
    .chain(parameters.vararg.as_deref())
    .chain(parameters.kwonlyargs.iter().map(|param| param.parameter))
    .chain(parameters.kwarg.as_deref())
{
    do_the_check()
}

It's also pretty easy to get this pattern subtly incorrect: for example, several of our rules currently incorrectly do parameters.args.iter().chain(&parameters.posonlyargs), but positional-only parameters always precede positional-or-keyword parameters in Python.

This PR adds two convenience methods to the ast::Parameters struct in ruff_python_ast/src/nodes.rs, to make iterating over all parameters more ergonomic. iter_non_variadic_params() iterates over just the non-variadic parameters (posonlyargs, args and kwonlyargs), and iter_all_params() iterates over all parameters in a function, including any variadic paramerers (vararg and kwarg). Using these convenience methods in our linter rules significantly reduces duplication.

Test Plan

cargo test. One parser snapshot changes slightly, but I think it's for the better: the syntax errors are now reported in the order that they appear in the source code. Previously, they were reported out of order.

@AlexWaygood AlexWaygood added the internal An internal refactor or improvement label Apr 27, 2024
@AlexWaygood AlexWaygood changed the title Add convenience methods for iterating over all parameters in a function Add convenience methods for iterating over all parameter nodes in a function Apr 27, 2024
Copy link
Contributor

github-actions bot commented Apr 27, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

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 like this a lot. I did not review all code changes. But we should have done this a long time ago :D

crates/ruff_python_ast/src/nodes.rs Outdated Show resolved Hide resolved
crates/ruff_python_ast/src/nodes.rs Outdated Show resolved Hide resolved
Comment on lines 61 to 67
if parameters
.posonlyargs
.iter()
.chain(&parameters.args)
.chain(&parameters.kwonlyargs)
.count()
> 1
Copy link
Member

Choose a reason for hiding this comment

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

The advantage of defining your own iterator type (vs using chaining) is that you could implement ExactSizeIterator and you could then call .len() here.

But I think the better solution here is to add a method to parameters to get this length

Copy link
Member Author

Choose a reason for hiding this comment

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

But I think the better solution here is to add a method to parameters to get this length

Currently the rule is doing something a little odd: it's checking how many nonvariadic parameters there are in this function, but paying no attention to how many variadic parameters there are. That makes no sense to me based on what the rule is supposed to be checking: I think it makes just as much sense to emit an error if an @property-decorated function has *args or **kwargs parameters, as that's just as invalid.

I think maybe I'll just revert all changes to this file for now, and fix the bug (at least, I think it's a bug) in a separate PR, with added tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Followup PR at #11200

@AlexWaygood AlexWaygood force-pushed the parameter-convenience-iterator branch 2 times, most recently from 3792179 to 8ace18c Compare April 27, 2024 21:19
@AlexWaygood AlexWaygood force-pushed the parameter-convenience-iterator branch from 2010e4d to 0323a78 Compare April 29, 2024 10:26
@AlexWaygood AlexWaygood enabled auto-merge (squash) April 29, 2024 10:29
@AlexWaygood AlexWaygood merged commit 87929ad into astral-sh:main Apr 29, 2024
19 checks passed
@AlexWaygood AlexWaygood deleted the parameter-convenience-iterator branch April 29, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants