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

Formatter: Don't reorder parameters in function calls #6498

Closed
Tracked by #6069
konstin opened this issue Aug 11, 2023 · 7 comments · Fixed by #7268
Closed
Tracked by #6069

Formatter: Don't reorder parameters in function calls #6498

konstin opened this issue Aug 11, 2023 · 7 comments · Fixed by #7268
Assignees
Labels
bug Something isn't working formatter Related to the formatter help wanted Contributions especially welcome

Comments

@konstin
Copy link
Member

konstin commented Aug 11, 2023

Currently, we format

f(a=b, *args, **kwargs)

as

f(*args, a=b, **kwargs)

Instead, we should preserve the function order.

@konstin konstin added bug Something isn't working formatter Related to the formatter help wanted Contributions especially welcome labels Aug 11, 2023
@MichaReiser MichaReiser added this to the Formatter: Alpha milestone Aug 11, 2023
@NeilGirdhar
Copy link

NeilGirdhar commented Aug 11, 2023

Why do you think that's better? The original order was contentious when we implemented PEP 448. And there's one good reason to do the reorder:

def g():
    print("g")
    return range(2)
def h():
    print("h")
    return 1

def f(*args, **kwargs):
    pass

f(a=h(), *g(), **{})  # Prints g then h!

Positional arguments are evaluated first.

If anything, you should probably add a Ruff rule to warn on keyword arguments being first especially if they have any side effects. The only problem with warning unconditionally is this pattern:

f(x=1, y=2, *args, **kwargs)

@charliermarsh
Copy link
Member

I believe we do warn when keyword arguments are provided in such an order: https://beta.ruff.rs/docs/rules/star-arg-unpacking-after-keyword-arg/ (but it's unconditional)

@MichaReiser
Copy link
Member

This could be hard with an AST-based approach (assuming I understand the problem correctly).

@NeilGirdhar The main reason for preserving the order is that this change goes beyond just formatting your code. It re-orders code (and potentially re-orders comments too). This is a good case for a lint rule that you can use in addition to the formatter.

@NeilGirdhar
Copy link

@charliermarsh My mistake, I tested it but forgot to enable all the rules 😄

@MichaReiser Fair enough, sorry for the noise!

@charliermarsh
Copy link
Member

@NeilGirdhar - You're telling me you haven't memorized every single rule in Ruff?

@charliermarsh
Copy link
Member

I honestly didn't realize that the evaluation order was independent of the literal order. I should update that rule documentation to include this, it's interesting.

@NeilGirdhar
Copy link

Good idea.

It's one of the few places I think. Really glad to see you have rule B026. Definitely an important one for preventing bugs.

Do you have statistics on how often your various rules are hit in public code? If this rule is hit very rarely, we may be able to convince the Python people to syntax-warn, and eventually deprecate this form.

konstin added a commit that referenced this issue Sep 13, 2023
## Summary

In `f(*args, a=b, *args2, **kwargs)` the args (`*args`, `*args2`) and
keywords (`a=b`, `**kwargs`) are interleaved, which we previously didn't
handle.

Fixes #6498

**main**

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1632 |
| **django** | 0.99966 | 2760 | 58 |
| transformers | 0.99930 | 2587 | 447 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99825 | 648 | 22 |
| zulip | 0.99950 | 1437 | 27 |

**PR**

| project | similarity index | total files | changed files |

|--------------|------------------:|------------------:|------------------:|
| cpython | 0.76083 | 1789 | 1632 |
| **django** | 0.99967 | 2760 | 53 |
| transformers | 0.99930 | 2587 | 447 |
| twine | 1.00000 | 33 | 0 |
| typeshed | 0.99983 | 3496 | 18 |
| warehouse | 0.99825 | 648 | 22 |
| zulip | 0.99950 | 1437 | 27 |


## Test Plan

New fixtures
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working formatter Related to the formatter help wanted Contributions especially welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants