[flake8-pyi] Avoid syntax error in the case of starred and keyword arguments (PYI059)
#18611
Conversation
ntBre
added
bug
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PYI059 | 26 | 0 | 0 | 0 | 26 |
| // adapted from `add_argument`, which doesn't automatically handle inserting before the first | ||
| // keyword argument. | ||
| let insertion = if let Some(ast::Keyword { range, value, .. }) = arguments.keywords.first() { | ||
| let keyword = parenthesized_range(value.into(), arguments.into(), comment_ranges, source) | ||
| .unwrap_or(*range); | ||
| Edit::insertion(format!("{argument}, "), keyword.start()) | ||
| } else { | ||
| add_argument(argument, arguments, comment_ranges, source) | ||
| }; |
There was a problem hiding this comment.
should we be adding this logic to add_argument, so that it's also fixed for other rules that use that utility? If we don't want to do that in this PR, is it worth opening a followup issue for that?
There was a problem hiding this comment.
Yeah, I actually checked the other callers of add_argument, and they were all adding keyword arguments! So I think it makes sense to fix this or add a note to the docs at the very least.
I don't mind doing it here, but I guess it might churn some snapshots to move the existing calls to the front of the kwarg list.
There was a problem hiding this comment.
I'm happy with doing it here or postponing to a followup -- either is fine by me!
There was a problem hiding this comment.
I went ahead and did it here. It might have even shortened the net diff!
You're welcome to review again of course, but otherwise I'll just merge this when CI finishes. Thanks for the help!
| add_argument(argument, arguments, comment_ranges, source) | ||
| }; | ||
|
|
||
| Ok(Fix::safe_edits(deletion, [insertion])) |
There was a problem hiding this comment.
Hmm... it's unrelated to your PR, but I'm not sure that this should be a safe fix :-(
Moving the position of Generic[] in the bases tuple could change the behaviour of the code by changing the class's MRO, and it's possible that the code was actually working before the fix. (Having Generic[] as not-the-last base class can sometimes work, it's just... never advisable.)
Even if we're okay with the fix changing the behaviour of the code, is there a possibility it could delete comments in some situations? We discussed this in the original PR so I think we have good test coverage for it, but at that point in time we didn't have as rigorous a policy for whether a fix should be deemed to be unsafe if it removed comments: #11233 (comment)
There was a problem hiding this comment.
I was wondering about that too, thanks for bringing it up.
It definitely removes comments (playground) trailing the Generic argument.
I'd be fine marking this always unsafe and adding a ## Fix safety section saying that it can change the MRO and also delete comments, if that sounds good to you.
This seems like a lot of changes for a rule about to be stabilized. How are you feeling about that? I'm definitely fine leaving this in preview for another cycle.
There was a problem hiding this comment.
I'd be fine marking this always unsafe and adding a
## Fix safetysection saying that it can change the MRO and also delete comments, if that sounds good to you.
That sounds good to me, yeah!
This seems like a lot of changes for a rule about to be stabilized. How are you feeling about that? I'm definitely fine leaving this in preview for another cycle.
Agreed -- let's make the changes proposed in this PR and the docs updates proposed in #18601, but leave it in preview for now?
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
* main: [`pylint`] De-emphasize `__hash__ = Parent.__hash__` (`PLW1641`) (#18613) [`flake8-pyi`] Avoid syntax error in the case of starred and keyword arguments (`PYI059`) (#18611) [ty] Add support for global __debug__ constant (#18540) [`ruff`] Preserve parentheses around `deque` in fix for `unnecessary-empty-iterable-within-deque-call` (`RUF037`) (#18598) [`refurb`] Parenthesize lambda and ternary expressions in iter (`FURB122`, `FURB142`) (#18592)
## Summary Fixes #18612 by: - Bailing out without a fix in the case of `*args`, which I don't think we can fix reliably - Using an `Edit::deletion` from `remove_argument` instead of an `Edit::range_replacement` in the presence of unrecognized keyword arguments I thought we could always switch to the `Edit::deletion` approach initially, but it caused problems when `maxlen` was passed positionally, which we didn't have any existing tests for. The replacement fix can easily delete comments, so I also marked the fix unsafe in these cases and updated the docs accordingly. ## Test Plan New test cases derived from the issue. ## Stabilization These are pretty significant changes, much like those to PYI059 in #18611 (and based a bit on the implementation there!), so I think it probably makes sense to un-stabilize this for the 0.12 release, but I'm open to other thoughts there.
Summary
Fixes #18602 by:
*argsare presentGenericbase class right before the first keyword argument, if one is presentIn an intermediate commit, I also had special handling to avoid a fix in the
**kwargscase, but this is treated (roughly) as a normal keyword, and I believe handling it properly falls out of the other keyword fix.I also updated the
add_argumentutility function to insert new arguments right before the keyword argument list instead of at the very end of the argument list. This changed a couple of snapshots unrelated toPYI059, but there shouldn't be any functional changes to other rules because all other calls toadd_argumentwere adding a keyword argument anyway.Test Plan
Existing PYI059 cases, plus new tests based on the issue