New rule to prevent implicit string concatenation in collections#21972
Merged
ntBre merged 10 commits intoastral-sh:mainfrom Dec 17, 2025
Merged
New rule to prevent implicit string concatenation in collections#21972ntBre merged 10 commits intoastral-sh:mainfrom
ntBre merged 10 commits intoastral-sh:mainfrom
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ISC004 | 342 | 342 | 0 | 0 | 0 |
Member
|
FWIW This was one of the more impactful lint rules in Fixit that prevented several serious outages at Instagram/Meta |
MichaReiser
reviewed
Dec 15, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
Thank you. This rule makes sense to me
crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs
Outdated
Show resolved
Hide resolved
ntBre
added
rule
ntBre
reviewed
Dec 16, 2025
Contributor
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thank you, this looks great!
I only had one comment with a couple of small ideas, but I'm also happy with the current state of the diagnostic.
I do agree with Micha that we should make this an unsafe fix, if we keep the fix, though.
I think we should also touch base upstream. They expressed interest in adding this rule as well (flake8-implicit-str-concat/flake8-implicit-str-concat#55), so we should double check the error code to stay in sync, if we make it an ISC rule, which I think makes sense otherwise.
Have you had a chance to go through the ecosystem results? Cases like this feel a bit unfortunate to me:
assert not np.isclose(val, expected, rtol=1e-16, atol=0.0), (
f"Permittivity value test gives {val} and should not be "
f"equal to {expected}.",
)Maybe we should exclude cases with a single element?
crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs
Outdated
Show resolved
Hide resolved
Member
|
I think I'm fine with assert not np.isclose(val, expected, rtol=1e-16, atol=0.0), (
f"Permittivity value test gives {val} and should not be "
f"equal to {expected}.",
)because the trailing |
Contributor
Author
|
Thank you both!
Like Micha says, this should be a string not a tuple. There were several accidental tuples in my work codebase, so this would be good to keep. (Most of these were asserts, we could consider a separate rule that complains about single element str tuples as assert message)
Moved it to a note, thanks!
Made it an unsafe fix, thanks!
I prefer "unparenthesized" because it makes it clearer what the lint rule wants you to do
Commented upstream
Yeah, I looked at them, felt mostly positive / expected hits to me. In a large internal codebase this has caught dozens and dozens and dozens of bugs, and is a readability improvement in most of the other cases. |
ntBre
approved these changes
Dec 16, 2025
crates/ruff_linter/src/rules/flake8_implicit_str_concat/rules/collection_literal.rs
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Fixed, and all is green! |
Contributor
|
Thanks again! I might give upstream at least until tomorrow to make sure there are no objections on the rule code (I take the 🚀 react on your comment as a positive sign), but this is otherwise good to go and should land before our release on Thursday. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a common footgun, see the example in #13014 (comment)
Fixes #13014 , fixes #13031