Repository navigation
[refurb] Fix FURB163 autofix creating a syntax error for yield expressions - #18756
Conversation
ntBre
left a comment
There was a problem hiding this comment.
Looks good, thanks! I tested a couple of other potentially-tricky cases like await and if expressions, but I think yield and yield from are the only problematic forms. We just need to resolve the merge conflicts, and then this is good to go.
Done! |
|
| )?; | ||
|
|
||
| let number = checker.locator().slice(arg); | ||
| let arg_str = if matches!(arg, Expr::Yield(_) | Expr::YieldFrom(_)) { |
There was a problem hiding this comment.
Could we use OperatorPrecedence here instead of hard coding Yield and YieldFrom
There was a problem hiding this comment.
Is there any benefit of using OperatorPrecedence over the matches!(arg, Expr::Yield(_) | Expr::YieldFrom(_))?
There was a problem hiding this comment.
I guess it would be more general in case something changes in the future and also make it easier to find precedence related checks. I haven't actually used OperatorPrecedence myself, but it looks like Yield has the lowest precedence, so something like OperatorPrecedence::from(arg) <= OperatorPrecedence::Yield should be equivalent to the matches! call?
Is that what you had in mind @MichaReiser?
There was a problem hiding this comment.
Looking at the fix. I'm also wondering if we could simply preserve any existing parentheses because that would fix it too.
OperatorPrecedence::from(arg) <= OperatorPrecedence::Yield
Roughly. Ideally there would be a , OperatorPrecedence but that doesn't exist. The main advantage is that this rule would be correct if we ever end up introducing a new OperatorPrecedence that binds lower than Yield (and we can check all Yield usages if it binds one level higher than Yield).
Summary
Fixes #18747
Test Plan
Add regression test