-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix(#19757)/isc003 autofix produces incorrect code #19778
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
Fix(#19757)/isc003 autofix produces incorrect code #19778
Conversation
… inside brackets (astral-sh#19757) ## Summary This update introduces stricter behavior for the ISC003 rule by detecting explicitly concatenated strings only if they are within brackets. Adds a helper function `is_inside_brackets` to verify bracket context and adjusts diagnostics accordingly. ## Test Plan ```bash cargo test ```
…d optimize imports
|
…iagnostics for ISC002 and ISC003
…mproved readability
ntBre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer a different approach here, but thanks for working on it!
| } | ||
| } | ||
|
|
||
| fn is_inside_brackets(checker: &Checker, expr_range: TextRange) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel like the right fix to me, and processing characters individually like this seems very brittle. My understanding of the issue in the report was that we just need to check if either the left- or right-hand-side of the binary concatenation operator is parenthesized because the fix simply strips out the + between them.
This is just a limitation in the concatable check because Ruff knows that ("1" "2") resolves to a single StringLiteral node in the AST.
I think we can probably just use this helper function to check if left and right are parenthesized and avoid the fix if either of them is:
ruff/crates/ruff_python_ast/src/parenthesize.rs
Lines 58 to 62 in 4d57fcd
| /// Returns the [`TextRange`] of a given expression including parentheses, if the expression is | |
| /// parenthesized; or `None`, if the expression is not parenthesized. | |
| pub fn parenthesized_range( | |
| expr: ExprRef, | |
| parent: AnyNodeRef, |
We can also still report a diagnostic in this case, we just can't autofix it. Another option would be to try to move one of the expressions into the existing parentheses from the other expression, but that might be too involved. I'd be happy with just offering a diagnostic but no fix if one of them is parenthesized to avoid causing the runtime error.
|
@mikeleppane thanks for working on this. Do you plan to come back to this PR or should we close it (don't feel pressued that you have to, just checking in on the status of this PR is). |
|
Thanks for working on this fix. I'll close this PR due to inactivity. A new PR (from you or anyone else who wants to work on this fix) with Brent's feedback would be welcomed. |
Summary
Problem
The ISC003 rule was flagging explicit string concatenation (
"a" + "b") in all contexts, but it should only apply when the concatenation is inside brackets (parentheses or braces) where implicit concatenation is possible.Solution
Added a new
is_inside_brackets()helper function that:(or{) before the expression)or}) after the expressionExample
Input
ruff check --fix=>ISSUE
Test Plan
cargo test